yjs / yjs

Shared data types for building collaborative software
https://docs.yjs.dev
Other
15.95k stars 583 forks source link

Decoding ydoc with undefined properties in ytext delta throws error #466

Open ralphiee22 opened 1 year ago

ralphiee22 commented 1 year ago

Checklist

Describe the bug Apply a delta (with undefined properties) to a ytext object. Encode the associated ydoc with encodeStateAsUpdate. Try applying the newly encoded state to another ydoc. JSON parse error is thrown.

To Reproduce

const Y = require('yjs');  // 13.5.39

const ydoc = new Y.Doc();

const ytext = ydoc.getText('id');

const delta = [{ insert: 'Test', attributes: { color: undefined } }];

ytext.applyDelta(delta);

const encodedState = Y.encodeStateAsUpdate(ydoc);

const ydoc2 = new Y.Doc();

Y.applyUpdate(ydoc2, encodedState);

Expected behavior Decoding to work properly with undefined properties.

Screenshots

SyntaxError: Unexpected token u in JSON at position 0
    at JSON.parse (<anonymous>)
    at UpdateDecoderV1.readJSON (yjs@13.5.39/node_modules/yjs/dist/yjs.cjs:770:17)
    at Array.readContentFormat (yjs@13.5.39/node_modules/yjs/dist/yjs.cjs:8550:83)
    at readItemContent (yjs@13.5.39/node_modules/yjs/dist/yjs.cjs:9712:87)
    at readClientsStructRefs (yjs@13.5.39/node_modules/yjs/dist/yjs.cjs:1368:13)
    at yjs@13.5.39/node_modules/yjs/dist/yjs.cjs:1595:16
    at transact (yjs@13.5.39/node_modules/yjs/dist/yjs.cjs:3266:5)
    at readUpdateV2 (yjs@13.5.39/node_modules/yjs/dist/yjs.cjs:1588:3)
    at applyUpdateV2 (yjs@13.5.39/node_modules/yjs/dist/yjs.cjs:1683:3)
    at Object.applyUpdate (yjs@13.5.39/node_modules/yjs/dist/yjs.cjs:1697:58)

Environment Information

Additional context I am not sure if YJS should be handling the undefined properties (or fail more gracefully), or we should never be passing undefined properties to the underlying shared types.

Huly®: YJS-317

dmonad commented 1 year ago

Hi @ralphiee22,

This is a problem because undefined is not valid JSON. You can try it out yourself: JSON.parse("{ val: undefined }") throws an exception as well.

I'm not quite sure yet what the best course of action is. In any case, you should not set undefined as a value. Hence, it would probably be best if Yjs would already throw an exception when you try to set undefined.

ralphiee22 commented 1 year ago

Hence, it would probably be best if Yjs would already throw an exception when you try to set undefined.

I think this would be too much of a performance hit to verify there are no undefined properties being set, as we would need to iterate over the entire object (which is what we are doing as a workaround before encoding the document).

dmonad commented 1 year ago

Formatting attributes in Yjs are JSON encoded and sent over the wire. Hence, Yjs cant accept undefined values as formatting attributes because they can't be JSON encoded. I can't change that.

In the future I will throw an error if someone tries to set undefined. I will leave the ticket open only to track that.