`<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?