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

Fix useIsomorphicLayoutEffect usage in React Native environments #2156

Merged

Conversation

aryaemami59
Copy link
Contributor

Overview

It looks like React Native was not using useLayoutEffect which was causing nested component updates to not get properly batched when using the connect API.

This PR:

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 of src/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.

  - 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).
Copy link

codesandbox-ci bot commented Apr 10, 2024

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.

@markerikson
Copy link
Contributor

markerikson commented Apr 10, 2024

sigh of course it was the useLayoutEffect issue, again.

Yeah, we bundle now, so that .native file is useless.

@aryaemami59
Copy link
Contributor Author

@markerikson hopefully this'll be the end of it :), just wondering do you think I should also switch the target for the main entry point from es2020 to esnext?

  - Since we bundle the library now, and React Native explicitly uses `useLayoutEffect`, we do not need this file anymore.
@aryaemami59
Copy link
Contributor Author

@sdg9 Thanks for the repro, it made debugging very easy!

@sdg9
Copy link

sdg9 commented Apr 10, 2024

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.

  1. Before react-redux v9 you used a .native.js suffix to differentiate behavior for ReactNative. In ReactNative useIsomorphicLayoutEffect was explicitly set to useLayoutEffect, while the other file had DOM vs server logic.
  2. With the react-redux upgrade to v9, the npm artifact was pre-bundled to a single js file. This change eliminated the use of the native.js suffix, which inadvertently caused React Native to be handled the same way as server environments (using useEffect) as react-native is not DOM based.
  3. With this PR ReactNative once again uses useLayoutEffect.

Is this an accurate understanding?

Just curious, diving more into # 2, it seems like the impetus of having a conditional driving useIsomorphicLayoutEffect is to differentiate server environments vs all others (based on comments in the modified file on how useLayoutEffect triggers warnings on servers).
Would it make more sense to structure the conditional logic to something like the following?

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 native.js file was dropped, React-Native would have defaulted to the client behavior rather than the server behavior.

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.

@markerikson
Copy link
Contributor

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.

@aryaemami59
Copy link
Contributor Author

@markerikson Do you want me to include further tweaks in this PR or separate?

@sdg9
Copy link

sdg9 commented Apr 13, 2024

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
Then if desired discuss long term whether the conditional makes sense to keep as-is or tweak it based on my question which is more of a forward looking potential preventative measure.

@markerikson markerikson merged commit 0396da3 into reduxjs:master Apr 14, 2024
25 checks passed
@markerikson
Copy link
Contributor

Okay, out in https://github.com/reduxjs/react-redux/releases/tag/v9.1.1 !

@aryaemami59 aryaemami59 deleted the fix-RN-useIsomorphicLayoutEffect-issue branch April 15, 2024 18:19
github-merge-queue bot pushed a commit to valora-inc/wallet that referenced this pull request Apr 18, 2024
[![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
[@&#8203;Connormiha](https://togithub.com/Connormiha) in
[reduxjs/react-redux#2122
- Move `Equals` constraint into an intersection type. by
[@&#8203;DanielRosenwasser](https://togithub.com/DanielRosenwasser) in
[reduxjs/react-redux#2123
- Fix `useIsomorphicLayoutEffect` usage in React Native environments by
[@&#8203;aryaemami59](https://togithub.com/aryaemami59) in
[reduxjs/react-redux#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>
github-merge-queue bot pushed a commit to valora-inc/wallet that referenced this pull request Apr 18, 2024
[![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
[@&#8203;Connormiha](https://togithub.com/Connormiha) in
[reduxjs/react-redux#2122
- Move `Equals` constraint into an intersection type. by
[@&#8203;DanielRosenwasser](https://togithub.com/DanielRosenwasser) in
[reduxjs/react-redux#2123
- Fix `useIsomorphicLayoutEffect` usage in React Native environments by
[@&#8203;aryaemami59](https://togithub.com/aryaemami59) in
[reduxjs/react-redux#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>
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.

Connected children components might have mapStateToProps not called
3 participants