valtiojs / valtio-yjs

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

Infinite loop when setting a nested object #14

Closed fson closed 2 years ago

fson commented 3 years ago

I'm trying to use valtio-yjs to bind a Valtio store with nested objects to YJS. I noticed that if I have 2 levels of nesting in the proxy, and set the inner object to a new plain object, this results in an infinite loop in valtio-yjs:

const ydoc = new Y.Doc();
const ymap = ydoc.getMap("objects");
const state = proxy({ items: { item1: { title: "hello" } } });
bindProxyAndYMap(state, ymap);

state.items.item1 = { title: "goodbye" };  // Page becomes unresponsive!

After setting item1, the library seems to keep alternating between setPValueToY and setYValueToP and the whole page becomes unresponsive.

Here's a codesandbox repro for the issue: https://codesandbox.io/s/valtio-yjs-demo-forked-w3otm?file=/src/App.js (press "change title" to trigger the infinite loop).

dai-shi commented 3 years ago

Oh, nice catch! Infinite loop is bad...

fson commented 3 years ago

Alright, spent a bunch of time debugging, here's what I found: The library adds multiple (2 or more) observers to nested objects. I think this is due to these recursive calls to bindProxyAndYMap: https://github.com/dai-shi/valtio-yjs/blob/bb5704540e38093228861426c43530de9fa58670/src/index.ts#L126 https://github.com/dai-shi/valtio-yjs/blob/bb5704540e38093228861426c43530de9fa58670/src/index.ts#L103

Each call to bindProxyAndYMap adds one Valtio subscription and one YJS observer.

The more deeply nested the object is, the more subscriptions/observers get added. For example, if the proxy is

{ items: { item1: { color: 'blue' } } }

then { item1: { color: 'blue' } } is observed by 2 event handlers and { color: 'blue' } by 3 event handlers.

Each call to bindProxyAndYMap also creates a new WeakMap cache (pv2yvCache), so even if one event handler has handled the update, the others don't have knowledge of it.

I don't have more time to debug this right now, but wanted to share this braindump in case it's of any help.

nc commented 3 years ago

Here's a GIST for a test case for this: https://gist.github.com/nc/f1ab3957350434f7387671b0b4b9ae5e

nc commented 3 years ago

I've added a call to deepEqual in subscribe as a workaround/hack for now so we can keep using the library and work on other features in the interim. https://github.com/dai-shi/valtio-yjs/pull/15

dai-shi commented 3 years ago

great work, both of you!