mapStateToProps called when component not due to render

[justincrow]

Hi,
I have a conditionally displayed component, based on a value in redux. All components below are connected.

eg in a wrapper component's render method:
{ (showValueInRedux) ? <ComponentUsingShowValue/> : <EmptyComponent/> }

ComponentUsingShowValue has a mapStateToProps that uses redux state that it expects to be there, hence we guard for that with showValueInRedux in the wrapper component.

In version 6, mapStateToProps in ComponentUsingShowValue is only called when that component is due to render.

In version 7, mapStateToProps is called even if the wrapping component has switched to choosing EmptyComponent due to showValueInRedux being false (after having once been true).

This causes an exception in our current code because the redux values that mapStateToProps uses ( in the now non-rendered component) are not available.

Please could you clarify this change in behaviour?

[timdorr]

We still ensure top-down update ordering. It is one of our biggest challenges with this library. AFAIK, we didn't change this behavior in 7.0.

Can you provide an example of what you're seeing in a Codesandbox? We need a reproducible test case to work with in order to figure out what's going on here.

[Andrekra]

I can confirm we're having the same issue (effectively breaking our application when upgrading from v6 to v7)

Here's an simple example (mind the typo's):

const shouldRender = WrappedComponent => props => props.shouldRender ? <WrappedComponent {...props} /> : null

const MyComponent = compose(
  connect(state => ({ shouldRender: Boolean(state.page) })),
  shouldRender,
  connect(state => ({ title: state.page.title }))
)(MyBaseComponent)

<MyComponent />

store.dispatch({ type: 'CLEAR_STATE' }) // => Sets state.page to null

In this example, if shouldRender becomes true, it mounts MyBaseComponent. When for some reason the state is cleared (new api request), it unmounts again (state.page === null). However the last connect still receives a state update (state.page has changed) even though it's about to get unmounted. Even if we would guard state.page.title, it still would be a performance hit as all child components would recalculate a new value (think of custom mappers of data on every child component)

We have a HOC that prevents mounting the component if certain data is not in the state (page not loaded). The next HOC (the connect) uses that data to display some information. The reason why we approach it this way, is that we can block a complex panel with many child components (with their own connects) from mounting, so the selectors used in the connects of the children don't break.

I haven't resolved the issue.

[markerikson]

@Andrekra : as Tim said, it would really help if you can provide a project that reproduces the issue, preferably as a CodeSandbox.

[iamandrewluca]

I also encountered this problem after upgrading to ^7.0.
After some analysis with my colleague we understood why this happened.
But did not understood why this was not happened before.
In our case this is how JavaScript scopes works.

https://codesandbox.io/s/p9mp9rqq4m

At first render AppImpl.props.myObject is undefined and TheConsumer.props.render function catches it into scope.
TheConsumer.props.render will be called only when TheProvider.props.value.myObject will have some value, but myObject from TheConsumer.props.render will still be AppImpl.props.myObject that was caught in first render and is undefined.

And this is the right behaviour.
A solution to this is to move all logic from TheConsumer.props.render into a Component, and connect that Component to redux

When app renders you'll se in console

TheProvider mapState 
undefined
App mapState 
undefined

And an async action was dispatched to set myObject. After some delay you'll see

TheProvider mapState 
Object {test: "!!!"}
The above error occurred in the <Context.Consumer> component:
  in TheConsumer (created by AppImpl)
  ...

As you can see TheProvider can see the new state, and TheConsumer.props.render is called, that has a reference to myObject that is undefined, and a error is thrown because test does not exist on undefined

Also if I set the async action somewhere bellow 100ms, console.log is showed only once with state already set, and I think this is something related to react async rendering.

[Andrekra]

@markerikson Here's an example https://codesandbox.io/s/v629pkllj0

[MrWolfZ]

@markerikson I haven't debugged this yet, but just going from the code could the issue be in this line?

I think what might be happening is that the useLayoutEffect is called before the cleanup function of the nested component's store subscription effect is called, i,e. the nested subscriber is notified before the containing component is properly unmounted.

[MrWolfZ]

Alright, notifyNestedSubs is definitely called before the child is unmounted. Here's a test case that shows this. For some reason completely unclear to me this test will actually succeed if the first INC is dispatched inside rtl.act. It seems there is definitely some ordering issue between the effects based on how they are set up.

Also, this test succeeds if we render the child from the start and only hide it. It has to be first hidden, then shown, then hidden again. Therefore I think this is what is happening:

• the parent is rendered and the effects are registered
• the store gets updated
• the child is now getting rendered and the effect (and its cleanup) are registered
• the store gets updated
• since the parent was in the DOM without the child, it's effects are called first before the child is unmounted

I don't know if this is a react issue to be honest, since it feels like this should not happen.

it('should not notify nested components after they are unmounted', () => {
  @connect(state => ({ count: state }))
  class Parent extends Component {
    render() {
      return this.props.count === 1 ? <Child /> : null
    }
  }

  const mapStateToProps = jest.fn(state => ({ count: state }))
  @connect(mapStateToProps)
  class Child extends Component {
    render() {
      return <div>{this.props.count}</div>
    }
  }

  const store = createStore((state = 0, action) =>
    action.type === 'INC' ? state + 1 : state
  )
  rtl.render(
    <ProviderMock store={store}>
      <Parent />
    </ProviderMock>
  )

  expect(mapStateToProps).toHaveBeenCalledTimes(0)
  store.dispatch({ type: 'INC' }) // works with this: rtl.act(() => { store.dispatch({ type: 'INC' }) })
  expect(mapStateToProps).toHaveBeenCalledTimes(1)
  store.dispatch({ type: 'INC' })
  expect(mapStateToProps).toHaveBeenCalledTimes(1)
})

