"Cannot read property 'forEach' of null" when trying to set property to object returned by reactive-crdt previously

[rhaenni]

The vue todo example included in this repo has this problem when testing the "Clear Completed" functionality. It filters the todo's array by completed todo objects and then tries to set the todos array to the filtered array leading to this error TypeError: Cannot read property 'forEach' of null

simplified code to reproduce:

import { crdt, Y } from "./packages/reactive-crdt/dist/reactive-crdt.js";

const doc1 = new Y.Doc();
let store1 = crdt(doc1);
store1.arr = [];
store1.arr.push({ title: "Todo 1", completed: true });
store1.arr.push({ title: "Todo 2", completed: false });
let filtered_array = store1.arr.filter(x => !x.completed);
store1.testit = filtered_array[0]; // this will throw error

stack trace

    ;/** @type {Map<string, any>} */ (this._prelimContent).forEach((value, key) => {
                                                           ^

TypeError: Cannot read property 'forEach' of null
    at YMap._integrate (C:\reactive-crdt\node_modules\yjs\dist\yjs.cjs:5438:60)
    at ContentType.integrate (C:\reactive-crdt\node_modules\yjs\dist\yjs.cjs:8714:15)
    at Item.integrate (C:\reactive-crdt\node_modules\yjs\dist\yjs.cjs:9263:20)
    at C:\reactive-crdt\node_modules\yjs\dist\yjs.cjs:4904:20
    at Array.forEach (<anonymous>)
    at typeListInsertGenericsAfter (C:\reactive-crdt\node_modules\yjs\dist\yjs.cjs:4880:11)
    at typeListInsertGenerics (C:\reactive-crdt\node_modules\yjs\dist\yjs.cjs:4933:12)
    at C:\reactive-crdt\node_modules\yjs\dist\yjs.cjs:5258:9
    at transact (C:\reactive-crdt\node_modules\yjs\dist\yjs.cjs:3201:5)
    at YArray.insert (C:\reactive-crdt\node_modules\yjs\dist\yjs.cjs:5257:7)
    at YArray._integrate (C:\reactive-crdt\node_modules\yjs\dist\yjs.cjs:5205:10)
    at ContentType.integrate (C:\reactive-crdt\node_modules\yjs\dist\yjs.cjs:8714:15)
    at Item.integrate (C:\reactive-crdt\node_modules\yjs\dist\yjs.cjs:9263:20)
    at typeMapSet (C:\reactive-crdt\node_modules\yjs\dist\yjs.cjs:5067:130)
    at C:\reactive-crdt\node_modules\yjs\dist\yjs.cjs:5573:9
    at transact (C:\reactive-crdt\node_modules\yjs\dist\yjs.cjs:3201:5)
    at YMap.set (C:\reactive-crdt\node_modules\yjs\dist\yjs.cjs:5572:7)
    at Object.set (C:\reactive-crdt\packages\reactive-crdt\dist\reactive-crdt.js:91:13)
    at file:///C:/reactive-crdt/test.mjs:22:12

tried to debug and found that the object proxied by reactive-crdt is not "unwrapped" before passing it to yjs but not sure why not or if that's really the problem

[rhaenni]

this error happens because yjs does not allow moving already inserted types, see https://github.com/yjs/docs/blob/main/getting-started/working-with-shared-types.md#caveats

implemented a workaround for reactive-crdt to unwrap the existing type before reinserting it into a new location, unsure about the implications. code can be found here https://github.com/rhaenni/reactive-crdt/commit/243f9ed310d450bf0b34bd27dcccc8eb75306793

[YousefED]

@rhaenni Thanks for reporting this, seems like a bug indeed. Although your workaround is quite clever, I think it will have some unintended consequences. Let's say:

• Alice (offline) changes the title of an item X
• Bob (offline) calls todos = todos.filter(todo => true)
• Bob and Alice sync
• The changes of Alice won't be reflected, as Bob has cloned his original data.

In other words, the store1.arr = store1.arr.filter(x => !x.completed) statement is not "conflict-proof". Your workaround makes it work, but I'd prefer to keep the error to make it explicit. Then the consumer (in this case my example code) can fix the code in one of the following ways:

explicity copy Use store1.arr = store1.arr.toJSON().filter(x => !x.completed). This still wouldn't sync Alice's changes, but at least makes it more explicit because it's clear that data is copied (which kind of makes it explicit that we're escaping auto-sync)

datastructure that supports deletes add a deleted boolean property to todoitems

delete items Instead of store1.arr = store1.arr.filter(x => !x.completed), explicitly delete completed items (e.g.: using splice).

What do you think?

[rhaenni]

Agreed, the workaround will lead to conflicts and raising an error is much better.

Should reactive-crdt support all native JS array/object features as much as possible? Personally I would prefer it it would, where not possible it should be documented.

It might make sense to raise a better error in this specific move case as the error message from Yjs is cryptic and does not give much information, reactive-crdt could check when a user is moving a yjs type with little overhead (just parent !== value.parent check) and return a better error message, happy to contribute this.

In the future maybe Yjs will support moving of Yjs types within the Y.doc, Yjs's author said probably not this year though: https://discuss.yjs.dev/t/moving-elements-in-lists/92/22

Another option is to support moving by adding an internal wrapper data structure around arrays that would allow moving elements (some examples given in the Yjs thread above), this adds quite some overhead but would allow easier usage of reactive-crdt as users may not be aware of limitations since they are basically working with native JS arrays.

I can think of other problems like this, e.g. with sparse arrays as that is also not allowed in Yjs and probably there are more. Would be great if you could share your thinking on these cases, should reactive-crdt just document the limitations or should it attempt to workaround if possible?

[YousefED]

Should reactive-crdt support all native JS array/object features as much as possible? Personally I would prefer it it would, where not possible it should be documented.

Yes, we should implement all array methods. Of course, the issue we're running into is that there is no native JS .filterItems (or it's inverse; deleteWhere) which filters items in place, so it's split into two operations (filtering and assignment) which make it more difficult to detect. I'm not sure yet what the best design for this use-case would be.

It might make sense to raise a better error in this specific move case as the error message from Yjs is cryptic and does not give much information, reactive-crdt could check when a user is moving a yjs type with little overhead (just parent !== value.parent check) and return a better error message, happy to contribute this.

Agree, we should detect do a parent check and throw a better error. PR welcome!

Regarding moving / sparse arrays I would love to see support for fractional indexing, but not sure it should be part of this package or a separate module that can be used by yjs standalone. Note that we'd run into the same issue described above; JS arrays don't have a native move function (just like it doesn't have a native filterItems function). So there is not really a method for us to "polyfill".

This would leave two options:

• Try to detect move and filterItem operations automatically when assigning to store.arrayValue, by comparing whether assigned items already occured in the array previously. I haven't thought this through, but it might introduce new edge cases.
• Add Array.move and Array.filterItems methods. This has the downside that the API will be different from "native JS Arrays"

Thoughts?

---

PS: As reactive-CRDT is a fairly new project I love hearing about use-cases and your experience so far :)

[rhaenni]

My only use case so far is the vue todo example although long-term my use case would be a fireyjs, so others may have more experience insights to share :) I love that reactive-crdt just uses native JS data types and maps them to yjs, hence I would not be looking for Array.move or other non native JS functions, it's also very usable already albeit lacking many native JS array/object functions (but they can probably all be implemented).

To support move I made a crude proof of concept that changes the data structure in the yjs.doc to keep an array of all yjs data types and then references those from the actual data structure, this allows reinserting (moving) or assigning the same yjs data type twice to different locations in the document, basically just like a native javascript object would behave. Code: https://github.com/YousefED/reactive-crdt/compare/main...rhaenni:compatmode

This would add overhead and complexity however personally I think it may be acceptable in the spirit of being compatible as much as possible with native JS.

The proof of concept is very crude as it does not delete unreferenced objects, abuses strings with a harcoded prefix as reference pointers and probably introduces some edge cases. As you mentioned above it may also make more sense to do something like this within yjs instead of reactive-crdt.