<Provider> misses state changes that occur between when its constructor runs and when it mounts

[rgrove]

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

If state changes occur during the time after <Provider>'s constructor runs and before <Provider> is mounted, <Provider> will miss those changes and won't make them available to connected components during the render phase of those components.

This is probably a rare use case, but it can occur in a very specific scenario that's useful in an app that uses both server-side and client rendering and needs to allow dynamically loaded components (such as components loaded when the route changes) to attach new reducers to an existing store.

Here's a reduced test case that fails in React Redux 6.0: https://codesandbox.io/s/612k3pv1yz

What is the expected behavior?

Connected components should always see the most recent Redux state, even if that state changed between when <Provider> was constructed and when it was mounted. This was the behavior in 5.x.

Here's an exact copy of the reduced test case above, but using React Redux 5.1.1 to demonstrate that this works fine there: https://codesandbox.io/s/yvw1vmnkrv

Which versions of Redux, and which browser and OS are affected by this issue? Did this work in previous versions of Redux?

Redux 4.0.1. This worked in React Redux 5.x, but broke in 6.0. It's unrelated to any specific browser or OS.

More background on this use case

I realize this use case may be a little hard to understand at first glance, so I'll try to explain in more detail why it's valuable.

The repro case above simulates a technique that's useful when using Redux and React Redux in an SSR-compatible app that needs to be able to load components and attach new reducers on demand (such as when the route changes) without knowing up front what components or reducers might eventually be loaded.

This technique is used extensively by Cake. I believe New Twitter uses a similar approach, but they're still on React Redux 4.x so aren't yet affected by this bug.

When rendering on the server, we can't load components during the render phase (because the SSR render phase is synchronous). So instead we need to load all components for a given render pass up front, then attach reducers as needed during the render phase of the app.

Dynamically loaded components may themselves import other components, and components at any level of the import tree could be Redux-connected. This means each component must be able to attach its own reducers. The withRedux decorator in the repro case simplifies this by wrapping a dynamically loaded component (such as the Route component in the example) in a higher order component that attaches reducers on demand in its constructor.

Since React constructs <Provider> before it constructs its children, this means that the Redux store <Provider> sees at construction time doesn't yet have a complete set of reducers attached.

Once the child components are constructed, all reducers will have been attached and React will begin to render the component tree, but <Provider> in React Redux 6.0 passes the old state to all its context consumers during the render phase, breaking any component that expects the new state. <Provider> doesn't check for state changes until componentDidMount() runs, at which point it's already too late.

Edit: s/Redux/React Redux/ in various places because I'm tired. I do know the difference, I promise!

Edit 2: Clarified that dynamically loaded reducers are attached during the render phase, not before it.

[markerikson]

I'll look at this over the next couple days, but I see one key sentence that I think is the heart of the issue:

Connected components should always see the most recent Redux state, even if that state changed between when was constructed and when it was mounted.

In v5, each connected component subscribed directly to the store, and called store.getState() directly. That meant that if a component higher in the tree dispatched an action while it was being constructed, all components constructed after that would see the updated state immediately.

However, that behavior is actually not desirable for the long-term. The React team (Andrew specifically) uses the word "tearing" to refer to cases where different parts of a component tree see different state values during the same render pass. While that's primarily a concern in the future concurrent React world, this loophole could also be classified as "tearing".

For v6, we've switched to putting the entire store state into new context, which means React guarantees that the entire component tree will see the same state value during a given render pass. So, even if parent components dispatch actions during construction, their children will still be constructed using the original state value.

Now, the v6 <Provider> does specifically try to update itself at the end of the mounting process if the store state changed during that time:

    // Actions might have been dispatched between render and mount - handle those
    const postMountStoreState = store.getState()
    if (postMountStoreState !== this.state.storeState) {
      this.setState({ storeState: postMountStoreState })
}

That technique came directly from @bvaughn's create-subscription module.

I know that someone else was complaining about dynamic reducer-type behavior in #935 . There may be something we can do to help with that scenario. But, the idea of a single consistent state value for a given render is a key aspect of v6's design.

As I said, I wrote all that without having actually dug into your specific issue in depth. I'll try to look at this in more detail over the weekend.

[markerikson]

