[FIX]: #1219 Save references before update

[josepot]

I will create the corresponding test ASAP. But I'm quite confident that this fixes #1219

Update

This is what is happening:

Most of the times the update triggered by invoking the dispatch of forceComponentUpdateDispatch happens "asynchronously" (in the next event loop). However, in other instances it happens synchronously.

When it happens asynchronously, everything works as expected because the references are being updated before the next render. However, in those rare cases where the update happens immediately, the references have not been updated before the next render, which causes the following block:

        if (
          childPropsFromStoreUpdate.current &&
          wrapperProps === lastWrapperProps.current
        ) {
          return childPropsFromStoreUpdate.current
        }

To return the same actualChildProps that were evaluated in the previous render. Therefore, the useMemo hook of renderedChild does not evaluate its compute function... That's why in those rare instances there is no update until the next dispatch.

Mutating the references before the dispatch fixes the issue.

The problem is that I can't find a way to create a test that reproduces this behavior 😞 Maybe it's because I'm tired (I'm in CEST and it's almost 5am). So, I will go get some rest and see if tomorrow I can find a way to add a test for this... But that would be testing an implementation detail, so I'm not even sure that a proper test for this bug exists. Maybe tomorrow I will see it differently...

The only thing that I'm sure of is that while this change can not break anything, it can fix #1219. So, IMHO it's save to merge it.

[netlify]

Deploy preview for react-redux-docs ready!

Built with commit b2c4230

https://deploy-preview-1220--react-redux-docs.netlify.com

[markerikson]

Yeah, this writeup matches my own mental model of what should happen, and what is probably happening.

I'd love a test for this too, and would really appreciate it if someone can still try writing one. But, looking at this change, I'm fine with merging this one in now.

Awesome work!

[MargaretKrutikova]

I debugged the reproduction repo, and was able to figure out exactly when the bug happened. It appears that if you use ReactReduxContext.Provider manually, and then dispatch an action from outside of event handlers (exactly as @luskin noted that the bug appeared when dispatching from a thunk or a saga), forceComponentUpdateDispatch in connectAdvanced will cause the component re-render right away without having a chance to set the new ref with childProps.

There is a flag called isBatchingUpdates inside react, which is set to false in the described scenario, which causes react to call performSyncWork function and re-render right away.

However, when I tried dispatching an action from an event handler in the same setup, isBatchingUpdates got set true and didn't cause an immediate re-render. I am guessing this is somehow related to this SO answer by Dan Abramov where he says that "there is yet no batching by default outside of React event handlers", but I am unsure about it, since the answer was written a year ago.

The library that was used in the reproduction repo (react-redux-modules) uses ReactReduxContext.Provider under the hood, which is what caused the bug in the first place.

My question now, what is the typical use-case scenario for using ReactReduxContext.Provider? Have never used it myself and I don't understand why you might want to wrap your components in both the usual Provider and ReactReduxContext.Provider, so I would really appreciate if someone could provide an example or an explanation :)
I might be able to write a test for this case, if it is still of interest :)

[markerikson]

@MargaretKrutikova : nice detective work! yeah, that totally makes sense to me.

Looking at the source code at redux-dynamic-modules/redux-dynamic-modules-react/DynamicModuleLoader.tsx, I think this is actually tied to the issues described in #1126 , where there's a need to manually retrieve the store in case any actions were dispatched during the loading sequence . The pattern we worked out there, which involves rendering the <ReactReduxContext.Consumer> to grab the store instance and store state, is shown in the Accessing The Store docs page. Except that's the

Oh.

OH!!!!!

NOW I see what the real issue is!

When you render <ReactReduxContext.Provider value={something}>, you're overriding what our own <Provider> component has put into that context.

redux-dynamic-modules does this:

    private _renderWithReactReduxContext = () => {
        const { store } = this.props;
        // store.getState is important here as we don't want to use storeState from the provided context
        return (
            <ReactReduxContext.Provider
                value={{ store, storeState: store.getState() }}>
                {this._renderChildren()}
            </ReactReduxContext.Provider>
        );
};

In v7, our context value looks like {store, subscription}. When the lib renders this way, it's going to keep any nested children from reading that subscription value, because it won't exist any more. That means the nested components won't actually be subscribed to our Subscription instance from <Provider>, and won't update in the right sequence (either from the cascading updates, or the batching). That's actually a bug on their end.

I've filed microsoft/redux-dynamic-modules#76 to bring this up. (paging @navneet-g ... )

[MargaretKrutikova]

Wow, what a turn :) Thanks for explaining it to me, @markerikson!

So there is no need to come up with a test for this use-case, I assume. I wasn't able to replicate it in test in a clean way anyway. Since react wants you to wrap all your updates in act, or you will get a fat warning otherwise (When testing, code that causes React state updates should be wrapped into act(...)), but act inside does batching of updates, so it basically fixes all the wrong behaviour for you and the test will always pass.

Good thing we figured the actual bug wasn't in react-redux :)

[markerikson]

Yeah, for a couple of the edge case tests we recently added, I actually had to not wrap the updates in act() to simulate synchronous updates without React flushing effects. Dirty, but sorta works?

[MargaretKrutikova]

Yeah :) It is a bit weird that I didn't see any warnings when I ran the tests though ..