valtiojs / valtio-yjs

valtio-yjs makes yjs state easy
MIT License
270 stars 15 forks source link

Maximum Call Stack Exceeded when referencing recursive object #46

Closed BrianUribe6 closed 11 months ago

BrianUribe6 commented 11 months ago

Hello, First of all awesome library it has saved me countless hours of effort when integrating Yjs and React.

I've recently ran into an issue when attempting to use a tree-like state. My state object looks something like this:

type  Item = {
 parent: Item | Item[]  // <--- This causes infinite recursion when binding
 label: string;
 children: Item[]
}

const doc = new Y.Doc();
const state = proxy<Item[]>([]);

bind(state, doc.getArray("sidebar");

function addItem(parent: Item | Item[], item: Item) {
  if (Array.isArray(parent)) {
    parent.push(item);
  } else {
    parent.children.push(item);
   }
   item.parent = parent;
}

I need to keep track of the parent of each item, and also allow adding child elements to any Item.

bind seems to infinitely recurse when trying to access the parent .

I've a created a minimal reproduction:

https://codesandbox.io/p/sandbox/valtio-yjs-recursive-type-bug-c3hjhh?file=%2Fsrc%2FApp.js%3A14%2C1

Thank you!

raineorshine commented 11 months ago

valtio-yjs has to convert the proxy into a valid YJS structure to work. Is there a valid YJS structure with circular references? I get an error when I try it:

import * as Y from 'yjs'

const doc = new Y.Doc()
const root = doc.getMap()
const parent = new Y.Map()
const child = new Y.Map()

root.set('parent', parent)
parent.set('child', child)

// ERROR: Cannot read properties of null (reading 'forEach')
child.set('parent', parent)

console.log(doc.toJSON())

https://codesandbox.io/p/devbox/yjs-ult-and-penult-forked-64j2h9

BrianUribe6 commented 11 months ago

valtio-yjs has to convert the proxy into a valid YJS structure to work. Is there a valid YJS structure with circular references? I get an error when I try it:

import * as Y from 'yjs'

const doc = new Y.Doc()
const root = doc.getMap()
const parent = new Y.Map()
const child = new Y.Map()

root.set('parent', parent)
parent.set('child', child)

// ERROR: Cannot read properties of null (reading 'forEach')
child.set('parent', parent)

console.log(doc.toJSON())

https://codesandbox.io/p/devbox/yjs-ult-and-penult-forked-64j2h9

Interesting, I modified your example like this. It still produces the same error:

import * as Y from 'yjs'

const doc = new Y.Doc();
const root = doc.getMap();
const child = new Y.Map();

root.set("child", child);
child.set("parent", root); // <- Error on circular reference

console.log(child)

Does this mean that Yjs does not support any data structure with circular references? or is this a bug?

PhilGarb commented 11 months ago

I think this has to do with yjs itself. It is not possible to insert a shared type twice in a document like the author explained here: https://github.com/yjs/yjs/issues/458#issuecomment-1224800219. If only the data is relevant you could: root.clone(). I am not sure that circular references are even supported by yjs.

raineorshine commented 11 months ago

The solution is to assign a unique id to each Item, and reference the parent and children by id, i.e. foreign key. Then you can store all items in a single Map.

For an even more normalized structure, you can store nodes and edges in separate Maps.

BrianUribe6 commented 11 months ago

Thanks everyone for your assistance. I encountered this issue after doing some refactoring on my code. Previously, I used to pass the child and parent references as separate values around the application. I thought it would be a good idea to combine them so that I could do child.parent instead, but now I understand why it is not possible.

The solution is to assign a unique id to each Item, and reference the parent and children by id, i.e. foreign key. Then you can store all items in a single Map.

I will be taking @raineorshine suggestions, since having to separately keep track of every node and its parent is very repetitive.

I'll mark this as closed. Once again thank you all!