(side note: I greatly appreciate the detailed issue writeup and the documented CodeSandbox example - thank you for that!)

[rgrove]

@markerikson You bet!

Now, the v6 <Provider> does specifically try to update itself at the end of the mounting process if the store state changed during that time:

Unfortunately it's already too late when <Provider>'s componentDidMount() runs, because child components that depend on up-to-date state have already executed their render phase and have broken because they didn't see the state they expected to see in their mapStateToProps() functions.

I agree that v6's behavior totally makes sense given where React is headed. I'd like to find a way to fix this without needing to go back to v5's approach, but so far I'm stumped. I've spent the last two days trying to think of a way to fix this, either in React Redux or in my own app, and I'm still at a loss, so I'm hoping someone else will have a bright idea.

I could do it if we didn't need SSR, or if async SSR were possible, but so far I haven't been able to come up with a way to support both synchronous SSR and dynamically attached reducers at any level of the component tree with v6.

[gaearon]

I agree that v6's behavior totally makes sense given where React is headed.

To be clear from React's point of view, a library "missing" an update is always a bug. No matter when an update happens, the end result should be consistent.

[markerikson]

It sounds like the key issue here is that these components are expecting to dynamically mount an associated reducer (which appears to be happening in another parent HOC), and immediately be getting the updated state tree before their own mapState runs. So, I wouldn't classify this as "missing an update", exactly. Rather, the issue is that the newly loaded component and its mapState function are seeing the state value from before the update, which obviously didn't have the just-added slice.

In a way, the fact that this ever worked in earlier versions seems like a quirk of the implementation.

Note that I'm not saying that dynamic reducer loading is a bad idea or anything. It's obviously a valuable use case, and I want to find a way to support it.

One potential workaround, for now, would perhaps be to "just" make your mapState functions more resilient. Don't assume that state.dynamicallyLoaded always exists - check to see if it's there first, don't let your mapState logic blow up if it's not there, and have some combo of default props and prop destructuring in your component to be able to deal with that first render.

Although, having said that... SSR generally is just a single render pass, right? That... could complicate things.

[rgrove]

Although, having said that... SSR generally is just a single render pass, right? That... could complicate things.

