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

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

Closed
Tirzono opened this issue Aug 2, 2022 · 12 comments
Closed

Comments

@Tirzono
Copy link
Contributor

Tirzono commented Aug 2, 2022

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
Copy link
Member

dai-shi commented Aug 2, 2022

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
Copy link
Contributor Author

Tirzono commented Jan 18, 2023

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
Copy link
Member

I remember upgrading R3F to v4 in pmndrs/react-three-fiber#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
Copy link
Member

dai-shi commented Jan 18, 2023

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
Copy link
Member

CodyJasonBennett commented Jan 18, 2023

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 facebook/react#12779). The following creates two no-op reconcilers based on pmndrs/react-nil to emulate react-dom and R3F:

@CodyJasonBennett
Copy link
Member

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

@Tirzono
Copy link
Contributor Author

Tirzono commented Jan 19, 2023

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
Copy link
Member

CodyJasonBennett commented Jan 19, 2023

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
Copy link
Contributor Author

Tirzono commented Jan 24, 2023

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
Copy link
Member

dai-shi commented Apr 27, 2023

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

@dai-shi dai-shi closed this as completed Apr 27, 2023
@CodyJasonBennett
Copy link
Member

CodyJasonBennett commented Apr 27, 2023

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
Copy link
Member

dai-shi commented Apr 27, 2023

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants