joebobmiles / zustand-middleware-yjs

Zustand middleware that enables sharing of state between clients via Yjs.
MIT License
103 stars 10 forks source link

Can't work with Class Instance #37

Closed arvinxx closed 1 year ago

arvinxx commented 1 year ago

Here is the demo.

when click the plus button ,It fails to render

https://codesandbox.io/s/zustand-yjs-middleware-with-class-r7nbgr?file=/src/App.tsx

When work with function, it works well. https://codesandbox.io/s/zustand-yjs-middleware-just-fn-6ru7yu?file=/src/App.tsx

joebobmiles commented 1 year ago

I'm gonna level with you. This:

class Person {
  hello() {
    console.log("hello");
  }
  name: string = "person";

  icon = () => {
    return <div>{this.name}</div>;
  } 

  set setName(name) {
    this.name = name;
  }
}

Is a bad idea and in no way idiomatic React.

However, the issue at hand isn't so much that this class is a bad idea but rather that when an instance of this class goes through the middleware, icon gets turned into an object. Obviously, React can't render an object, which is what you try to do later with <person.icon />. I'm trying to wrack my brain around how this transformation happens. As written, the middleware should be leaving any functions out of the shared type, but still in the store used by the client.

I'm gonna need some time to sort out what is going on here, but in the meantime, you should definitely rethink storing functional components in classes.

joebobmiles commented 1 year ago

OK, time for an update as I better understand what is happening. The source of this bug is that the middleware is not handling state updates correctly. The update triggered by increment causes the shared type to drop both increment and person, but keep count. Because increment is a function, it is ignored and not deleted, but person is. Normally this would make React complain about being given undefined rather than object, but apparently, we're not done. Another update propagates that wants to add person back into the store. However, this is not the same person instance from before, but one that was butchered such that icon is no longer a proper function but an empty object.

The remaining questions before I figure out how to fix this are:

  1. Where does the second update come from? My best guess is that it is a side effect of either Zustand or Yjs trying to keep the other updated. It is more than likely coming from Yjs.
  2. Why is the member function being downgraded from a function to a plain object? Again, my best guess is that person made it into the Yjs store, which demotes functions to objects, and then sent out an update to Zustand telling it that it's missing something.
joebobmiles commented 1 year ago

Another update and a breakthrough.

Update: it turns out that Yjs is updating Zustand twice after calling increment instead of only once. Not sure why this was occurring, and it would probably take some more time rummaging through a debugger and Yjs source to figure it out. But this leads to the breakthrough.

Breakthrough: The double update had me re-read the Yjs documentation for map.observeDeep, which we use to propagate changes that happen in Yjs to Zustand. It turns out that every change you make to a Yjs shared type triggers an observe event. So in this example, we made two changes to Yjs based on the Zustand store (still unsure how), which triggered two updates from Yjs to Zustand. The second update is loaded with corrupt data (a function demoted to an object) and breaks the example app.

To rectify this, it turns out that the Yjs library has a function for grouping changes into single transactions. Wrapping the patchSharedType calls we make whenever the Zustand store changes with a transact eliminates the multiple updates, preventing bad information from leaking into the Zustand store.

I'm going to spend some time writing a few tests to replicate this so that we can catch any regression in the future and the patch should be out shortly.

arvinxx commented 1 year ago

Thanks for your replying! It's very clear you dig out the key reason why the class case breakes. Waiting for a fix~😆

Besides,I agree that person.icon isn't a good practice,but really needed in our case by now.

joebobmiles commented 1 year ago

As I work on this more, I'm finding more and more issues with the idea behind person.icon. Why do you want to use this Person object? Is the intent to have the Person instance available to all connected peers? For instance, are you looking to create an array of Person objects that provides the user with information about who is connected? Or is this Person object to store information about the local user?

arvinxx commented 1 year ago

Conside the situation. There is a house with some person living in. House has living room , bedroom , kitchen... and every person can settle things into these different space. And how to settle controlled by the person's state. and room will know person's state .

in React it may like

<House>
  <person.LivingRoom />
  <person.bedroom />
  <person.kitchen />
</House>

and person is a class to the implement.

class Person {
   constructor(LivingRoom,Bedroom,kitchen,createStore,ZustandProvider){
      const store = createStore();
      this.state = store;
      const Provider = ({children})=><ZustandProvider createStore()=> store> {children}</ZustandProvider>
      this.LivingRoom = (props) => <Provider><LivingRoom /></Provider>
      this.Bedroom = (props) => <Provider><Bedroom /></Provider>
      this.Kitchen = (props) => <Provider><Kitchen /></Provider>
  }
}

and actually I don't want to sync the Person class ,just for the person state,(because Person is same for every peer).

But fornow it seems the middleware will handle all state in zustand store.

joebobmiles commented 1 year ago

:tada: This issue has been resolved in version 1.2.7-rc.2 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

joebobmiles commented 1 year ago

:tada: This issue has been resolved in version 1.2.8-rc.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

joebobmiles commented 1 year ago

:tada: This issue has been resolved in version 1.2.8 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

joebobmiles commented 1 year ago

Tested and verified working with your CodeSandbox example.

I'll leave this with a final note for you to consider (if you haven't already): this middleware (and Yjs) does not allow functions to be transported between peers. This is simply because JS doesn't allow functions to be serialized and unserialized for security reasons (code injection).

It's unclear to me if you expect those member functions to be shared between peers, but if you are, I regret to inform you that it is impossible.