pmndrs / zustand

🐻 Bear necessities for state management in React
https://zustand-demo.pmnd.rs/
MIT License
47.36k stars 1.46k forks source link

Zustand v4 gives error in combination with react-three-fiber (works with v3) #1164

Closed Tirzono closed 1 year ago

Tirzono commented 2 years ago

I have a hard time giving this issue a proper title, so apologies for that.

I have the following codesandbox prepared which did work correctly with v3 and doesn't work correctly with v4 anymore:

https://codesandbox.io/s/zustand-r3f-bug-97x50l

image

Steps to reproduce the issue:

  1. Press "2" on your keyboard to add two boxes
  2. Press "1" on your keyboard to remove one box

Note that this issue does not happen when you follow the following steps to add/remove boxes:

  1. Click on "Add box" twice
  2. Click on a random "Remove box"

When downgrading to v3, the codesandbox works correctly. This issue only appears in the situation where we have react-three-fiber in the play, which gives me an idea it could be to do with the react-reconciler and the order of rendering. But I am not expert in this.

dai-shi commented 2 years ago

Thanks for reporting. What I found is if we change it to legacy ReactDOM.render, the error is reproducible with buttons. https://codesandbox.io/s/zustand-r3f-bug-forked-t7gcli?file=/src/index.js

The change between v3 and v4 is basically use-sync-external-store, which might not work with r3f? @drcmda

Tirzono commented 1 year ago

I've updated the packages (zustand and r3f) to the latest versions in the codesandbox, hoping that some changes in both packages would resolve the issue (especially because some changes are done in r3f with its-done). Unfortunately they don't. @drcmda or @CodyJasonBennett, do you maybe have an idea what could cause this issue or how to work around it?

CodyJasonBennett commented 1 year ago

I remember upgrading R3F to v4 in https://github.com/pmndrs/react-three-fiber/pull/2558 without issue, I'll have to take a closer look to study what's going on here. I'm assuming you're still able to reproduce this despite Zustand 4.3.2?

dai-shi commented 1 year ago

fwiw, it works with use-zustand, which doesn't use use-sync-external-store. https://codesandbox.io/s/zustand-r3f-bug-forked-56myu5?file=/src/App.js

CodyJasonBennett commented 1 year ago

This only happens when accessing state from another renderer and passing it down. It doesn't happen when using react-dom or R3F on their own via their respective createRoot APIs nor when reading useStore from the same renderer (which is a good work-around if your app permits it).

As an aside, I can reproduce this via React 18.x and R3F 8.x, but not React 17.x and R3F 7.x (react-dom version matching react ofc). I have not tested with Zustand v3.

React only supports two renderers at once -- a primary and secondary renderer (see https://github.com/facebook/react/pull/12779). The following creates two no-op reconcilers based on pmndrs/react-nil to emulate react-dom and R3F:

CodyJasonBennett commented 1 year ago

Related, might need a bump of use-sync-external-store eventually https://twitter.com/tannerlinsley/status/1615813228831068160.

Tirzono commented 1 year ago

Thanks for making me aware of use-zustand. I will use that one so I can at least update Zustand and stay up-to-date.

So as far as I understand this is not something that we can address in Zustand (or r3f), but has to do with the inner working of use-sync-external-store?

As an aside, I can reproduce this via React 18.x and R3F 8.x, but not React 17.x and R3F 7.x (react-dom version matching react ofc). I have not tested with Zustand v3.

In case it's worth to know: I am currently running Zustand v3 with React 18.x and R3F 8.x in my application and that's not causing an issue.

CodyJasonBennett commented 1 year ago

So as far as I understand this is not something that we can address in Zustand (or r3f), but has to do with the inner working of use-sync-external-store?

I'm afraid it's a React bug or limitation with concurrent renderers and useSyncExternalStore. I'd like to further isolate Zustand's usage here and make an issue in React, but it may not be actionable (we've seen similar issues in the past with effect timings, among other issues where renderers don't get along).

I can only point you to work-arounds in the meantime, we may be able to employ some here.

Tirzono commented 1 year ago

fwiw, it works with use-zustand, which doesn't use use-sync-external-store. https://codesandbox.io/s/zustand-r3f-bug-forked-56myu5?file=/src/App.js

I've now got it working now with use-zustand as a workaround:

import { createStore } from 'zustand/vanilla';
import { useZustand } from 'use-zustand';

const create = (createState) => {
    const api = createStore(createState);
    const useBoundStore = (selector, equalityFn) => useZustand(api, selector, equalityFn);
    Object.assign(useBoundStore, api);
    return useBoundStore;
};
dai-shi commented 1 year ago

closing as the fundamental issue is the combination of uSES and r3f.

CodyJasonBennett commented 1 year ago

Is there any reason why https://github.com/dai-shi/use-zustand is its own package rather than a part of Zustand? I haven't been following React 18 discussion around state/concurrency, but this affects all concurrent renderers, not just R3F. I don't think it came across that this is a defect of useSyncExternalStore and React, R3F so happens to use react-reconciler but it's not the only one -- react-native etc. The demo above is my minimal recreation if this should be moved to React.

dai-shi commented 1 year ago

Yes, if you can reproduce the issue with uSES and r3f without zustand, please file an issue there.

I'm not sure if RN is the same. It's never reported, I guess?