Don't resubscribe if mapState changes

[ianobermiller]

Instead, use a ref to hold the last mapState value both for comparison and for access within the store callback.

[ianobermiller]

@sophiebits can you please take a look and see if I'm missing anything here?

[sophiebits]

Unfortunately this looks incorrect. You assign to mapStateRef in the render phase but read from it in the commit phase. That means if we start processing an update to mapState but it hasn’t committed yet, your check function will use the new mapState but it should use the old one.

[ianobermiller]

@sophiebits is it possible to write a test for this case? Sounds like the solution is to use useEffect to set the current value of the ref.

[ianobermiller]

Actually, does it matter if the check uses the new value of mapState? Wouldn't that imply that it already entered the condition on the last render (the one that didn't commit) and ran setDerivedState already, so that we'd already have the correct state?

[ianobermiller]

Ah, I get it now! Thanks for the explanation, @sophiebits. mapStateRef.current could be set to a new function that could potentially be incompatible with the existing one, and if the store calls the subscriber callback after the partial render the bad map state function could be used to call setState again.

I'm wondering though, if setState is being called, that will be another render cycle anyway, during which mapState will be updated anyway. So it seems like it shouldn't produce any incorrectness.

[leepowelldev]

I've been looking at this PR for over an hour. Maybe it's my misunderstanding of hooks - but I can't see the issue with using a ref in the way you have done. I'd love to understand why I'm interpreting it incorrectly though.

[sebmarkbage]

@ianobermiller If the new map function returns a value that happens to be the same as lastRenderedDerivedState.current, then it wouldn't rerender even if it would've if you had the correct one.

[ianobermiller]

@sebmarkbage if it is returning the same value as before, it is expected that the component doesn’t rerender, even if the mapState function changed. I’m still struggling to see the correctness issue here.

[sebmarkbage]

The new map function hasn’t happened yet though. That’s a future state. So in the current state’s map function it should rerender.

[leepowelldev]

Are you saying that the checkForUpdates function will now not be called after rendering if mapState changes - and will only be called when the store is updated?

[ianobermiller]

I think I finally understand this, here is my attempt to explain it.

With concurrent rendering, the render phase can happen without the commit phase, and then might happen again before it actually commits. For a functional component, render is just calling the component.

The blog post on the new lifecycle methods explains this well, calling it "async rendering". Though it doesn't talk about hooks, it makes the distinction between "render" lifecycle methods like componentWillReceiveProps and "commit" phase methods, like componentDidUpdate.

For hooks, the only ones that run during commit phase are the callbacks to useEffect and useLayoutEffect. What this means is that if we mutate a ref in render (or in the main body of the functional component), the component might not commit (which means your UI won't be updated). Then, render might be called again, and actually committed. So with this PR, the order of operations could be:

1. Render & commit once with mapState
2. Props change and mapState is a new function
3. Render only with mapState, storing mapStateRef.current = mapState and calling setDerivedState
4. React decides not to commit this render (say because the component is offscreen)
5. React renders the component again with the new mapState, but this time it is the same as the old one since we mutated the ref, and setDerivedState is not called
6. The component does not ultimately render with the new derived state

Now, what makes this implausible to me is that in step 3, not only does it set the ref, but it also calls setDerivedState -- are even state updates in render discarded if the component is not committed? I would expect that they'd always be honored, in which case during the next render at step 5, derivedState would correspond to the result of the new mapState function, and all is well.

[ianobermiller]

According to https://reactjs.org/docs/hooks-faq.html#how-do-i-implement-getderivedstatefromprops, I assumed the set state call at render time would cause the component to immediately rerender again.

[leepowelldev]

I Dan's latest blog post, he's using a ref to store the callback, but uses it inside a useEffect hook:
https://overreacted.io/making-setinterval-declarative-with-react-hooks/
It looks similar to what you are doing here. Might help explain things a little.

[ianobermiller]

Superseded by #40