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

y-indexeddb #26

Closed
PhilGarb opened this issue Mar 22, 2022 · 25 comments
Closed

y-indexeddb #26

PhilGarb opened this issue Mar 22, 2022 · 25 comments
Labels
bug Something isn't working

Comments

@PhilGarb
Copy link

Is y-indexeddb supported by valtio-yjs?

@dai-shi
Copy link
Member

dai-shi commented Mar 22, 2022

I have never tried, but if y-indexeddb provides two-way binding to ydoc, valtio-yjs would just work without knowing about y-indexeddb, at least for some simple cases. Please give it a try.

@PhilGarb
Copy link
Author

Thank you for the quick reply. You are probably correct that y-indexeddb is not special and should just work. I do have problems getting it to work however. I adjusted the codesandbox from the messages array example and replaced the websocket provider with the indexeddb one. https://codesandbox.io/s/valtio-yjs-array-demo-forked-38flhk?file=/src/App.js

When you write messages and send them they are indeed reloaded when you refresh. But if you add messages afterwards they are not restored after refreshing.

Peek.2022-03-23.11-55.mp4

I am at a loss about why this would happen. Could it have something to do with the order of binding and the updates coming from the indexed db?

@dai-shi
Copy link
Member

dai-shi commented Mar 23, 2022

Hmm, it can be a bug. You might need to dig deeper.
One thing you can try quickly is to use the message object example and see if the problem persists. This will reveal if it's the array specific issue.

@PhilGarb
Copy link
Author

Good idea. I will do that tomorrow.

@PhilGarb
Copy link
Author

Ok I am pretty sure this is a bug with the array type specifically. I tried the following:

  1. Use the map instead of the array as the top level type as suggested. This works.
    https://codesandbox.io/s/valtio-yjs-demo-forked-kf3twn?file=/src/App.js
    https://user-images.githubusercontent.com/38015558/159975345-79434a5a-0022-4ad5-8eb1-dcce8d0e6cdc.mp4

  2. Then I wanted to know whether it is just the top level array that introduces issues. So I created a sandbox with a map and a messages key that holds the messages array. This has the same issue.
    https://codesandbox.io/s/valtio-yjs-demo-forked-9yxf7e?file=/src/App.js
    https://user-images.githubusercontent.com/38015558/159976214-9cf6fa1a-e111-446b-be67-2bb2a3cdfd6f.mp4

@dai-shi
Copy link
Member

dai-shi commented Mar 25, 2022

Thanks. Seems like a bug.
Can you try reproducing it without y-indexeddb? (Maybe it happens when data exists in ydoc before binding??)
And, then we need to create a minimal failing test.

@dai-shi dai-shi added the bug Something isn't working label Mar 25, 2022
@PhilGarb
Copy link
Author

I am not sure about how to reproduce it without y-indexeddb. I do not really have any persistence apart from that so the state would always be empty on refresh. The issue does not exist in the code sandbox examples in the Valtio-yjs readme which are using the websocket provider.

PhilGarb added a commit to open-decision/open-decision that referenced this issue Mar 25, 2022
…ecycle

The builder url includes the tree id. This id is used to identify which websocket connection to use. By using react context we can
make sure that the YDoc and websocket connection only ever exists when the user is on the respective builder page.

- y-indexeddb is not used, because of a possible bug in valtio-yjs valtiojs/valtio-yjs#26
@dai-shi
Copy link
Member

dai-shi commented Mar 25, 2022

Hm, the bug doesn't seem to be related with indexeddb.
We need to understand the behavior/implementation of y-indexeddb. Maybe, something different from y-websocket.

#22 and #24 will require some refactors, and having a failing test would be very important. (And, some contributors would be helpful.)

@PhilGarb
Copy link
Author

Sadly the documentation for y-indexeddb alone is not that detailed about the inner workings. I will try to get some help from the y-indexeddb maintainers.

I would love to help with the other issues you mentioned. Although I might need some time to familiarise myself with the codebase. I will try to make progress on this in the next two weeks.

PhilGarb added a commit to open-decision/open-decision that referenced this issue Mar 31, 2022
…ecycle

The builder url includes the tree id. This id is used to identify which websocket connection to use. By using react context we can
make sure that the YDoc and websocket connection only ever exists when the user is on the respective builder page.

- y-indexeddb is not used, because of a possible bug in valtio-yjs valtiojs/valtio-yjs#26
@dmonad
Copy link

dmonad commented May 29, 2022

Hi all,

as mentioned before, valtio is not special and will just sync to y-indexeddb.

However, y-indexeddb is not supposed to sync tabs. It needs some kind of provider that syncs tabs. Usually, this is done using cross-tab communication using broadcastchannels.

y-websocket and y-webrtc both support broadcastchannel communication. It makes more sense to implement cross-tab communication in the network providers, because they have more knowledge about the network. In y-webrtc, for example, two clients will not open a webrtc connection to each other and only use broadcastchannel.

At the moment there is no pure broadcast channel provider for Yjs. However, it might make sense to write a generic broadcastchannel provider that has an interface for network providers.

@dai-shi
Copy link
Member

dai-shi commented May 29, 2022

So, it's not a bug of valtio-yjs per se, right?

@dmonad
Copy link

dmonad commented May 30, 2022

Nope

@dai-shi dai-shi removed the bug Something isn't working label May 30, 2022
@PhilGarb
Copy link
Author

Thank you @dmonad for your answer. Do I understand you correctly that by reloading the codesandbox preview it works like opening another tab? Otherwise I am not sure what tabs have to do with this issue.

@dmonad
Copy link

dmonad commented May 30, 2022

Oh wait. This issue is not about cross-tab communication at all. I have no idea how the bug in the video can happen.

There is a lot of stuff going on in the codesandbox. If you can reproduce this issue without valtio, let me know.

I think the bug might happen because you manipulate the document in the same transaction that loads the initial y-document (e.g. during an observer call).

@PhilGarb
Copy link
Author

PhilGarb commented Jun 1, 2022

Ok if I understand you correctly the issue is that it causes two transactions simultaneously?
I have adjusted the codesandbox with a timeout so this does not happen and that does indeed fix the issue. https://codesandbox.io/s/valtio-yjs-array-demo-forked-38flhk

This is of course not a real solution and I am wondering if this is a more general bug with how valtio-yjs is supposed to work. Theoretically the binding always happens in the same transaction. The only reason I can see why this does not happen with something like y-websocket is the network delay introducing a natural timeout that results in two transactions.

@dai-shi
Copy link
Member

dai-shi commented Jun 1, 2022

So, it's related with #22?
Let's put bug label again.

@dai-shi dai-shi added the bug Something isn't working label Jun 1, 2022
@PhilGarb
Copy link
Author

PhilGarb commented Jun 1, 2022

I am not sure it is related. While it would be good if only one transaction where caused I don't think it matters for this issue.

My current thoughts/questions on why this could happen:

  1. Why does valtio-yjs cause a transaction at all on initialization when state is restored from y-indexeddb? Shouldn't a transaction only be caused as a result of a direct proxy update?
  2. There are multiple deleteSet items on each of the transactions. Could it be that instead of restoring valtio-yjs somehow replaces everything? I did have a similar issue in another situation where issues arose when an initial value was provided to valtio-yjs.
  3. However why is the issue then fixed by adding a timeout? Could there be a closure problem where both y-indexeddb and valtio-yjs replay there state on the same doc leading to the drop of the new values?
  4. Lastly this still only happens with arrays. The place of the transaction is probably based upon the index of the item in the array. Could this lead to the drop of items?

@dmonad
Copy link

dmonad commented Jun 15, 2022

Why does valtio-yjs cause a transaction at all on initialization when state is restored from y-indexeddb? Shouldn't a transaction only be caused as a result of a direct proxy update?

A transaction happens whenever the Yjs document is modified (also when content loads from a database, or from the network). You can distinguish transaction by their transaction-origin.

There are multiple deleteSet items on each of the transactions. Could it be that instead of restoring valtio-yjs somehow replaces everything? I did have a similar issue in another situation where issues arose when an initial value was provided to valtio-yjs.

This behavior commonly happens if you prefer the content that you receive from some other source (e.g. the "initial state" from the server replaces the current version of the document). I recommend to make Yjs the source of truth.

However why is the issue then fixed by adding a timeout? Could there be a closure problem where both y-indexeddb and valtio-yjs replay there state on the same doc leading to the drop of the new values?

Applying an update (e.g from y-indexeddb) throws an event. If you modify the document within the event-handler, the changes will happen within the same execution context. However, y-indexeddb will ignore all changes of the current execution context as it thinks that y-indexeddb created all the changes within the current transaction call.

This line actually is responsible for ignoring updates.

This can be solved by filtering transactions by origin (which was not a thing when I originally created y-indexeddb).

I created an issue / feature request for that: yjs/y-indexeddb#19

I still recommend not to manipulate the document within event-handlers as other providers might not handle this correctly as well.

Lastly this still only happens with arrays. The place of the transaction is probably based upon the index of the item in the array. Could this lead to the drop of items?

This problem is probably just more visible with Y.Arrays. Other data types will have similar issues.

@PhilGarb
Copy link
Author

Just so I understand correctly. Are we talking about event handlers in the codesandbox or in valtio-yes source code?

@dmonad
Copy link

dmonad commented Jun 16, 2022

Event handlers in the valtio-yjs source code - you shouldn't modify Yjs in event handlers like observe and update.

Anyway, I just "fixed" the behavior in y-indexeddb v9.0.8. I still recommend that you don't modify Yjs in event handlers, because other providers might still not work.

@ShravanSunder
Copy link

@dai-shi @PhilGarb I was wondering if this still the case where valtio-yjs is modifying yjs in event handlers?

Event handlers in the valtio-yjs source code - you shouldn't modify Yjs in event handlers like observe and update.

Anyway, I just "fixed" the behavior in y-indexeddb v9.0.8. I still recommend that you don't modify Yjs in event handlers, because other providers might still not work.

@dai-shi
Copy link
Member

dai-shi commented Oct 3, 2023

Yeah, I don't think we have changed anything since then.

@ShravanSunder
Copy link

@dmonad @dai-shi Would the idiomatic way to do this would be to push the updates to a queue or stack and do them outside of the subscribe block? or whats the preferred methodology to do this?

@raineorshine
Copy link
Contributor

I am no longer able to reproduce this issue with the original codesandbox. All messages persist across multiple refreshes. Anybody else?

@PhilGarb
Copy link
Author

PhilGarb commented Dec 2, 2023

It has worked for me for quite a while. I am not sure the issue has ever been caused by valtio-yjs. So I am gonna close this.

@PhilGarb PhilGarb closed this as completed Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants