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

Connected children components might have mapStateToProps not called #2150

Closed
1 task done
vuminhkh opened this issue Mar 30, 2024 · 18 comments · Fixed by #2156
Closed
1 task done

Connected children components might have mapStateToProps not called #2150

vuminhkh opened this issue Mar 30, 2024 · 18 comments · Fixed by #2156

Comments

@vuminhkh
Copy link

What version of React, ReactDOM/React Native, Redux, and React Redux are you using?

  • React: 18.2.0
  • ReactDOM/React Native: 0.73.5
  • Redux: 5.0.1
  • React Redux: 9.1.0

What is the current behavior?

In my react native app, I have connected parent components which might contain connected children components (react navigation top material tabs). The child components might dispatch action to change its state, but sometimes the state of the child components is not refreshed, mapStateToProps is not called at all. It's not an issue about mutable state because if I change to another tab and return back, the state is correctly refreshed and subsequent state change after this is correctly refreshed. I moved back from 9.1.0 to 8.1.3, and the problem is solved.

What is the expected behavior?

mapStateToProps should be called whenever store state change.

Which browser and OS are affected by this issue?

react native ios

Did this work in previous versions of React Redux?

  • Yes
@markerikson
Copy link
Contributor

Nothing about connect's implementation changed in 9.0.

At a minimum, we'd need a full repro project that demonstrates this happening to investigate.

@vuminhkh
Copy link
Author

Thanks for your response, my project is pretty complex, I'll try to have a simple reproduction scenario once I have time, for now it works with 8.1.3. It's the weirdest issue that I've ever had with redux until now, just change the current tab to another tab and return back, and the connected children components receive store updates again.

@markerikson
Copy link
Contributor

Out of curiosity, is there a reason why you're still using connect and not useSelector?

Also, does this issue happen if you try using useSelector instead?

@vuminhkh
Copy link
Author

I use connect because I like to have dumb UI components, with useSelector, it's even worse, because when I change tab and return it does refresh the state the first time, but then when websocket fires up state changes, the children are not refreshed.

@c-gasiciel
Copy link

c-gasiciel commented Apr 3, 2024

