-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Fix RN batching and effect behavior #1444
Conversation
Deploy preview for react-redux-docs ready! Built with commit 1f93e2e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix looks good to us, we can work out a babel plugin to apply native.js for our renderer. Appreciate the fix
@MingruiZhang : what renderer are you using? |
Okay, let's go with this. |
I just published https://www.npmjs.com/package/react-layout-effect I'll gladly add you as a maintainer if you want to use it in |
* Add React Native deps for testing * Add a Jest config to run tests in an RN environment * Add initial RN tests * Add test for RN batch export * Add jest-native testing assertions * Extract useIsomorphicLayoutEffect utility * Add additional RN batching-related tests * 7.1.2-alpha.0
Fixes #1313 and fixes #1437 (hopefully!).
Per facebook/react#14927 ,
useLayoutEffect()
warns in SSR. We tried to feature-detect that by checking ifwindow
exists, and if so, falling back touseEffect
:react-redux/src/components/connectAdvanced.js
Lines 40 to 41 in dbfcc97
In #1283 , that was found to be insufficient if someone polyfilled
window
, so the check was made more explicit:react-redux/src/components/connectAdvanced.js
Lines 40 to 45 in dcf2cb0
We then duplicated that over into
useSelector
as well.Unfortunately, it looks like that feature detection breaks in React Native, and we've been falling back to
useEffect
the whole time. This has led to subtle timing bugs popping up, and we (I) ignored the issue reports largely because we couldn't reproduce them in a web environment. In addition, while several folks said that something had broken withconnect
between 7.0.3 and 7.1.0, I only looked at the one tiny change inside ofconnect
itself, not at theuseIsomorphicLayouteffect
change, and didn't catch that all the reports were in RN projects.This PR hopefully fixes all those, by:
useIsomorphicLayoutEffect.native.js
file that always usesuseLayoutEffect
with RNuseIsomorphicLayoutEffect === useLayoutEffect
in an RN environmentuseSelector
#1437 as a test case to verify this works right