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

React 18 - production mode only: 2 * useSyncExternalStore + useState = Explosion #23250

Closed
henrify opened this issue Feb 8, 2022 · 9 comments
Labels
React 18 Bug reports, questions, and general feedback about React 18 Resolution: Needs More Information Type: Discussion

Comments

@henrify
Copy link

henrify commented Feb 8, 2022

I tried to migrate an app from React 18 beta and useMutableSource to RC0 and useSyncExternalStore. Everything worked fine, except:

  • If a component has more than one useSyncExternalStore hooks
  • And if the component has useState hook
  • And if the useState's setter is invoked
  • And if the mode is production
  • Then React explodes 100% repeatably

The exact 'explosion' seems to depend on where/how useState setter is invoked and/or what other hooks are used. In some cases useRef starts returning 'undefined' and in some cases useCallback starts throwing internally "typeError: Cannot read property 'length' of undefined".

The above is in our real app - I haven't tried isolated repro. Also in our app, the two useSyncExternalStore hooks are subscribed to different stores, not sure if that is related or relevant.

Maybe you could quickly add the above case (2x useSyncExternalStore + useState) to your existing tests, and if it doesn't reproduce the bug, then I can do a minimal isolated repro?

@henrify henrify added React 18 Bug reports, questions, and general feedback about React 18 Type: Discussion labels Feb 8, 2022
@eps1lon
Copy link
Collaborator

eps1lon commented Feb 8, 2022

Maybe you could quickly add the above case (2x useSyncExternalStore + useState) to your existing tests, and if it doesn't reproduce the bug, then I can do a minimal isolated repro?

It's usually faster to just create a codesandbox to verify. You can deploy a codesandbox to netlify or vercel to test production.

@henrify
Copy link
Author

henrify commented Feb 9, 2022

@eps1lon Okay here is the repro: https://codesandbox.io/s/eloquent-rosalind-f6riy?file=/src/App.js

Unfortunately the bug reproduces 100% -- our actual production application and the codesandbox crash in the exactly same way, in a couple of seconds.

@henrify
Copy link
Author

henrify commented Feb 9, 2022

@eps1lon Here is a second, slightly different repro, which results in a different error: https://codesandbox.io/s/goofy-glade-3xz29?file=/src/App.js

I'm also seeing a third error in our real app (useRef returning undefined) but that one I haven't reproduced yet.

Also, when trying these out, please remember that the problem is production-mode-only.

@gaearon
Copy link
Collaborator

gaearon commented Feb 9, 2022

@acdlite
Copy link
Collaborator

acdlite commented Feb 25, 2022

If it only affects production mode, I highly suspect it's related to this bug, which is fixed in main: #23150

Are you able to reproduce using the latest prerelease? Try updating React and React DOM to 18.0.0-rc.0-next-8c4cd65cf-20220224.

@eps1lon
Copy link
Collaborator

eps1lon commented Feb 25, 2022

codesandbox.io/s/goofy-glade-3xz29?file=/src/App.js

This evenutally crashed with https://reactjs.org/docs/error-decoder.html/?invariant=300: https://csb-2szpyh-hswzi5eco-eps1lon.vercel.app/

Using 18.0.0-rc.0-next-8c4cd65cf-20220224 cause no crash: https://csb-rorqbe-2osjmrk20-eps1lon.vercel.app/

Versions can be verified in React Devtools > Components tab > click on root > "react-dom@...."

@henrify
Copy link
Author

henrify commented Feb 25, 2022

Thanks @acdlite I can confirm that the latest main does not crash our production app.

As a separate issue, I think you should consider the versioning and/or comms -- companies will be testing against the highest numbered RC.X tag (RC.0 has more downloads than all the later versions combined, so our company is clearly not the exception). But in reality, the RC.0 is not even close to the actual release candidate and the community is now largely testing the wrong / old version.

PS @gaearon sorry, I had missed your earlier reply.

@henrify henrify closed this as completed Feb 25, 2022
@gaearon
Copy link
Collaborator

gaearon commented Feb 25, 2022

There’s nothing wrong with testing RC0 per se. We try to batch up changes before cutting the next RC so that there’s a limited number of versions people are actively testing. RC1 will go out soon.

@acdlite
Copy link
Collaborator

acdlite commented Feb 25, 2022

RC.0 is not even close to the actual release candidate and the community is now largely testing the wrong / old version.

I agree we could have done a better job communicating about this specific bugfix but RC.0 is really close to what's in main. The only reason we haven't bumped to RC.1 yet is because we wanted to also include some improvements to error reporting (#23207) in the same version.

Regarding whether the RC is the right label: we were in beta for months without receiving this bug report — we only got it after the bump to RC got us an additional wave of testers. So while I understand it's frustrating that something marked "release candidate" contained a bug, the purpose of an RC is to confirm whether there are any remaining issues so we can fix them before going to stable.

Thank you for reporting the bug and please let us know if you find any additional issues so we can fix them!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
React 18 Bug reports, questions, and general feedback about React 18 Resolution: Needs More Information Type: Discussion
Projects
None yet
Development

No branches or pull requests

4 participants