React-redux: `<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:

https://github.com/reduxjs/react-redux/blob/595ae8fbfd251c75bcf1a7855996d210cfa97f0f/src/components/connectAdvanced.js#L200-L214

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:

1. 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 https://github.com/reduxjs/react-redux/issues/935#issuecomment-447087011 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:

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

[markerikson]

Yeah, given that this is really a userland concern in the first place, I'm going to say that a userland solution is appropriate.

What I _would_ like to see is an addition to the docs that covers this. There's an open PR to add a "Code Splitting" page to the Redux docs that I haven't gotten back around to looking at. I think it would also be valuable to have some info on that topic in the React-Redux docs as well.

I'd appreciate it if someone could put together a docs PR that adds a new page under an "Advanced" sidebar heading discussing how to handle things like dynamic reducer injection from components., and specifically covers this use case.

[timdorr]

BTW, I cleaned this up just a tad:

export default function withRedux(Component) {
  const { mapStateToProps = null, reducers } = Component;
  const ConnectedComponent = connect(mapStateToProps)(Component);

  class WithRedux extends React.Component {
    static contextType = AppContext;

    constructor(...args) {
      super(...args);
      this.context.addReducers(reducers);
      this.firstRender = true;
    }

    render() {
      if (this.firstRender) {
        this.firstRender = false;
        return (
          <ReactReduxContext.Consumer>
            {reduxContext => (
              <ReactReduxContext.Provider
                value={{
                  ...reduxContext,
                  storeState: reduxContext.store.getState()
                }}
              >
                <ConnectedComponent {...this.props} />
              </ReactReduxContext.Provider>
            )}
          </ReactReduxContext.Consumer>
        );
      }
      return <ConnectedComponent {...this.props} />;
    }
  }

  return hoistStatics(WithRedux, ConnectedComponent);
}

And if SSR isn't a concern at all:

export default function withRedux(Component) {
  const { mapStateToProps = null, reducers } = Component;
  const ConnectedComponent = connect(mapStateToProps)(Component);

  class WithRedux extends React.Component {
    static contextType = AppContext;

    constructor(...args) {
      super(...args);
      this.state = { mounted: false };
    }

    componentDidMount() {
      this.context.addReducers(reducers);
      this.setState({ mounted: true });
    }

    render() {
      return this.state.mounted ? <ConnectedComponent {...this.props} /> : null;
    }
  }

  return hoistStatics(WithRedux, ConnectedComponent);
}

[GuillaumeCisco]

Hello there, just tried to implement this new solution.
So at first, it looks like it is working: no more errors. Reducer is dynamically injected from a lazy loaded component. Works on the server, works on the client.
BUT, now, every time I want to update my reducer, the reducer is well called, but the connected components are never updated...no more render calls.
Do you think this is due to overriding the ReactReduxContext.Consumer?
Can someone confirm me the last accepted solution works in his project?

Information: I'm not using hoistStatics from recompose.
I write the withRedux part like:

import React, {Component} from 'react';
import {ReactReduxContext} from 'react-redux';

export default function withRedux(WrappedComponent) {
    class WithRedux extends Component {
        constructor(...args) {
            super(...args);
            this.firstRender = true;
        }

        render() {
            if (this.firstRender) {
                this.firstRender = false;
                return (
                    <ReactReduxContext.Consumer>
                        {reduxContext => (
                            <ReactReduxContext.Provider
                                value={{
                                    ...reduxContext,
                                    storeState: reduxContext.store.getState(),
                                }}
                            >
                                <WrappedComponent {...this.props} />
                            </ReactReduxContext.Provider>
                        )}
                    </ReactReduxContext.Consumer>
                );
            }
            console.log('not first render');
            return <WrappedComponent {...this.props} />;
        }
    }

    return WithRedux;
}

(reducer is injected with redux-reducers-injector, reduxContext.store.getState() correctly give me the state with updated reducer).

And call it like:

const mapStateToProps = (state, ownProps) => ({
    item: state.myDynamicallyInjectedRecuer.item,
});

export default withRedux(connect(mapStateToProps)(MyComponent));

Now, when calling actions to update reducers, no more render are called.

[markerikson]

Hmm. No, that doesn't look correct. If you don't have something that triggers a re-render of the WithRedux class, then it will continue to override the context value.

[GuillaumeCisco]

Thank you @markerikson , can you help me understand what does not look correct. The code provided or the behavior?
Do you think using hoistStatics and make the connect part inside the withRedux component will make it behave correctly?

Calling my actions (like fetching a ressource) normally call a rerender. As discussed in https://github.com/reduxjs/react-redux/issues/1133, the mapStateToProps method is not even called. As a result, the following render is not called.

[markerikson]

No. The problem, as far as I can see, is that your withRedux component won't actually re-render itself unless a parent component happens to re-render. So, nested components will never see a new state value.

Try two things for me:

• Add a console.log() statement at the start of render(), and see if it ever actually renders a second time
• Add a setTimeout() in componentDidMount(), and call this.forceUpdate() to ensure a second render happens.

[Ephem]

Hmm, this is some tricky stuff. Anything inside of <ReactReduxContext.Consumer> should be re-rendered when the context is updated right? However, the actual WithRedux component is not.

Your solution and @timdorr's are very close to each other, in that the if (firstRender) is _outside_ the <ReactReduxContext.Consumer>. This means that the console.log('not first render'); will never happen, since the WithRedux component is never re-rendered. Studying @timdorr's cleaned up version in React DevTools we can see this in that the inner Provider never goes away:

This version still works in that the wrapped component still re-renders though.

In the original solution, the Provider does go away since the if-statement is _inside_ of the Consumer:

This solution has another drawback though, that the Consumer will never get removed (even if WithRedux re-renders), not sure if this has any side-effects besides a small performance hit.

If I add a console.log at the start of render in the CodeSandbox, it is only logged on first render. If I add one inside of the ContextConsumer it is run on every click though. This is true both for the original and the cleaned up version.

So, two things:

1. @GuillaumeCisco Does it work if you put the if (firstRender)-statement _inside_ of the Consumer?
2. If you are implementing this for release in a library (or otherwise), do note that is really tricky to get right and we probably haven't found the perfect solution yet. Take care and test extensively, including performance. :) (And share your findings!)

Addendum: Just to be clear, I _think_ the wrapped component should re-render in all these cases (since everything inside of the Consumer re-renders), but with slightly different behaviour. In the original solution, the storeState on the context will not update mid-render-pass after the first render. In the refactored solution, I think the storeState is updated mid-render-pass even after the first render, possibly leading to accidental tearing.

[Ephem]

Actually, the more I think about this, the trickier it gets, I just thought of another case, unrelated to the above comment.

Scenario:

1. We render two React roots (on the client)
2. One root (A) uses the above solution to add a reducer
3. The other root (B) receives an user event which leads to a dispatch and state update, this is higher prio than (A)
4. (A) had already started rendering, this was paused for (B), then (A) is resumed and reducer is added. The above solution for adding a reducer updates the state for an entire subtree. If the state-update from (B) is used both inside and outside of this subtree, we now have tearing between different parts of the application..

Soo hard to wrap ones head around these things.. :)

(This could also happen on the same root but with different prio-events, but choose the two root-scenario because I find it easier to describe)

Possible solution:

1. Use replaceReducer on the real store so that it updates just as before
2. Instead of:

<ReactReduxContext.Provider value={{ store, storeState: store.getState() }}>

we need to patch the storeState manually with only the the state that is added from the new reducers initial state, preserving the "old" state from context to avoid tearing from events from other roots.

    renderWrapped = ({ store, storeState }) => {
      if (this.firstRender) {
        this.firstRender = false;
        const subtreeState = {
          ...storeState
        };
        for (const [key, reducerFunc] of Object.entries(reducers)) {
          if (!subtreeState[key]) {
            subtreeState[key] = reducerFunc(undefined, {});
          }
        }
        return (
          <ReactReduxContext.Provider
            value={{ store, storeState: subtreeState }}
          >
            <ConnectedComponent {...this.props} />
          </ReactReduxContext.Provider>
        );
      }
      return <ConnectedComponent {...this.props} />;
    };

I updated the CodeSandbox with this solution.

(Btw, tearing _between_ roots is still an unsolved problem in React when using state that lives outside of the UI-tree like Redux does. This early proposal rfc describes that problem and a proposed solution well.)

[Ephem]

Oh, another thing that adds to this complexity is what @markerikson posted in his first comment:

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

Source here

This runs on every didMount and didUpdate which means:

1. Whenever we use replaceReducer inside the render-loop clientside, we will re-render that loop an extra time
2. If different prio events or different roots both update the state, the last to finish rendering will re-render with the latest state

Nr 2 should lead to eventual consistency and avoid tearing so maybe the solution in my latest comment isn't needed? On the other hand, couldn't this lead to starvation? For example, if high prio animation events updates the store-state, a low prio render might never finish because it keeps re-rendering? (Maybe this has or should be discussed elsewhere since it's a separate issue)

[markerikson]

@Ephem : no, that snippet is in <Provider>, not connect(). That's only for handling the case where actions were dispatched during the initial app mount sequence.

[markerikson]

And yeah, let's not concern ourselves with the tearing / multi-root aspects atm. As Sebastian just posted at https://github.com/facebook/react/issues/14110#issuecomment-448000121 , those are complex issues the React team hasn't solved yet. We can't do anything about implementing a permanent solution until they know what such a solution looks like.

[Ephem]

Oh, I see! Thats good. :) (I realised after your comment that the reason it runs twice in the Sandbox is that we do two ReactDOM.render there)

I absolutely agree about the more complex multi root-aspects. That said, I do think it's important to not do store.getState() in something like WithRedux, that could lead to tearing in single root applications with different prio renders as well.

(Edit: And this has the nice side-effect that it also avoids tearing within a single root _triggered_ from other roots, while the issue of tearing _between_ roots still stands.)

[GuillaumeCisco]

Thank you folks for all these comments.
I just woke up, let me answer to your questions:

• Add a console.log() statement at the start of render(), and see if it ever actually renders a second time
  Just added a console.log at the start of the render and it does not render a second time.

• Add a setTimeout() in componentDidMount(), and call this.forceUpdate() to ensure a second render happens.
  Adding a setTimeout in componentDidMount does ensure a second render as expected.

• @GuillaumeCisco Does it work if you put the if (firstRender)-statement inside of the Consumer?
  Unfortunately it does not. I have the exactly same architecture as your first image.
  Code is:

import React, {Component} from 'react';
import {ReactReduxContext} from 'react-redux';

export default function withRedux(WrappedComponent) {
    class WithRedux extends Component {
        constructor(...args) {
            super(...args);
            this.firstRender = true;
        }

        renderWrapped = ({store, storeState}) => {
            console.log(WrappedComponent.displayName);
            if (this.firstRender) {
                this.firstRender = false;
                return (
                    <ReactReduxContext.Provider
                        value={{
                            store,
                            storeState: store.getState(),
                        }}
                    >
                        <WrappedComponent {...this.props} />
                    </ReactReduxContext.Provider>
                );
            }
            console.log('not first render');
            return <WrappedComponent {...this.props} />;
        };

        render() {
            return <ReactReduxContext.Consumer>
                {this.renderWrapped}
            </ReactReduxContext.Consumer>;
        }
    }

    return WithRedux;
}

Provider does go away if I add the

        componentDidMount() {
            setTimeout(() => this.forceUpdate(), 1500);
        }

part.
But actions on my connected component will still not rerender it (mapStateToProps not called).

• If you are implementing this for release in a library (or otherwise), do note that is really tricky to get right and we probably haven't found the perfect solution yet. Take care and test extensively, including performance. :) (And share your findings!)
  I'm only testing things right now ;) Don't worry.

Regarding the new concern you talked about. Using redux-reducers-injector will make transformations on the global referenced store. So I don't even know if it is an issue. I need to test more these particular use cases.

[Ephem]

@GuillaumeCisco That's what I thought, something else must be causing this (but I did think the Provider would go away actually..)

Could you provide a CodeSandbox where this problem is happening? That would make it a lot easier to debug.

[GuillaumeCisco]

@Ephem I tried to reproduce the current issue in codesandbox with react-redux 6.0.0 : https://codesandbox.io/s/q8on7x46r6
"Unfortunately", this code works correctly with react-redux 6.0.0. The state is well updated with my injectReducer code...
This codesandbox is the closest to what my project ssr app is. I do not know how to ensure the bug like @rgrove did. Maybe I'm missing something. Maybe it is because I do not use the create-react-app template.
I've attached the withRedux file to the codesandbox, for future testing.

Right now, I have nothing more :/

Hope it will help,

[GuillaumeCisco]

Ok folks, I have very good news!!!
It works correctly with redux-reducers-injector and withRedux implementation (either with if (firstRender)-statement inside of the Consumer or not!).

For the record, As you could see, I tried to create a codesandbox to expose the issue, but... it worked...

What is going on?
The codesandbox dependencies use the very latest by default. That's why it worked
For debugging purposes, I had to set react-dom 16.6.3 back to 16.5.2, otherwise I had absolutely no errors logs in the console and we could not understand what was wrong (a.k.a state not updated with injected reducer). It allowed us to create the withRedux component. This component fixes the issue with the context correctly updated with dynamic injection but a new issue appeared: no more rerender!

Simply setting react-dom to 16.6.3 makes it works!!!! :tada:

That's why all my attempts to create a mini project to show the bug failed.
Codesandbox with bug (react-dom 16.5.2) : https://codesandbox.io/s/q8on7x46r6
Codesandbox without the bug (react-dom 16.6.3) : https://codesandbox.io/s/ryxk561qp4

Thank you all for your help, I'm now thinking to a way to make this code available.
I'm not sure its place has to live in redux-reducers-injector neither in react-universal-component.

Maybe in the documentation of react-redux? Dealing with dynamic injected resources to the state?

Thanks again,

[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