I'm seeing similar issues in an enterprise app I'm working on (can't share code for legal reasons). A navigation component isn't receiving updated props via mapStateToProps even though our store has been accurately updated. I haven't been able to investigate further impact, since this breaks our navigation, and I'm stuck on our initial screen. Using redux v.5.0.1 and react-redux v.9.1.0. If I revert to react-redux v.8.0.5, everything works as it should. There's a lot of legacy code to upgrade, so unfortunately moving from connect to useSelector as a condition of upgrading is not ideal.

@markerikson
Copy link
Contributor

markerikson commented Apr 3, 2024

If someone can provide an example repo , CodeSandbox, Expo Snack, or Replay ( https://replay.io/record-bugs ) that shows this happening, I can try to take a look. But I am genuinely surprised that there would be any differences in behavior between v8 and v9 here.

@c-gasiciel
Copy link

I'll see if I can put something together replicating the issue this weekend. It's tough because our codebase is very large and complex; I'm not sure how easy it will be to recreate the problem since I can't share any of our actual code or debug sessions.

I did try refactoring our navigation component from a class to a function and using useSelector to see if that changed anything, but it didn't. React-redux doesn't appear to recognize changes in our store even though data in the store itself has updated. Everything works as expected when I revert to react-redux v8.0.5.

@markerikson
Copy link
Contributor

The fact that there's two separate reports makes me agree that something is going on here, but yeah, without an actual repro there's nothing I can do to investigate this.

The only potentially relevant change I can think of is that we switched from using the "shim" version of useSyncExternalStore to the built-in version, and there shouldn't be any difference in behavior there.

sdg9 added a commit to sdg9/react-redux-issue-on-rn that referenced this issue Apr 6, 2024
@sdg9
Copy link

sdg9 commented Apr 6, 2024

I have the issue reproduced on a brand new react-native app showing components not re-rendering when state they have in mapStateToProps changes

Repo: https://github.com/sdg9/react-redux-issue-on-rn

If you reproduce and swap to 8.x (yarn add react-redux@8.1.3), all components re-render as expected.

I couldn't reproduce on web (https://codepen.io/steveng9/pen/QWPmLao)

The issue seems to stem from having two nested components each with a mapStateToProps. From version 9.x of react-redux onward, at least within the react-native framework, nested components fail to re-render following state changes indicated by their mapStateToProps unless the parent's mapStateToProps is a superset of what the child is looking for.

One quirk I thought I'd mention, in the demo if you update the date first only 2 of the examples re-render (component w/ no parent and component w/ parent who maps to date as well).
If you then update the counter, 3 examples re-render (component w/ no parent and 2 components with parents mapping to counter).
What I don't understand is if you then update date (after counter is updated), 3 examples re-render (including the component that is a child of a parent who only maps to counter and NOT to date).

@markerikson
Copy link
Contributor

Gotcha, thanks for the repro. I'll be honest and say I'm not sure when I'll have time to look at this - pretty busy atm and have a trip coming up soon. But having a repro will hopefully make it possible to look into this.

@sdg9
Copy link

sdg9 commented Apr 6, 2024

Doing a git bisect 247a41e works as expected.

9f55732, the commit going from babel -> tsup, introducing the issue. To to test this commit I had to comment out import { unstable_batchedUpdates } from "react-dom"; and it's references in node_modules\react-redux\dist\react-redux.mjs

I tested with the project I shared previously and the steps listed here https://stackoverflow.com/a/60389669/1759504, I'm not sure if it's most efficient but I was having issues with yarn link in react native and yalc worked well enough letting me build specific react-redux commits and test those locally.

I'd do the following

  1. in my react-redux cloned dir
    1. checkout desired commit sha
    2. yarn install
    3. yalc publish
  2. In react-native repo
    1. yalc add react-redux
    2. yarn install
    3. yarn start –reset-cache
      1. I don't think cache reset was required but did both that and killed/restarted app on device when testing

@markerikson
Copy link
Contributor

Hmm. Yeah, that suggests it has to do with batching somehow. Lovely.

Latest RN, so latest React, so it should have auto batching.

Huh. Off the top of my head I'm stumped.

@sdg9
Copy link

sdg9 commented Apr 7, 2024

This is my first time diving into the internals of react-redux but doesn't this commit suggest it's it's more about transpiling differences between babel and tsup, at least with how they are configured/implemented in this repo, than batching?

Aren't the batching changes in this commit simply to satisfy typescript? I see a variable name change but no functional change unless I'm missing something.

Example

To illustrate this, I added sdg9@0da0e6b on top of the problematic commit. Now a build/publish generates the lib (babel) directory again, in addition to dist (tsup).

Using my previous comment's yalc steps to add a local package I now have both babel & tsup outputs I can swap between, generated from the same underlying source code.
image

I still have to comment out the react-dom reference in the tsup output, but otherwise in node_modules\react-redux\package.json I just confirm main is pointing to dist to test tsup (stopping & yarn start --reset-cache the metro server in between)

  "main": "dist/cjs/index.js",  // tsup

or lib to test babel

  "main": "lib/index.js",  // babel

The babel version behaves as expected, the tsup version introduces the issue.

@aryaemami59
Copy link
Contributor

@sdg9 Is this an issue only in React Native?

@markerikson
Copy link
Contributor

@sdg9 unfortunately it's much more complicated than that :(

In all versions of React-Redux up through v8, we published the package as separate files transpiled via Babel:

In v9, we switched to pre-bundled artifacts:

Unfortunately, that change did break things with RN, as I found out on the day we shipped.

We've relied on React's built-in batching behavior for some of our subscription handling, but the unstable_batchedUpdates method is exported from the reconcilers like ReactDOM and React Native, not the core react package. In order to make this work prior to React 18, we had separate internal files that import unstable_batchedUpdates from the right package, and the RN version had a .native.js extension. That way the RN bundler would pick up that file during the app build process.

When I switched us to pre-bundle the artifacts, there weren't separate files, and there was no longer a separate file with a .native.js extension for the RN bundler to find. So, if you look at the 9.0.0 release, it always imported from react-dom. I frantically tried to fix this in next couple patch releases, before eventually realizing that React 18 always batches by default anyway and React-Redux 9 requires React 18, so we could actually drop this whole split and simplify things.

So, the bundling aspect does matter.

Thinking about it, yes, I would expect that commit did actually break things, because I know it broke things already - that's why I had to do the follow-on patch releases.

The real question is whether any of the patch release versions still work okay, and then why the current version doesn't work.

Unfortunately I don't expect to have time to look into this until at least early May - I'm about to head off on a conference trip and am trying to get ready for that.

@aryaemami59
Copy link
Contributor

@markerikson I'll look into it as soon as I get a chance. I already tested the other patch versions of v9, none of them seem to work. The problem does seem to be the batch function. I am in the process of moving at the moment but I will definitely take a closer look sometime within the next few days.

@sdg9
Copy link

sdg9 commented Apr 7, 2024

@sdg9 Is this an issue only in React Native?

@aryaemami59 Correct, I'm able to reproduce in React Native but not web when having nested components each with their own mapStateToProps.

@aryaemami59
Copy link
Contributor

@sdg9 looks like I found the culprit, I will put up a PR with the fix.

aryaemami59 added a commit to aryaemami59/react-redux that referenced this issue Apr 10, 2024
  - 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).
fairsky0201 added a commit to fairsky0201/react-redux that referenced this issue Apr 17, 2024
  - React Native was not using `useLayoutEffect` which was causing nested component updates to not get properly batched when using the `connect` API [#2150](reduxjs/react-redux#2150).
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 a pull request may close this issue.

5 participants