cerebral/overmind

[BUG] Incompatible with React 18 with Strict Mode

crimson-med opened this issue · 13 comments

crimson-med commented

Per discussion in the discord we discovered that the latest versions is incompatible with React 18.X Strict Mode

Here is a repo showcasing the example:

https://github.com/crimson-med/state-issue/tree/overmind

Quote from DavidTheUnwise from discord

the double effect call in strict dev mode causes the tree to dispose (and therefore stop reacting to changes) even though the component is still mounted. i tried to look for inspiration from valtio on how it solved the problem, given that it works on a very similar, proxy-based mechanism, and i feel like the real fix is to do something similar with the new useSyncExternalStore hook.
rather naively, i just commented out that dispose call to observe the effects, and it appears to work fine, the dispose still eventually gets called when the component unmounts on line 169
i suspect that this causes degradation in performance, if multiple force-renders are added when the state updates (the tree track registration would register a new force render listener every time the component renders)
however, i'm not sure how common this would be in production build. from my understanding the double effect call can happen in production, but it's not forced like in dev

Link to the call: https://github.com/cerebral/overmind/blob/next/packages/overmind-react/src/index.ts#L161

grandevel commented

Discussed and we have a session with @christianalfoni on Monday to research and implement a plan

  • 🎉3
MaxSpiro commented

Thank you. I was having this problem as well

sergeyzwezdin commented

Hm, seems it's related to #567
@christianalfoni @grandevel @MaxSpiro Did you guys have a chance to look into this?

latobibor commented

I'm a bit confused on this ticket: is overmindjs not compatible with React 18 or this is only a problem for strict version of React 18. We are updating our react-native app and we used and ❤️ loved overmindjs and we want to keep the lib. (I hate the idea of going back to some pseudo FP clunky lib like zustand).

christianalfoni commented

Hi there 😄

React 18 support was shipped with the latest version, though I was not personally able to refactor any existing app to React 18, so could not personally verify. But it is a "either it works or does not" kind of thing 👍 😄

  • 👍1
latobibor commented

Thanks a lot for the clarification @christianalfoni!

larrifax commented

I'm experiencing stale UI values in parts of my application that's running in StrictMode, so it looks like the React 18 support isn't 100% fault free @christianalfoni. The same values are up-to-date when removing StrictMode, so it looks to be a "strict mode only" bug.

Environment:

  "overmind": "28.0.2",
  "overmind-react": "29.0.2",
  "react": "18.2.0",
christianalfoni commented

Hi @larrifax 😄

Thanks for reporting, I created a PR which I believe fixes the issue: #586

The issue is indeed related to StrictMode and Reacts "double render checks" to detect bad side effects. Overmind has a "controlled bad side effect" to more efficiently clean up memory usage, but this breaks in development and the previous "development fix" seems to be too fragile.

I'll let you know when it is merged to @next and I hope you are able to test to verify that it works as I have not been able to reproduce the original issue with simple examples 😄

christianalfoni commented

@larrifax If you are eager to test you can copy the OvermindReact code from this Sandbox and create your hooks from that file instead 😄 : https://codesandbox.io/p/sandbox/little-hooks-3p998f?file=%2Fsrc%2FOvermindReact.ts

christianalfoni commented

Okay, so with help from @henri-hulski we have a pretty smooth deploy process now and the PR is already merged and available on @next. So install all packages of overmind using the @next tag, like npm install overmind@next, but also for overmind-react etc. 😄

  • ❤️1
christianalfoni commented

I will take the devtool and target it for an upgrade to React 18 to properly verify these changes, but it was quicker to do a theoretical change now 😄

larrifax commented

Thanks for the swift response!
I've upgraded my project to the latest @next tag, and it seems to have fixed the bug I was encountering. I'll keep using it until you cut a new release. Will report back if I notice any oddities with it. 👍

henri-hulski commented

Should be solved in #586.
Please reopen if there's still a problem.

  • ❤️1