[MrWolfZ]

My analysis above was wrong.

Scratch all of the above. It is actually a different timing issue. The problem is that the store subscription is created inside useEffect. In the sandbox from @Andrekra (and the test above) the dispatches are done immediately after another without giving react a chance to execute effects. That means the the order of events is this:

• the parent is rendered and the store is subscribed to
• the store gets updated
• the child is now getting rendered and the store subscription effect is queued up
• the store gets updated again
• the parent gets updated and hides the child
• the update on the child forces the queued store subscription effect of the child to be executed (which calls checkForUpdates immediately and with the latest state that has the second update already applied)
• the child store subscription effect gets immediately cleaned up

You can see in this sandbox how this happens with just plain react. The second toggling render is printed before the child is mounted for the first time.

The proper fix (that passes all tests including the one I posted above) is to use useIsomorphicLayoutEffect for setting up the store subscription. I have created a PR for this. I don't see another way of solving this since when the store subscription effect is executed it is impossible to know whether the component was already queued to be unmounted.

[markerikson]

Yeah, I wondered whether having the store subscription in an async effect was going to cause issues.

[justincrow]

We are also experiencing the same issue with 7.0.2. I've tried to re-create with a minimal example in codesandbox, but I cannot get the same error. I'll keep trying.
Please can this be re-opened?

[MrWolfZ]

Do any of you that experience this issue use concurrent mode?

@justincrow do you have exactly the same issue, i.e. a non-rendered component's mapState is called? I want to clarify whether this is still a timing issue with unmounting components or something different.

[iamandrewluca]

Do any of you that experience this issue use concurrent mode?

No

With new patch release I still encounter the issue. And the problem it seems to be as I said above

At first render AppImpl.props.myObject is undefined and TheConsumer.props.render function catches it into scope.

[justincrow]

Hi @MrWolfZ ,
Yes, exactly the same issue.
We don't use concurrent mode (that I'm aware of).
mapState is called on a non-rendered component.
I'll try to find some more time to re-create it in the smallest form (our app has deep nesting).
Many thanks.

[MrWolfZ]

@iamandrewluca I had a look at your sandbox and the issue is something completely different, since in your case the mapState of AppImpl is not called, while it should. I am not 100% sure on why it behaves like it does, but it seems to me that he issue is very specific to the use of providers and consumers. You can in fact reproduce your issue without using a render prop (i.e. the problem is not related to capturing an old value in a closure). I believe the cause is that when a provider gets a new value it will not automatically re-render all children, but it walks the component tree to find all components interested in the context. In the sandbox that means the consumer callback is executed, but the containing component is not re-rendered (and therefore its mapState is not called).

You have already pointed out a workaround for your sandbox and in mine the workaround would be to simply check if myOuterObject has a value. In any case, since it is something unrelated to this issue I recommend that you open a new one.

[MrWolfZ]

@justincrow my current theory would be that something like this happens:

• a store update causes a parent component to hide the child (i.e. the parent render is called)
• react does not commit this change to the DOM immediately, but defers it (this should only be possible in concurrent mode as far as I understand, but maybe not?), so the child does not get unmounted
• another store update occurs, and the child is notified, even though it should already be unmounted

[markerikson]

If anyone's still seeing this issue, we really need an example project that reproduces the problem.

[justincrow]

I've finally managed to re-create the error in codesandbox.
Please excuse the surplus code, it needs tidying, but it does show that mapStateToProps is called in customerDashboardTab when it is not due to render when using react-redux 7.0.2, but behaves as expected in 6.0.1

https://codesandbox.io/s/z55jvr7nm

[markerikson]

@justincrow : cool, thanks. Can you point to specifically which files are showing the issue, and the exact steps to run in the sandbox to demonstrate that?

[justincrow]

When the sandbox runs, you'll see two links, 'Jim' and 'Bob', when you click on them, an initial action is dispatched to clear the id (which is checked to see whether to display the customer dashboard tab), then a timeout is set (to represent network activity) which sets the selected id.
With 7.0.2, you can see in the console logs that mapStateToProps is called in customerDashboardTab, even though the id is undefined and the component should not be rendered.
Switching to 6.0.1, the mapStateToProps is not called, and so does not error.

[MrWolfZ]

I made the sandbox much smaller, so that it is easier to analyze. I need to go to sleep now, and I will only be able to continue on Friday, but maybe somebody else can solve this. Based on my reduced example the issue can only be due to either the timing of the async fetch action in the select inbox thunk or due to a state update occuring inside componentDidUpdate in InboxView.

[MrWolfZ]

I have further minimized the sandbox and the problem seems clear now: in certain situations two dispatches just after another will lead to checkForUpdates (the store subscription callback) being called twice after another. If the result of the second call doesn't change, it will immediately trigger notifyNestedSubs. This causes the issue. I will spend some time later today or tomorrow to turn this into a test case and then hopefully come up with a fix (though I don't really have an idea right now how this could be fixed).

[MrWolfZ]

I have created a new issue for this that contains a truly minimal reproduction sandbox. I will shortly create a PR with the repro as a test case and a potential solution.

[MrWolfZ]

@justincrow 7.0.3 was just released and your sandbox works correctly with that version. Could you please test it in your app as well and see if there are still issues?

[justincrow]

@MrWolfZ Great work - this issue is resolved in our app.
Thank you.