reduxjs / react-redux

Official React bindings for Redux
https://react-redux.js.org
MIT License
23.4k stars 3.36k forks source link

useSelector() in combination with lazy loaded components breaks with react v18 #1977

Closed sag-tobias-frey closed 1 year ago

sag-tobias-frey commented 2 years ago

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

What is the current behavior?

  1. Open the codesandbox ( https://codesandbox.io/s/elegant-hofstadter-i2h34h?file=/src/features/counter/InnerCounter.js )
  2. Open the page in a new browser window
  3. Click the "INCREMENT ME!" text
  4. If the number in the first row increments -> Full reload the page, repeat 3.
  5. The number does not increase although the store is updated (see console.log output) but the selector does not trigger

Additional note: This works correctly with react v17 (ReactDOM.render) and breaks in v18 (createRoot). I have also debugged the behavior and have seen that the Provider component is unmounted which removes the store subscriptions of the useSelector hooks. After the Provider mounts again, the subscriptions are not restored because the child components do not render again.

What is the expected behavior?

The number is increased on every click.

Which browser and OS are affected by this issue?

all

Did this work in previous versions of React Redux?

markerikson commented 2 years ago

Hmm. I honestly don't think I understand what this sandbox is trying to show, or what the repro steps are ("full reload"?). Also, why are there multiple separate <Provider>s in the components? Why are there multiple calls to initStore()? and why is there a <Suspense> around the entire component tree? This seems like a very odd setup, to the point of being actively wrong.

FWIW in v8 we're ultimately just using React's useSyncExternalStore API, and no longer managing subscriptions ourselves.

sag-tobias-frey commented 2 years ago

Thanks for looking at this bug report. Let me try to give you some more information on it.

Background

The code is actually from a pretty big application which I tried to break down as much as possible. Therefore, there might be some odd looking constructs.

1) The "full reload" (Crtl + F5) is only needed because with the small reproduction it will not work every time and you have to try until it breaks. For our big application it actually breaks every time. 2) The application actually consists of many smaller components and every component uses its own redux store. For simplification, I used the same store layout twice. 3) The <Suspense> around the component tree to catch any lazy loaded comp which are not caught lower in the tree.

Reproduction

I have modified the reproduction to make it clearer. The issue basically is that on clicking the button, the upper value is not updated although the store is updated with the new value which can be seen in the console.log output. An example of a broken run:

image

Subscriptions

I have seen that you are using useSyncExternalStore under the hood. However, the <Provider> component still creates a subscription which is passed down through the context and in the end used as input for the useSyncExternalStore.

https://github.com/reduxjs/react-redux/blob/8d03182d36abe91cb0cc883478f3b0c2d7f9e17f/src/components/Provider.tsx#L34

The problematic code is in the cleanup function of the useEffect where the subscription is unsubscribed.

https://github.com/reduxjs/react-redux/blob/8d03182d36abe91cb0cc883478f3b0c2d7f9e17f/src/components/Provider.tsx#L53

This removes all previous subscriptions (including useSelector subscriptions) from being fired on store changes.

https://github.com/reduxjs/react-redux/blob/8d03182d36abe91cb0cc883478f3b0c2d7f9e17f/src/utils/Subscription.ts#L135

This happens when the <Provider> component is unmounted and then mounted again. Although the subscription subscribes again, the previous subscriptions from the useSelector hooks lower down in the tree are not restored.

React Version

This actually works correctly when using ReactDOM.render and breaks with the new v18 createRoot api which I think both already use useSyncExternalStore.

markerikson commented 2 years ago

Hmm. I sorta see the "outer doesn't update" behavior in the sandbox... but if I clone this project and build+run it locally, both numbers update fine, in both a dev and production build.

I'm still confused over what you're describing overall. Yes, components stop subscribing when they unmount, because they're unmounting. And if a <Provider> unmounts, all the components inside of it will unmount, because that's how React rendering works.

sag-tobias-frey commented 1 year ago

@markerikson I have changed the codesandbox by delaying the lazy loading of the component by 1s. After doing that I can reproduce it every time locally.

I was also really confused about the behavior; however the inner components seems not to be unmounted and are still relying on the outdated subscriptions which are no longer firing. There seems to be some behavior change between react v17 & v18 because using the ReactDOM.render api makes the issue disappear completely.

rolintoucour commented 1 year ago

I am not able to give some reproducible stuff, however if this may help: we have a project with react-redux and react-routeur. Routes are declared lazily (React.lazy(() => import(...)). We have a top menu that is inserted in the page before the routing outlet (but within the provider) and that uses a useSelector to manage its state.

After the migration to React 18 AND using the new createRoot method, the top menu doesn't update anymore. The selector callback isn't executed anymore (sorry I am a newbie and I don't have the exact term, maybe this is some mounting / unmounting).

Now if I move the top menu into the final components (that are displayed within the routing), or if I remove the lazy loading, then it works great.

francescocaveglia commented 1 year ago

Seems to me that the problem is that the Provider is inside the Suspense, moving it outside fixes the problem


<Provider store={store}>
  <Suspense>
    <Counter />
   </Suspense>
</Provider>`
sag-tobias-frey commented 1 year ago

Yes, however it worked with React 17 or without createRoot without any problems either way.

lennerd commented 1 year ago

@francescocaveglia solution did not work for us as we do not use a single redux store for our whole application but multiple stores for all our more comprehensive, more state driven tools. This leads to quite a few suspense boundaries outside these tools causing the problems described in this ticket.

So AFAICT this problem is because useLayoutEffect is used to subscribe to the store inside the Provider component. useLayoutEffect is calling its unmount callback when a suspense boundary is hiding content and showing a loading spinner instead. Like @sag-tobias-frey is saying: this adds a new subscription to the redux store without resubscribing subscriptions from useSelector hooks.

So … to solve this problem for us we use the mechanic behind useLayoutEffect. We rerender our redux provider and all its children via the key property when ever a useLayoutEffect callback is called. To make sure a suspense inside our redux provider is not remounting the whole app, we use a suspense boundary inside the store provider (see below).

const [store] = useState(() => configureStore({ /* ... */ }));
const [renderKey, setRenderKey] = useState(1);

useLayoutEffect(() => {
  setRenderKey((key) => key + 1);
}, []);

return (
  <Provider store={store} key={renderKey}>
    <Suspense>{children}</Suspense>
  </Provider>
);

Fixes the problem for us for now. Hope this helps others. Looking forward to more input from @markerikson and the team. Maybe we just missing something here?

markerikson commented 1 year ago

Hmm. I know there's some nuances in the internal timing and calls to cleaning up useLayoutEffect, useEffect, and useSyncExternalStore in these cases.

Maybe useSyncExternalStore would help here...