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

Yjs Middleware Does Not Work with Immer Middleware #53

Open
tiagoefreitas opened this issue Jul 2, 2023 · 8 comments
Open

Yjs Middleware Does Not Work with Immer Middleware #53

tiagoefreitas opened this issue Jul 2, 2023 · 8 comments
Assignees
Labels
bug Something isn't working

Comments

@tiagoefreitas
Copy link

tiagoefreitas commented Jul 2, 2023

I am trying to use this to sync a react flow store but when yjs syncs react doesn't refresh the page from zustand contents.

I tried to add immer but still not working

here is my store:

import { WebsocketProvider } from 'y-websocket'
import * as Y from "yjs";
import { immer } from 'zustand/middleware/immer';
import { useStore } from 'zustand';
import yjs from "zustand-middleware-yjs";
import { createStore } from 'zustand/vanilla';

// Create a Y Doc to place our store in.
const ydoc = new Y.Doc();
const docName = "xxx";

if (typeof window != "undefined") {
  const provider = new WebsocketProvider('wss://y-websocket-server.xxx.co:5900',docName, ydoc)

  provider.on('synced', () => {
      console.log('content from the database is loaded')
  })
}

const store = createStore(
  immer(
    yjs(
      ydoc, 
      docName, 
      (set) => 
      ({
        nodes: [],
        edges: [],
        addNode: (node: any) => {
          set((state: any) => {
            state.nodes.push(node)
          })
        },
        removeNode: (id: any) => {
          set((state: any) => {
            state.nodes = state.nodes.filter((node: any) => node.id !== id)
          })
        },
        addEdge: (edge: any) => {
          set((state: any) => {
            state.edges.push(edge)
          })
        },
        removeEdge: (id: any) => {
          set((state: any) => {
            state.edges = state.edges.filter((edge: any) => edge.id !== id)
          })
        }
      })
    )
  )
);

export const usePanelStore = (selector: any) => useStore(store, selector)
@tiagoefreitas tiagoefreitas added the bug Something isn't working label Jul 2, 2023
@joebobmiles
Copy link
Owner

joebobmiles commented Jul 2, 2023

I notice in your example that you are using the Zustand vanilla createStore function in your code snippet.

The vanilla createStore function does not invoke any React hooks and thus will not trigger a re-render.

Are you using the vanilla createStore in your project as well?

@tiagoefreitas
Copy link
Author

tiagoefreitas commented Jul 2, 2023

Yes that's exactly what I have for the store. What should I use instead?

And is that the correct way of using immer?

I added immer because I want to be able to change node fields directly (like node.field=x) and re-render automatically.

I am understanding correctly?

Thanks!

@joebobmiles
Copy link
Owner

To get React to update whenever the Zustand store changes, you should just use import { createStore } from 'zustand'. The vanilla import is for using Zustand outside of React.

That should fix your problem. Middlewares work the same regardless of which version of createStore you use, so you don't need to change anything about how you use Immer or Yjs.

@tiagoefreitas
Copy link
Author

tiagoefreitas commented Jul 3, 2023

If I use that code without vanilla and without immer, react still doesn't refresh until I reload the tab. But that may be something I did wrong in react flow.

If I use immer I get this error calling the addNode, on the client that syncs (I don't get the error on the client that calls it initially):

client.js:1 Caught error while handling a Yjs update TypeError: Cannot add property 4, object is not extensible
    at Array.splice (<anonymous>)
    at eval (yjs.mjs:376:1)
    at Array.reduce (<anonymous>)
    at applyChangesToArray (yjs.mjs:371:1)
...

Even if I use my old immutable methods I get the same error.

const store = createStore(
    immer(yjs(
        ydoc, 
        docName, 
        (set) => 
        ({
            nodes: [],
            edges: [],
            addNode: (node: any) => {
                set((state: any) => ({
                    ...state,
                    nodes: [...state.nodes, node]
                }))
            },
            removeNode: (id: any) => {
                set((state: any) => ({
                    ...state,
                    nodes: state.nodes.filter((node: any) => node.id !== id)
                }))
            },
            addEdge: (edge: any) => {
                set((state: any) => ({
                    ...state,
                    edges: [...state.edges, edge]
                }))
            },
            removeEdge: (id: any) => {
                set((state: any) => ({
                    ...state,
                    edges: state.edges.filter((edge: any) => edge.id !== id)
                }))
            }
        })
    ))
);

@joebobmiles
Copy link
Owner

joebobmiles commented Jul 3, 2023

If I use immer I get this error calling the addNode, on the client that syncs (I don't get the error on the client that calls it initially):

client.js:1 Caught error while handling a Yjs update TypeError: Cannot add property 4, object is not extensible
    at Array.splice (<anonymous>)
    at eval (yjs.mjs:376:1)
    at Array.reduce (<anonymous>)
    at applyChangesToArray (yjs.mjs:371:1)
...

OK that is definitely an error in the Yjs middleware. applyChangesToArray is what is called to transform an array in the client's old state to the new state received from a peer.

Looking at the code in question in the Yjs middleware, it received an Immer immutable array (created on store initialization) and then tried to call Array.splice on it when the peer sent an update. Based on my assumptions about what Immer does under the hood, it makes sense that the Yjs middleware would get an error about an object not being extensible.

I'm going to need to do some digging to figure out what exactly the sequence of events are when the Yjs and Immer middlewares are composed to together. In the meantime, try switching the order of the Yjs and Immer middlewares in your code, i.e.:

const store = createStore(yjs(immer(...)));

(This should not change the result, as middleware should be commutative (order does not matter).)

@seanryy
Copy link

seanryy commented Sep 14, 2023

Hey @joebobmiles, running into the same issue. Did you ever get around to looking into this?

@joebobmiles
Copy link
Owner

I... Yes? Do you not see the linked PR? The two linked commits?

That out of the way, I did forget about this issue. Looking at the errors from the failed tests on the PR, I am reminded that this bug will be a pain to resolve.

Immer expects that a producer (the code that mutates an immutable value) either returns the new value for Immer to swap or mutates a reference to that value which Immer provides to the producer. The Yjs middleware violates this because of the two-way binding it creates between Zustand and the Yjs store.

What happens is that when a value is modified locally via Zustand, that change is sent to Yjs in a transaction. Yjs evaluates the transaction, but because the Yjs store changed, it sends an 'echo' of the change it received back to Zustand. If unchecked, this would cause an infinite loop, but in this case, Yjs is using a version of Zustand's set that hasn't been augmented by the middleware, avoiding Zustand sending a duplicate change.

But this results in a problem: Zustand updated its state twice during a single set operation, once in a "legal" way and once in an "illegal" way, as far as the Immer middleware is concerned. But the only way around this is to, more or less, break the two-way binding between Zustand and Yjs, which breaks the core functionality of the Yjs middleware.

So I have to re-evaluate and rewrite how the two-way binding works just to appease Immer. Which is a pain.

@seanryy
Copy link

seanryy commented Sep 14, 2023

I did see the PR and the tests but more around if you had been able to get any further since then 😄 That was perhaps lost in translation of my British-ness, sorry!

I appreciate your hard work - that does sound like a pain. I think what I'll do then is just quit using Immer.

Thanks for the extra explainer!

@joebobmiles joebobmiles changed the title React not refreshing on sync Yjs Middleware Does Not Work with Immer Middleware Sep 25, 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

3 participants