mapStateToProps called on componentWillUnmount

[benjamindulau]

redux v4.04
react-redux v7.1.0 (same issue with 6.x)

Maybe related to #351 or #1226.

In a smart component, I have an unexpected call tomapStateToProps when executing componentWillUnmount()

To provide you with some context:

We're in a CRM with some routes rendering similarish lists of items like users, leads, messages, etc.. We use the same smart component for controlling the List behavior. A list can be rendered by specifying a type as prop so the container can tell the action which entity types it should fetch and the selector which type it should return as results. Everything is normalized using normalizr in a shared entities substate.

Here is a (shortened) code sample where I can experience this issue:

class ListContainer extends Component {
    static propTypes = {
        type: PropTypes.oneOf(['leads', 'offers', 'messages']),

        // Injected by React Redux
        results: PropTypes.array.isRequired,
        dispatch: PropTypes.func.isRequired,
    };

     componentWillUnmount() {
         // proper unload
         this.props.dispatch(actions.list.reinitialize());
     }

    render() {
        return (
            <List
                results={this.props.results}
            />
        );
    }
}

export default connect(
    (state, props) => ({
        results: selectors.list.getResults(state, props.type),
    }),
)(ListContainer);

The getResults selector is similar to:

export function getResults(state, entityType) {
    return state.list.result.map(id => {
        switch (entityType) {
            case 'leads':
                return entitySelectors.getLead(id, state);
            case 'messages':
                return entitySelectors.getMessage(id, state);
            default:
                throw new Error(`${entityType} is unsupported`);
        }
    });
}

the App component above would be somewhat like:

const App = () =>
    <div>
        <Switch>
            <Route path="/leads" render={(routeProps) => <List {...routeProps} type="leads" />} />
            <Route path="/messages" render={(routeProps) => <List {...routeProps} type="messages" />} />
        </Switch>
    </div>
;

So the issue occurs when:

1. We load the leads route, rendering a leads list, thus fetching leads type results. So at this point state.list.result would contains something like ["lead-1", "lead-2", "lead-3"]. The switch/case does the job and the list renders correctly.
2. Now we try to navigate to /messages which also renders a List, but with items of type messages.

You can see that we try to properly reinitialize the result by dispatching the actions.list.reinitialize() action. This would basically updates the state like the following in the reducer:

function reinitialize(state, action) {
    return {
        ...state,
        result: []
    };
}

But we get an error, because the mapStateToProps executes and at this point state.list.result still contains: ["lead-1", "lead-2", "lead-3"] and not [] so the List container would try to select items of type messages which are not yet loaded.

Shouldn't the state be up to date when mapStateToProps is called? The expected behavior would be that componentWillUnmount executes, the state updates to an empty result list and , only then, List container is rendered again with a call to mapStateToProps.

[markerikson]

I'm kind of confused on the sequencing you're trying to describe. Can you provide a CodeSandbox that demonstrates the issue, preferably with some logging that annotates the expected vs actual sequence of events?

[benjamindulau]

@markerikson Well my guess is that with react-router (and the withRouter HoC), a soon as we navigate to a different route, mapStateToProps is being called immediately before unmount, resulting in potentially trying to select data from the state that are not yet available.

See what I mean?

I'm not sure if that's a react-router issue, redux issue, or if it is even an issue at all :)

But this problem is constantly happening for us.

I can try to provide a CodeSandbox, but I'm not sure I can reproduce it easily...

[timdorr]

It's mainly an "issue" with React Router. I use quotes because it's really an expected behavior. React doesn't give us fine grained control over when other components in the tree mount or unmount. Nor would we really want that, because it sounds like a nightmare to control.

Instead, I would make the suggestion that relying on one component's unmount to "clean up" Redux state is probably going to be problematic in the long run. Tracking that is simple right now, but could easily become a huge problem if your app gets any more complex. More ideally, your components should be able to load up whatever they need independently of each other and Redux can serve as the central point for them to converge.

The most obvious thing you should do is get rid of the generic results state key and use specific keys for specific parts of your state. Trying to coordinate that across many independent components is going to be very difficult and likely not worth the effort. I would instead structure your state around specific types of data going into it, which will be easier to reason about both now and in the long run.

[benjamindulau]

@timdorr We did what you suggested at first. But as you know, we constantly need to find the right balance between productivity/maintainability. In this specific app, most of the screens are very similar(ish). We made that choice for productivity concerns and for being able to deliver according to the fixed roadmap.

It may not seem like a great productivity increment but in the long run it is. As we are able to create new screens really fast.

While this is not really an issue, it's yet kind of a pain in the ass if you ask me ;-)

Having said that, thanks for your feedbacks! We'll try to find another approach without sacrifying too much productivity.

[benjamindulau]

Ho and about "relying on one component's unmount to clean up Redux state" not being scalable.

It doesn't seem to be so problematic to me since we have a common list "container" (smart) being the entry point of every single list throughout the app. Using the componentWillUnmount function seems to be the right place to do proper clean up as far as the "list" state slice is concerned. We keep separation of concerns here. Our List component is only responsible of cleaning up the portion of the state it is dependent to.

At some point we need to do clean ups and I can't see another way than using the React components lifecycle for that. How you'd do it?

[markerikson]

In general, mapState is supposed to be called immediately. That's the point. Dispatching is synchronous, running mapState is sorta-mostly synchronous depending on where and when you dispatched. And, while we don't guarantee mapState will get called for every action (like if there's a bunch of actions dispatched right in a row, updates might get batched), you should be expecting that if you dispatch an action in cWU, it's going to result in mapState running right away.

[benjamindulau]

@markerikson In this case the problem is not that mapState is called right away when dispatching an action in componentWillUnmount (that is the expected behavior) but that it's being called immediately before componentWillUnmount is called and the action is dispatched after that even if the component doesn't need to render.

[markerikson]

@benjamindulau : uh... now I'm really confused.

mapState functions will only execute after an action has been dispatched. Therefore, some action is being dispatched to trigger that being run.

Please review the action log using the Redux DevTools Extension to see what action is being dispatched and figure things out from there. I highly suggest turning on the "action stack trace" feature to see the exact code path that is dispatching that action.

As I said earlier, a demo that shows the actual vs expected behavior would also help me see what's going on here, because the explanations aren't very clear to me atm.