-
-
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
Connected children components might have mapStateToProps not called #2150
Comments
Nothing about At a minimum, we'd need a full repro project that demonstrates this happening to investigate. |
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. |
Out of curiosity, is there a reason why you're still using Also, does this issue happen if you try using |
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. |
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. |
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. |
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. |
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 |
I have the issue reproduced on a brand new react-native app showing components not re-rendering when state they have in Repo: https://github.com/sdg9/react-redux-issue-on-rn If you reproduce and swap to 8.x ( 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 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). |
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. |
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 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 I'd do the following
|
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. |
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. ExampleTo illustrate this, I added sdg9@0da0e6b on top of the problematic commit. Now a build/publish generates the Using my previous comment's I still have to comment out the "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. |
@sdg9 Is this an issue only in React Native? |
@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 When I switched us to pre-bundle the artifacts, there weren't separate files, and there was no longer a separate file with a 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. |
@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 |
@aryaemami59 Correct, I'm able to reproduce in React Native but not web when having nested components each with their own mapStateToProps. |
@sdg9 looks like I found the culprit, I will put up a PR with the fix. |
- 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).
- 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).
- Added a unit test for [React-redux reduxjs#2150](reduxjs/react-redux#2150)
- Added a unit test for [React-redux reduxjs#2150](reduxjs/react-redux#2150)
- 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).
What version of React, ReactDOM/React Native, Redux, and React Redux are you using?
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?
The text was updated successfully, but these errors were encountered: