valtiojs / valtio-yjs

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

Initialization causes data to be deleted #45

Open PhilGarb opened 10 months ago

PhilGarb commented 10 months ago

Problem

When using vanilla valtio the way to initialize it is to add data when creating the proxy. While this is not shown in the valtio-yjs readme it is an easy mistake to make. The problem is that whatever data is added in the proxy is causing an update in the yjs doc every time the bind method is called. This leads to data corruption. The issue is that the normal proxy is also inferring the type from the provided data so just not providing data leads to having no types at all. Currently we are working around this by calling proxy like this:

const store = proxy({} as OurDataType)

While this is a lie the expectation is that the persisted data is coming from the yjs doc.

Solution

Valtio-yjs should abstract the proxy. Currently the adoption seems like a consumer would have both a store and a yjs doc available. However the yjs doc is the way the data is transmitted (through adapters), stored (locally or in db) and conflicts are handled. The store is only there as a consequence of the yjs doc and not as an equivalent. Treating it as such is confusing.

This would be a first idea about how the public interface could look like:

const yDoc = new Y.Doc()
const store = bind<OurDataType>(yDoc)

Opportunity

While for an existing yjs doc the behavior should change like described above there is an opportunity here. Since the bind is inserting whatever it is provided this could be a great way to initialize a new yjs doc. Just providing a javascript type is preferable to the yjs canonical way of creating shared types and inserting them into each other manually. I would split this functionality to make its aim clear. It could look something like this:

const { store, yDoc, yMap } = createBoundStore({ property: "test", another_property: ["string"], .... })
const { store, yDoc, yArray } = createBoundStore(["string", "string2"])

The returned yDoc does now contain the initialData and can be handled like it usually is.

raineorshine commented 10 months ago

Could you post a Codesandbox with the problem? I'm having a hard time understanding the initialization issue and separating that from the architectural changes you are recommending.

Part of what I'm hearing is, why not bind the entire Doc rather than individual shared types, which is a good question. Then it would be more like a YJS provider.

dai-shi commented 10 months ago

I'm not sure about the exact problem, so hope @PhilGarb can help on it.

Valtio-yjs should abstract the proxy.

I originally thought it would be neat to "bind" existing instances at any levels, but the implementation became complicated. So, if this approach makes it simpler, I'm in favor of it.

why not bind the entire Doc rather than individual shared types

Yeah, I took it so.

raineorshine commented 10 months ago

Another reason to sync the whole Y.Doc: YJS syncs all shared types of a Doc en masse. There's no way to sync some and not others. So they're all going to be in memory anyway.

PhilGarb commented 10 months ago

I could not reproduce the issue outside our codebase this morning. I will have a deeper look into it, but it looks like this is not a bug in valtio-yjs. I think if this issue does not exist the only part that would still be valid here is the deliberate initialisation. This is already possible by creating a yDoc and binding a proxy to it, but could be exposed as a nice utility.

himself65 commented 10 months ago

In yjs design, data shouldn't have defaultValue(except the initialization) otherwise it will overwrite the upcoming update.

I think my approach is to load the initial data before using valtio

MH4GF commented 4 months ago

I had the same issue. It is a tricky problem that happens completely randomly, and I am trying to prepare a small reproduction code, but have not been able to do so. (You can see what's left of it here) As @himself65 said, if I wait for the first websocket response and then bind, it starts to stabilize. (But I still haven't found the cause...)

let unbind = () => {}
doc.once('afterAllTransactions', () => {
  const yMap = doc.getMap("room")
  unbind = bind(store, yMap)
})

If I find a way to reproduce the minimum, or if I find useful information, I will share it.

dai-shi commented 4 months ago

FYI, the current situation is that we know the issue which is caused by the fundamental design choice and I agree with the proposed solution. We need a contributor who understand this library well and implement the solution.