Use componentWillMount instead of componentDidMount for subscriptions

[jdeal]

Even if this PR doesn't get accepted, I'm hoping to find out the reasoning behind the current implementation and find out what patterns to use to avoid the problem I found. The problem being...

Because component store subscriptions are created in componentDidMount (which fires child before parent), you can get into situations like this:

1. Parents make a decision about what components to render, based on store state.
2. Children get current store state which is fine.
3. Store state is changed, requiring a new component tree.
4. Children get new store state before the parents, which is bad, because the new state may not make any sense for the new components.
5. Parents get new store state and switch out components. But it's too late, because children have already received bad state.

Here's a simple example showing the problem:

https://jsfiddle.net/justindeal/myqyxpcw/6/

There's a value in the store 'a' or 'b' which is used to switch components A and B. I have some side effect rendering to show what values are actually rendered by A and B. You can see that A incorrectly gets a 'b' value on the first toggle.

By switching to componentWillMount, subscriptions are always in parent-child order, so there's never any inconsistent state.

If you want to consider a simple real-world case, think about the filter string from the canonical todo example. Now consider that you can switch between todos and recipes. The filter for todos may make no sense for filtering recipes. So sending the global filter for todos to the child recipe components makes no sense and can cause errors to be thrown if unexpected props/values show up.

This change had one side effect for existing tests: because we haven't yet rendered when subscribing, the impure case avoids a duplicate initial call to its state mapping function. As far as I can tell, this has no effect on what's actually rendered though.

I also saw no negative outcome when I made the change for my app. (Just a fix to the surprising behavior I saw.) But I'm curious to get some feedback.

Thanks!

[gaearon]

Please see the discussion in #292.

We can’t use componentWillMount because it runs on the server, which would result in memory leaks. The solution here is to batch updates but React doesn’t yet provide a stable API to do this.

Check out #293 for a possible solution. In the meantime you can implement this in userland.

[jdeal]

Ah, sorry I missed the open issue. Thanks for the heads up.

FWIW, I don't think userland really works in the case of trying to build a redux-compatible lib that provides state for multiple components. The lib would have to caveat its usage. (Of course, with this problem, usage has to be caveated anyway.)

I'll keep an eye on #292 and #293.

[gaearon]

Such library could wrap its dispatches into ReactDOM.unstable_batchedUpdates(), could it not?

[jdeal]

I guess my concern was:

The library would have to do all dispatching. It can't export a raw action creator and let the consumer dispatch that. At least not without the consumer knowing that it also has to wrap those dispatches with unstable_batchedUpdates. Essentially, an implementation detail (about inconsistent state updating) is leaking out to the consumer.

Having said that, I think it's pretty workable for now. unstable_batchedUpdates seems to cure the problem, and it's pretty easy to slide in. It's more or less an aesthetically odd thing to say "caveat: use this unstable API for these edge cases."

BTW, still a big fan of Redux! Just picking this nit!

[gaearon]

It's more or less an aesthetically odd thing to say "caveat: use this unstable API for these edge cases."

Yeah but it’s the best we have until React either enables batching by default or provides a stable API for this. :-)