mapStateToProps is called when component is not due to render when a dispatch is triggered from componentDidUpdate

[ykagan]

When a dispatch is triggered from a lifecycle method as a side-effect of a re-render from a previous dispatch, we wind up calling mapStateToProps on components that are never going to be rendered and are about to be unmounted.

#1263 fixed a very similar issue when multiple dispatches are triggered simultaneously.

Minimal repro is here: https://codesandbox.io/embed/react-redux-issue-debug-rvr-8whlb

[markerikson]

Thanks for filing this.

Hmm. It may very well be an unavoidable aspect of how the current implementation works, but I'd have to spend time to examine the exact timing sequence of React effects vs subscription callback execution to figure that out. To be honest, that's a very low priority for me atm. If you could do that, it would probably help me think through what's going on.

[timdorr]

Related to #1396?

[ykagan]

The sequence of events is

Dispatch 1
Parent mapState
Parent render
// Child is rendered here but child mapState() is queued rather than running immediately
Parent componentDidUpdate 
Dispatch 2
Parent mapState
Child mapState // Redux state is invalid at this point
Parent render
<no child render>

In v6.0.1 the order is

Dispatch 1
Parent mapState
Parent render
Item mapState
Item render
Parent componentDidUpdate
Dispatch 2
Parent mapState
Parent render

The fact that item.mapState and item.render is not called immediately on the first render before the lifecycle method is kinda unexpected.

I think the problem may be that useLayoutEffect() is triggered after componentDidUpdate is resolved rather than immediately on render?

[timdorr]

6.0.1 isn't the correct version to be using here. It has known issues that led to us abandoning its approach entirely. You should be on 7.x, as it's built on Hooks and doesn't have the same quirks or performance issues.

[markerikson]

@timdorr : the sandbox is on v7, and the point being made is that v6 and v7 are behaving differently.

@ykagan : There are definitely differences in internal implementation in v6 and v7 that lead to different timing of behaviors. I documented a lot of that in my post The History and Implementation of React-Redux.

Also, I'm not entirely clear on what you mean by "item.connect() is being called". Are you referring to the mapState functions, specifically?

[ykagan]

Yes, each of those connect calls refers to mapState function calls.

What I meant is that we're seemingly rendering the children in the first dispatch but the child render function doesn't get called but instead gets queued up behind the next dispatch and re-render.

The problem we have is that we can then no longer assume that the child's mapState function gets called with valid state (eg: we can no longer assume it's rendered top-down like we could with v5 and v6).

I see in that blog post you mentioned the "zombie child" issue - is that issue specific only to using hooks or is it generic to any use of v7?

[markerikson]

connect has always been supposed to enforce top-down behavior and eliminate "zombie children" since v5.

If that's not happening here, then there's an edge case that's slipping through the cracks.

[ykagan]

connect has always been supposed to enforce top-down behavior and eliminate "zombie children" since v5.

If that's not happening here, then there's an edge case that's slipping through the cracks.

Kinda? Here's a slightly more clear example with the invariant removed: https://codesandbox.io/embed/react-redux-issue-debug-rvr-8whlb

dispatch update 
Parent mapStateToProps 
Parent render 
Parent componentDidUpdate 
dispatch remove 
Parent mapStateToProps 
child mapStateToProps // why is this called here?
Parent render 
Parent componentDidUpdate 

the child render function is never called but mapState is, and it's called out of order. its a bit ambiguous whether this is a "zombie" since it never gets rendered, but it makes writing logic in mapState difficult.

[ckedar]

The issue is not because of top-down update. This can be verified by overriding the top-down update by providing store={store} prop to App and Item element.

When run with version 5.0.7, the test case does not throw error, however if we add console.log before invariant we can see that mapStateToProps does throw invariant error, just that this error gets swallowed as component does not render subsequently.

In 7.1.1, for some reason, the error caught during mapStateToProps is re-thrown during un-mounting, from unsubscribeWrapper. This seems to be unnecessary.

[karolinkas]

I there a workaround for this?

[lukas1994]

Any updates? Having the same issue.

[markerikson]

I'm going to close this as a WONTFIX for now, largely because we're encouraging folks to stop using connect (even though it'll be fully supported in v8). This is a real bit of behavior that I can understand is annoying, but I don't think there's a meaningful change we can make at this point, and I don't intend to spend time fixing it myself.

If someone still feels strongly about this please comment, or even better file a PR to improve behavior here.