Yeah, exactly. Even if I did take this approach (which would mean refactoring dozens and dozens of components, but I'd do it if it would help!), it wouldn't solve the problem because we only get one render pass on the server.

[markerikson]

Is your CodeSandbox actually representative of an SSR setup? I've never done any SSR myself. It looked like you were actually doing two renders in that case, but I guess on the server you'd be calling renderToString() instead?

[rgrove]

Is your CodeSandbox actually representative of an SSR setup?

No, the CodeSandbox is somewhat contrived because reproducing a full SSR setup would have made it a lot more complicated and I was trying to keep it simple.

But the approach shown there is very similar (minus the actual server rendering and routing) to the approach we use in our app. And it needs to work the same on both the server and the client in any case, so I think reproducing the issue on the client still makes for a valid repro case.

[markerikson]

Right, I was mostly trying to understand the notion of calling ReactDOM.render() twice, while awaiting for something to load for a route change. If you can render twice, why not once more to get the results of the setState() calls?

Actually, now I'm curious. What does happen with setState() calls in an SSR setup? I genuinely have no idea.

[rgrove]

If you can render twice, why not once more to get the results of the setState() calls?

Sorry for the confusion. The only reason for the second render in the repro case is to simulate a client-side route change that might cause a component to be dynamically loaded. It's not something you would do as shown in an app. Probably just unnecessary complexity.

If you remove the first render, the issue will still occur:

async function main() {
  let Route = await loadRoute();
  renderApp(<Route />);
}

Actually, now I'm curious. What does happen with setState() calls in an SSR setup? I genuinely have no idea.

There's never a valid opportunity to call setState() on the server, because the server only performs a single render phase and never calls componentDidMount(). Technically you could call setState() inside UNSAFE_componentWillMount() on the server and it would work since the state change would be committed before rendering, but I'm assuming relying on deprecated lifecycle functions is out of the question here, so I haven't even considered using that.

[markerikson]

Hmm. Yeah, this does seem tough.

I'll admit that at first glance at the moment, I don't see a way to make this work for SSR with v6 and new context. Fundamentally, that one render pass is only ever going to see whatever is in the initial store state.

On the client, if the components can survive that first mapState call where the expected slice of state doesn't exist, they should get it in the re-render. I just tried playing around with the sandbox to check to see if state.bar exists in Route.mapState, and added a default barCount prop as a fallback. That allows the app to load, and logging props.barCount shows it rendering twice, once with the value from defaultProps, and the second with the value from the store.

Am I correct that the general idea here is to SSR-render a bunch of dynamically loaded components+reducers as part of the initial render? Is it perhaps possible to add those to the store ahead of the renderToString() call?

[rgrove]

On the client, if the components can survive that first mapState call where the expected slice of state doesn't exist, they should get it in the re-render.

Yeah, but if we only cared about client-side rendering, we wouldn't need to use this approach at all. There are much easier ways of dealing with dynamically loaded components if you don't care about SSR. 😄

Am I correct that the general idea here is to SSR-render a bunch of dynamically loaded components+reducers as part of the initial render?

As part of the only render, yep (because SSR only gets one shot). But we have to be able to load and render the exact same components on the server and on the client, which is why we can't use two different approaches.

Is it perhaps possible to add those to the store ahead of the renderToString() call?

Nope. That would require knowing about all the components that have been loaded before we render them. If we were only loading a flat list of connected components, this would be doable. But we're actually loading a tree. The components we load can themselves import components, which can import more components, and so on.

In other words, at the top level we can await import('./Foo') and we know that we just loaded Foo, but we have no way of knowing what components Foo might have imported, or what components those components might have imported. We can attach Foo's reducers before rendering, but we don't know what other components might need to attach reducers.

That's why the components themselves need to be able to attach their own reducers during the render phase; there's not really any other opportunity to do it.

[markerikson]

Really stupid question: could you do something like this?

const dummyElement = jsDomOrSomething.createElement("div");

// dummy render to let the store update itself
ReactDOM.render(
    <Provider store={store}>
        <App />
    </Provider>,
    dummyElement
);

const html = ReactServerOrSomething.renderToString(
    <Provider store={store}>
        <App />
    </Provider>
);

[rgrove]

Hmm. Maybe? But that's one render for the price of two on every server response and every client-side route change, so it'd be a serious performance killer. And since componentDidMount() would run on the DOM render, even on the server, there'd be all kinds of side effects from components attaching DOM event listeners, trying to measure element rects, and things like that. I think that's probably not an option.

I think you're probably right that there's not a good way to make this work in v6 without reintroducing the "tearing" problem you mentioned earlier. But it does seem like tearing is preferable to the alternatives. And v6 does include the store itself in the new context object. Could connect() accept an option that would cause it to get state from store.getState() instead of storeState?

It seems like that could work here:

react-redux/src/components/connectAdvanced.js

Lines 200 to 214 in 595ae8f

	const { storeState, store } = value
	let wrapperProps = this.props
	let forwardedRef
	if (forwardRef) {
	wrapperProps = this.props.wrapperProps
	forwardedRef = this.props.forwardedRef
	}
	let derivedProps = this.selectDerivedProps(
	storeState,
	wrapperProps,
	store
	)

That might make it possible to support this use case on an opt-in basis.

[markerikson]

Yeah, I know it was a bad suggestion, but I had to throw it out anyway :)

Hmm. Interesting idea.

Okay, lemme throw out something of a notional hack-y API suggestion.

Maybe we support something like:

<Provider store={store} UNSAFE_readLatestStoreStateOnFirstRender={true}>

And then in the component:

 let { storeState, store, readStoreStateOnFirstRender } = value 
  
 let wrapperProps = this.props 
 let forwardedRef 
  
 if (forwardRef) { 
   wrapperProps = this.props.wrapperProps 
   forwardedRef = this.props.forwardedRef 
 } 

 if(readStoreStateOnFirstRender  && this.isFirstRender) {
   storeState = store.getState()
 }

 this.isFirstRender = false
  
 let derivedProps = this.selectDerivedProps( 
   storeState, 
   wrapperProps, 
   store 
) 

Thoughts?

[cellog]

