Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initialization causes data to be deleted #45

Open
PhilGarb opened this issue Dec 2, 2023 · 7 comments
Open

Initialization causes data to be deleted #45

PhilGarb opened this issue Dec 2, 2023 · 7 comments
Labels
help wanted Extra attention is needed

Comments

@PhilGarb
Copy link

PhilGarb commented Dec 2, 2023

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
Copy link
Contributor

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 dai-shi added the help wanted Extra attention is needed label Dec 2, 2023
@dai-shi
Copy link
Member

dai-shi commented Dec 2, 2023

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
Copy link
Contributor

raineorshine commented Dec 2, 2023

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
Copy link
Author

PhilGarb commented Dec 3, 2023

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
Copy link

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
Copy link

MH4GF commented May 13, 2024

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
Copy link
Member

dai-shi commented May 13, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants