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

Use useIsomorphicLayoutEffect in Provider for consistency #1683

Merged

Conversation

speakingcode
Copy link
Contributor

Provider should use useIsomorphicLayoutEffect instead of directly calling useEffect, for consistency with useSelector

@netlify
Copy link

netlify bot commented Jan 26, 2021

Deploy preview for react-redux-docs ready!

Built with commit d5d4b0a

https://deploy-preview-1683--react-redux-docs.netlify.app

@timdorr
Copy link
Member

timdorr commented Jan 26, 2021

This isn't a layout effect, so we don't need to elevate its priority to run before DOM manipulation.

@timdorr timdorr closed this Jan 26, 2021
@markerikson
Copy link
Contributor

Actually, this is something I wanted us to look at changing.

@timdorr timdorr reopened this Jan 26, 2021
@timdorr
Copy link
Member

timdorr commented Jan 26, 2021

Okie dokie, we can look at it.

@markerikson
Copy link
Contributor

markerikson commented Jan 26, 2021

Bit of background for context.

Per https://stackoverflow.com/a/65660253/62937 , I recently debugged someone's project that was using react-blessed (a React renderer for CLI output), where the UI stopped updating after the first dispatch.

The real problem was that react-blessed was using an old version of react-reconciler that happened to have a bug and caused effect cleanups to run too early. However, I also noted that:

-Provider is still calling useEffect instead of useIsomorphicLayoutEffect, which is indeed generally inconsistent with our other components

  • There's no way to force use of useLayoutEffect when running under Node in cases where you might be using an alternate renderer instead of ReactDOM.

So, I was tossing around the idea of tweaking Provider to call useIsomorphicLayoutEffect, and also expose a util function from alternate-renderers to let users force that that point to useLayoutEffect in case that matters to them.

@speakingcode
Copy link
Contributor Author

speakingcode commented Jan 26, 2021

Indeed @markerikson, that was my project you helped with. Took me a while to get around to this. I'm working on the API to force useLayoutEffect at the moment and will try to submit a PR today

@markerikson markerikson merged commit 3aa8993 into reduxjs:master Mar 22, 2021
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

Successfully merging this pull request may close these issues.

None yet

3 participants