how about we make Provider SSR-aware, and then do the stuff needed if it is explicitly told that we are server-rendering? tearing is impossible in SSR, so if we are certain it's safe, this could be done in something like UNSAFE_cwrp or even in gDSFP

[markerikson]

The issue here isn't entirely SSR-specific, per the earlier discussion. There's issues with dynamically loading reducers and components on the client, too, where this option would be useful.

[rgrove]

@markerikson

Maybe we support something like:

<Provider store={store} UNSAFE_readLatestStoreStateOnFirstRender={true}>

Yeah, I think that could work well!

[markerikson]

Okay. I've got some other priorities atm, but if you wanted to submit a PR that implements something like that, we can certainly take a look at it.

[rgrove]

Definitely. I'll work on this tomorrow. Thanks for talking this through with me!

[Ephem]

@rgrove Fantastic writeup of this issue! 👏 Great discussion all around, I thought I'd add a few things.

To me, conceptually this previously worked precisely because of finely controlling the ability to tear so that the tear always happened before any components that relied on it was rendered.

Server rendering is really finicky. For anyone that has not worked on it before, I tend to recommend to think of it in terms of renderToNodeStream rather than renderToString. This makes it a lot more clear why things like setState won't work (we have already sent down the markup to the client) and why a single render pass is necessary, even in the future when we have Suspense. This means that any solution will probably be a long term one.

I can see a couple of approaches to this problem, most has already been mentioned. The first two are the same as we do for data fetching on the server side today:

1. Render multiple times. - Bad for performance, tricky to implement, not future safe.
2. Add reducers before render. This requires knowledge of which exact components and therefor reducers will be needed before rendering, basically building a parallel tree outside of React. - Bad DX and prone to error.
3. Add all reducers serverside, register used ones, remove rest after render. This would turn the approach on its head and would (I think) work with current API. Instead of adding reducers before they are used, we subtract them after they have not been (on the server only). - Avoids tearing as a solution altogether but is not as ergonomic as 4 and might have some tricky edge cases. I think I like the proposed solution better but thought I'd throw it out there.
4. Introduce an escape hatch for voluntary tearing. This is the proposed solution.

Since this is an escape hatch, should it be so tightly coupled to first render or should this be up to the consumer to decide (in case it is needed for other now unknown usecases)? The first render-part should be easy to implement in userspace.

<Provider store={store} UNSAFE_readLatestStoreState={isFirstRender}>
// or
<Provider store={store} UNSAFE_readLatestStoreState={isServer}>

This seems more flexible but might be slightly harder to use. Not sure what's best, just thinking out loud. :)

[Ephem]

Actually:

5. Let the component loader that is the parent of the connect-wrapped component explicitly pass in a store-object with the now replaced reducer as props and use this instead of reading from context.

Son is waking up so can't write this up more right now, but wanted to dump it in here since it just struck me as something that might be possible?

[GuillaumeCisco]

Excellent discussion!
@rgrove exactly defined the issue and what is going on. It is totally linked to #935 (comment) issue.

I'll try in a first step to implement the 4 solution for testing how the app behaves.
My app uses SSR streaming. So we will have a robust return of proof of concept for this kind of app too.

Regarding 5 solution, it looks like the most intuitive to me at first. But I don't know how we could implement that. Furthermore, it is weird to override the connected state of a child from its wrapper. As the real deal is that we do not need to read it from its wrapper component once the reducer has been correctly injected in the store.

Thank you,

[Ephem]

Hmm, having a snack with the son so throwing another quick one in here:

6. Component loader-parent wraps children with new context-provider with the new values, affecting that subtree.

[GuillaumeCisco]

I've just created a PR with the 4 solution in mind, but I don't think this is what we should head for.
PR is at https://github.com/reduxjs/react-redux/pull/1128/files
I've put some console.log for having a debug phase. Will be deleted in the future.
Using this works well. But now all my components compute the storeState on their first render.

On the server, I have logs like:

recompute state for getting last one on component Routes
recompute state for getting last one on component Top
will inject challenge reducer
injected challenge reducer
recompute state for getting last one on component Search
recompute state for getting last one on component ComplexSearchToggle
recompute state for getting last one on component Base
recompute state for getting last one on component Index

On the client side:

