joebobmiles / zustand-middleware-yjs

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

Upgrade the zustand version from "^3.5.5" to "4.0.0" #32

Closed hunterZh37 closed 2 years ago

hunterZh37 commented 2 years ago

Hi, I am just wondering if it is possible to upgrade the zustand version from "^3.5.5" to "4.0.0"? The project my team is working on requires zustand 4.0.0, and it would be great if we can have this library use zustand 4.0.0 too!

Just to provide more context to the error I am getting: I am trying to use multiple zustand middlewares, zustand-middleware-yjs being one of them, in the code state below; however, I am getting an error shown in the picture. I am suspecting that the error I am getting has to do with the difference in zustand versions between the middlewares, but I am not too sure. All the other middlewares are using zustand 4.0.0

const useStore = create((yjs(doc,"shared",subscribeWithSelector(computed(immer(store),computedSlice))))); Screen Shot 2022-04-27 at 10 49 52 AM

joebobmiles commented 2 years ago

Hmm, I'm not well versed with NPM version syntax, but I assumed leaving the ^ would have let you install the latest version of zustand. Have you checked your package-lock.json or node_modules to confirm that you have multiple versions of zustand installed?

That error message makes me also wonder if you've uncovered a bug with state patching. Currently, when patching, we have this check:

        else if (value.__old !== undefined && value.__new !== undefined)

And that error seems to suggest that value is actually null. Can you give me a minimum working example of this bug?

joebobmiles commented 2 years ago

Well, baring this actually being related to multiple versions of zustand in your project, I did find a problematic code path that could lead to value being null when patching an object.

Notes for myself (and anyone interested):

When patching a plain-data object (PDO), we do the following:

  1. Diff the new PDO (state received from YJS) with the old PDO (local state).
  2. Compute a list of changes from the diff that will alter the old state into the new state.
  3. Use the change list to change the old state into the new state.

The problem I've noticed happens in step 2. When we prepare a change list for a PDO (as opposed to an array), we first augment the diff to contain all the unchanged, undeleted values from the old state. (No idea why I did it this way, as it's been well over a year and that bit of code is quite esoteric in hindsight.)

When we find a parameter that hasn't changed or been deleted, we simply insert that property into the diff. Everything is OK here, since later on we will simply just add a "no op" change to the change list when we see that a property has gone unchanged in the diff:

changes.push([ "none", property, value ]);

But! Before we get to this line, we have to go through a bunch of checks:

        if (property.match(/__added$/))
          changes.push([ "add", property.replace(/__added$/, ""), value ]);

        else if (property.match(/__deleted$/))
          changes.push([ "delete", property.replace(/__deleted$/, ""), undefined ]);

        else if (value.__old !== undefined && value.__new !== undefined)
          changes.push([ "update", property, value.__new ]);

        else if (value instanceof Object)
          changes.push([ "pending", property, undefined ]);

        else
          changes.push([ "none", property, value ]); // insert the "no op" change

And we can see the problem. Before we get to the "no op" line, we perform two checks on the value of a property: first, we check if it has __old and __new properties (indicating that the value changed between the new and old states) and second, we check if the value is an object (which indicates that we need to recurse deeper into the old state to transform it). It's this first check where I think this issue is happening - if value is null (a totally valid, if annoying, value), then we can't access __old or __new, and everything crashes down on top of our heads.

TL;DR I trusted a user provided value to never be null/undefined. Oops.

hunterZh37 commented 2 years ago

Thank you so much for your response, it seems like you have found where the bug is. Do you still want me to give you a minimum working example of this bug?

joebobmiles commented 2 years ago

No, that won't be necessary. I've implemented some tests that reproduce this error. I'll let you know when a new version is available.

joebobmiles commented 2 years ago

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

The release is available on:

Your semantic-release bot :package::rocket: