Connected children components might have mapStateToProps not called about react-redux HOT 18 CLOSED

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

from react-redux.

@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:

• https://unpkg.com/browse/[email protected]/es/

In v9, we switched to pre-bundled artifacts:

• https://unpkg.com/browse/[email protected]/dist/

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.

from react-redux.

@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.

from react-redux.

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.

from react-redux.

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.

from react-redux.

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?

from react-redux.

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.

from react-redux.

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.

from react-redux.

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.

from react-redux.

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.

from react-redux.

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.

from react-redux.

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 [email protected]), 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).

from react-redux.

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.

from react-redux.

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

from react-redux.

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.

from react-redux.

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.

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.

from react-redux.

@sdg9 Is this an issue only in React Native?

from react-redux.

@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.

from react-redux.