recompute state for getting last one on component Routes
recompute state for getting last one on component Top
will inject challenge reducer
injected challenge reducer
recompute state for getting last one on component Search
recompute state for getting last one on component ComplexSearchToggle
recompute state for getting last one on component Base
recompute state for getting last one on component Index
component Top did mount
component ComplexSearchToggle did mount
component Search did mount
component Index did mount
component Base did mount
component Routes did mount

The Search Component is the one component which is lazy loaded and has to dynamic loads the reducer.
So it works correctly.

However, we force the call to store.getState() for every component on their first render. This leads to others calls each time we are switching between pages, because the component is destroyed then recreated.
Originally we wanted to make this call once and only once for injecting the reducer in the app. I'm not talking about reducers management by adding/removing them here.

Regarding solution 6. This is something I tried to achieve by creating a new context provider. Without success as react-redux connect method will refer to the state of ReactReduxContext not the new created one. Maybe there is a way to do it.

Hope it will help ;)

[AgentCoop]

I think this is a much broader problem then described.

We also have an SSR-compatible app that stopped working with the upgrade to the latest react-redux. I've tried to refactor its architecture thinking that maybe something is wrong with it but eventually gave up.

The main reason is the root component misses state changes.

I've added a test prop to the root component too see whether it changes on state updates:

export default compose(
	withRouter,
	connect(
		({ app }) => ({
			...app,
                       foo: Math.random()
		}),

		dispatch => ({
            loadOnClient: (source, clientDataLoader) => clientDataLoader(source, dispatch)
        })
	)
)(App);

and then I've dispatched an action from within a nested connected component in a way

setTimeout(() => {
 // dispatch action here
}, 1000)

just to be sure that the whole app tree is rendered to the time of the call.

The new state value will be passed on the next state update. So it kind of lags in one update. No idea how to solve this problem, I suppose we stick to 5.x for good.

[cellog]

@AgentCoop let's not give up just yet. Everyone will need this to be solved, including me, so we just have to figure out the right question to ask and then a solution will present itself. I may have time to take a look later today. It would help a lot if you could forward the codesandbox above and modify it to fit the test case you have described. Do you think that is possible? Thanks!

[Ephem]

Since ReactReduxContext is already being exported from react-redux, we can solve this problem in userland using solution 6, wrapping the subtree in a new <ReactReduxContext.Provider>. I forked the sandbox and got it to work (only made changes to withRedux):

https://codesandbox.io/s/pwmm8r3110

I think this solution makes sense.

---

Rationale (& somewhat of a summary)

Sometimes (seldom) it makes sense to update the state within a render-pass, such as dispatching an action in componentDidMount or the like. In those cases it is usually fine for React to re-render the application before committing which is also the idiomatic way, but this is not what we are talking about here.

This special case requires the update to happen within the same render-pass, both in the sandboxed client-side version and any serverside solution. The reason this is safe to do at all, is because of the constraint that only children of the component that performs the change relies on that change (or children in different parts of the tree whose parents all introduce the same change).

In order to support concurrent rendering and avoid tearing, v6 no longer fetches the current Redux store-state in every connect-component, instead relying on the version that is stored on context. Because of the above constraint however, we only have to update the context of the subtree that relies on this change, and thankfully React provides a way to do this.

---

Downsides

Downsides of this solution is a heavier implementation-burden in userland. This would be a unintended/unplanned for breaking change (of an undocumented implementation detail, so not sure it counts..). Very possible it has other downsides I can't come up with right now. :)

That being said, I'm hesitant to if it's a good idea to introduce a long term escape hatch for tearing unless it is strictly needed, this feels like a more idiomatic solution.

[markerikson]

@Ephem: I have to say this seems like the best solution I've seen so far.

[rgrove]

Thanks for all the great suggestions everyone! 😄

I tried out @Ephem's solution and it's working great. This definitely feels like the right way to go if we want to solve this in userland.

@markerikson I'm also still happy to work on a PR if you think it's worth solving this in the library (although @GuillaumeCisco's PR looks pretty close to what I'd implement, so you could just take that one). But if you'd rather we stick to a userland fix, I'm happy with that too.

[Ephem]

I'm glad you found the problem! 🎉

With that I think we can consider this issue resolved. If anyone read this and think they have an issue related to this one (code splitting with replaceReducer or "updating the data on ReactReduxContext for a subtree during a single render pass"), I'd advise you to open a new issue and reference this one. :)

[markerikson]

Yep.

I'd really appreciate it if someone could take the time to write up a new React-Redux docs page on this topic. Maybe focus it specifically on the code-splitting aspect for now, and we can generalize the "here's how to access the store if you really need to" part later.

[bumi001]

Comparing what @rgrove did with the Provider's code, I have a related question. He initially supplies the Provider with a store, but keeps modifying the store (by adding reducers dynamically) outside the Provider. Inside the Provider, the store is kept in its state. I think the right approach would be to have a handler in the Provider that takes a new store as parameter and call setState with the new store. Am I missing something?

[markerikson]

@bumi001 : uh... no. The point of this discussion is to keep using the same Redux store reference, but update it with a new reducer function (and thus a new store state value). No one said anything about creating or using a different store reference. Why are you bringing that up?

[bumi001]

Comparing what @rgrove did with the Provider's code, I have a related question. He initially supplies the Provider with a store, but keeps modifying the store (by adding reducers dynamically) outside the Provider. Inside the Provider, the store is kept in its state. I think the right approach would be to have a handler in the Provider that takes a new store as parameter and call setState with the new store. I would add this handler to the value supplied to Context.Provider. Am I missing something?

[bumi001]

It looks to me that the store is passed down via the Provider and through the AppContext to child components. Since Provider is the parent of App and all its children, in order to be consistent with React's design, if you want to change the state of the parent then you need to do it via a handler than an external reference. I could also keep an external reference to something in the store and change it via that instead of dispatching an action. But, that would be inconsistent with redux's design.

[markerikson]

@bumi001 : I'm... really not sure what you're trying to say. Seriously. What point are you trying to make / question are you trying to ask?

[bumi001]

The render function in Provider is
render() {
const Context = this.props.context || ReactReduxContext

return (
  <Context.Provider value={this.state}>
    {this.props.children}
  </Context.Provider>
)

}

I am interested in dynamically injecting reducers. I do it like below, which is a simplified version of what is in react-boilerplate.

import React from 'react';
import hoistNonReactStatics from 'hoist-non-react-statics';
import { ReactReduxContext } from 'react-redux';
import history from './history';

export default ({ key, reducer }) => WrappedComponent => {
class ReducerInjector extends React.Component {
static WrappedComponent = WrappedComponent;

static contextType = ReactReduxContext;

componentWillMount() {
  const store = this.context.store;
  store.injectedReducers[key] === reducer;
  store.replaceReducer(createReducer(store.injectedReducers)(history));
}

render() {
  return <WrappedComponent {...this.props} />;
}

}

return hoistNonReactStatics(ReducerInjector, WrappedComponent);
};

The function createReducer calls combineReducers. The difference from rgrove's code is that I get the store from ReactReduxContext.Consumer. My question is, if I update the store this way, is it going to change/update the store and storeState in Provider?

[bumi001]

store.injectedReducers[key] = reducer; //I had a typo

[markerikson]

if I update the store this way, is it going to change/update the store and storeState in Provider?

Yes, because the store dispatches an action when you call replaceReducer(), and <Provider> is subscribed to the store.

[bumi001]

I see. Got it. Thank you. I didn't look into replaceReducer. I saw the subscribe in Provider.

[bumi001]

In the context of react-boilerplate, I do the following to deal with the initialization issue:
(1) export initialState from the reducer (in reducer.js)

import { fromJS } from 'immutable';

export const initialState = fromJS({
username: '',
});

(2) import initialState in selectors.js and use it like:

import { initialState } from './reducer';

//immutable.js map returns initialState if 'home' is not found in state
const selectHome = (state) => state.get('home', initialState);

(3) where 'home' is the key given to injectReducer in index.js
import injectReducer from 'utils/injectReducer';
import reducer from './reducer';

const withReducer = injectReducer({ key: 'home', reducer });

[bumi001]

Here is my PR for react-boilerplate that uses react-redux:6.0.0 and connected-react-router:6.1.0 if anyone is interested. It passes all their tests.
https://github.com/bumi001/react-boilerplate