-
-
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 useIsomorphicLayoutEffect
usage in React Native environments
#2156
Fix useIsomorphicLayoutEffect
usage in React Native environments
#2156
Conversation
- React Native was not using `useLayoutEffect` which was causing nested component updates to not get properly batched when using the `connect` API [reduxjs#2150](reduxjs#2150).
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
sigh of course it was the Yeah, we bundle now, so that |
@markerikson hopefully this'll be the end of it :), just wondering do you think I should also switch the |
- Since we bundle the library now, and React Native explicitly uses `useLayoutEffect`, we do not need this file anymore.
@sdg9 Thanks for the repro, it made debugging very easy! |
I'm rephrasing #2150 (comment) with the fix of this PR in my own words as I want to make sure I understand what is going on.
Is this an accurate understanding? Just curious, diving more into # 2, it seems like the impetus of having a conditional driving const isServer = /* some logic to reliably check if Node.js server, if such a thing exists */
export const useIsomorphicLayoutEffect = isServer ? useEffect : useLayoutEffect I ask because had it been written this way this issue would never have ocurred when the I can understand if this is easier said than done, for example I imagine it's easier/more consistent to check if we're running in a DOM vs react-native environment than to check if it's Node.js/server but I'm not certain, hence the question. |
Yep, that's a good summary. (And ironically, that warning is going away in React 19.) Yeah, tweaking the conditional that way would make sense, I think. |
@markerikson Do you want me to include further tweaks in this PR or separate? |
What about a two pronged approach? Move forward with this PR as-is since we know it resolves the issue. This will unblock RN consumers like myself immediately and return behavior back to how things were in v8.x for RN |
Okay, out in https://github.com/reduxjs/react-redux/releases/tag/v9.1.1 ! |
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [react-redux](https://togithub.com/reduxjs/react-redux) | [`^9.1.0` -> `^9.1.1`](https://renovatebot.com/diffs/npm/react-redux/9.1.0/9.1.1) | [![age](https://developer.mend.io/api/mc/badges/age/npm/react-redux/9.1.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/react-redux/9.1.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/react-redux/9.1.0/9.1.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/react-redux/9.1.0/9.1.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>reduxjs/react-redux (react-redux)</summary> ### [`v9.1.1`](https://togithub.com/reduxjs/react-redux/releases/tag/v9.1.1) [Compare Source](https://togithub.com/reduxjs/react-redux/compare/v9.1.0...v9.1.1) This bugfix release fixes an issue with `connect` and React Native caused by changes to our bundling setup in v9. Nested `connect` calls should work correctly now. #### What's Changed - Remove unused isProcessingDispatch by [@​Connormiha](https://togithub.com/Connormiha) in [https://github.com/reduxjs/react-redux/pull/2122](https://togithub.com/reduxjs/react-redux/pull/2122) - Move `Equals` constraint into an intersection type. by [@​DanielRosenwasser](https://togithub.com/DanielRosenwasser) in [https://github.com/reduxjs/react-redux/pull/2123](https://togithub.com/reduxjs/react-redux/pull/2123) - Fix `useIsomorphicLayoutEffect` usage in React Native environments by [@​aryaemami59](https://togithub.com/aryaemami59) in [https://github.com/reduxjs/react-redux/pull/2156](https://togithub.com/reduxjs/react-redux/pull/2156) **Full Changelog**: reduxjs/react-redux@v9.1.0...v9.1.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 5pm,every weekend" in timezone America/Los_Angeles, Automerge - "after 5pm,every weekend" in timezone America/Los_Angeles. 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/valora-inc/wallet). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zMDEuNCIsInVwZGF0ZWRJblZlciI6IjM3LjMwMS40IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJucG0iLCJyZW5vdmF0ZSJdfQ==--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [react-redux](https://togithub.com/reduxjs/react-redux) | [`^9.1.0` -> `^9.1.1`](https://renovatebot.com/diffs/npm/react-redux/9.1.0/9.1.1) | [![age](https://developer.mend.io/api/mc/badges/age/npm/react-redux/9.1.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/react-redux/9.1.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/react-redux/9.1.0/9.1.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/react-redux/9.1.0/9.1.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>reduxjs/react-redux (react-redux)</summary> ### [`v9.1.1`](https://togithub.com/reduxjs/react-redux/releases/tag/v9.1.1) [Compare Source](https://togithub.com/reduxjs/react-redux/compare/v9.1.0...v9.1.1) This bugfix release fixes an issue with `connect` and React Native caused by changes to our bundling setup in v9. Nested `connect` calls should work correctly now. #### What's Changed - Remove unused isProcessingDispatch by [@​Connormiha](https://togithub.com/Connormiha) in [https://github.com/reduxjs/react-redux/pull/2122](https://togithub.com/reduxjs/react-redux/pull/2122) - Move `Equals` constraint into an intersection type. by [@​DanielRosenwasser](https://togithub.com/DanielRosenwasser) in [https://github.com/reduxjs/react-redux/pull/2123](https://togithub.com/reduxjs/react-redux/pull/2123) - Fix `useIsomorphicLayoutEffect` usage in React Native environments by [@​aryaemami59](https://togithub.com/aryaemami59) in [https://github.com/reduxjs/react-redux/pull/2156](https://togithub.com/reduxjs/react-redux/pull/2156) **Full Changelog**: reduxjs/react-redux@v9.1.0...v9.1.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 5pm,every weekend" in timezone America/Los_Angeles, Automerge - "after 5pm,every weekend" in timezone America/Los_Angeles. 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/valora-inc/wallet). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zMDEuNCIsInVwZGF0ZWRJblZlciI6IjM3LjMwMS40IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJucG0iLCJyZW5vdmF0ZSJdfQ==--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [react-redux](https://togithub.com/reduxjs/react-redux) | [`^9.1.0` -> `^9.1.1`](https://renovatebot.com/diffs/npm/react-redux/9.1.0/9.1.1) | [![age](https://developer.mend.io/api/mc/badges/age/npm/react-redux/9.1.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/react-redux/9.1.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/react-redux/9.1.0/9.1.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/react-redux/9.1.0/9.1.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>reduxjs/react-redux (react-redux)</summary> ### [`v9.1.1`](https://togithub.com/reduxjs/react-redux/releases/tag/v9.1.1) [Compare Source](https://togithub.com/reduxjs/react-redux/compare/v9.1.0...v9.1.1) This bugfix release fixes an issue with `connect` and React Native caused by changes to our bundling setup in v9. Nested `connect` calls should work correctly now. #### What's Changed - Remove unused isProcessingDispatch by [@​Connormiha](https://togithub.com/Connormiha) in [https://github.com/reduxjs/react-redux/pull/2122](https://togithub.com/reduxjs/react-redux/pull/2122) - Move `Equals` constraint into an intersection type. by [@​DanielRosenwasser](https://togithub.com/DanielRosenwasser) in [https://github.com/reduxjs/react-redux/pull/2123](https://togithub.com/reduxjs/react-redux/pull/2123) - Fix `useIsomorphicLayoutEffect` usage in React Native environments by [@​aryaemami59](https://togithub.com/aryaemami59) in [https://github.com/reduxjs/react-redux/pull/2156](https://togithub.com/reduxjs/react-redux/pull/2156) **Full Changelog**: reduxjs/react-redux@v9.1.0...v9.1.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 5pm,every weekend" in timezone America/Los_Angeles, Automerge - "after 5pm,every weekend" in timezone America/Los_Angeles. 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/valora-inc/wallet). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zMDEuNCIsInVwZGF0ZWRJblZlciI6IjM3LjMwMS40IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJucG0iLCJyZW5vdmF0ZSJdfQ==--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Overview
It looks like React Native was not using
useLayoutEffect
which was causing nested component updates to not get properly batched when using theconnect
API.This PR:
useIsomorphicLayoutEffect
usage in React Native environments.Possible related issues: #1313, #1444, #1510, #1436.
I did test the actual build with
yalc
, React Native, Expo and Expo web seem to be working properly. We can probably get rid ofsrc/utils/useIsomorphicLayoutEffect.native.ts
file, so let me know if you want me to do that I can include it in this PR or a separate one.