Fix issues with stale props

[esamattis]

Fix for #86

The idea in this change is to delay executing mapStateToProps until the render call. The render call is forced for every state change by setting the store state to the component state for every connect wrapper. This works because state changes are batched in React. Which means that when the render and there for the mapStateToProps is executed the state and any props based on it are consistent across the full component tree. Which is awesome!

Only concern that I have with this is performance. That's why in the second commit I add a pure render wrapper component (PureWrap) which makes sure that the original component is only rendered when the mapStateToProps actually produces a change. Not sure whether this is better or worse than the original. But even if this is not that performant I'd prefer it because it avoids whole class of weird edge cases.

---

Anyone wanting to help and test this I pushed a compiled version to epeli/react-redux#fix86. Put that in your package.json as the react-redux version number.

[gnoff]

@epeli this brings us back to the way things were before #1 which as @gaearon says makes the worst case performance the default. I'm inclined to leave the existing behavior as is. If we did adopt this form I would hope that it be made optional and opt-in much like the 2nd arg props parameter to the first two connect arguments.

[esamattis]

this brings us back to the way things were before #1 which as @gaearon says makes the worst case performance the default

Not able apprehend right away every point made in #1 but I'd like hear why this would be the worst case compared to the current implementation because in it mapState is also executed always when the store changes. Also note the PureWrap optimization in this PR which prevents any useless renders in the wrapped components if the mapState does not produce any changes.

[gnoff]

I could be wrong because I haven't tried running it with your PR but I believe that this change will make even users of the mapStateToProps with a single arg call that selector on every re-render of the component, not just when state actually changes. It is true that if you use the props arg for mapStateToProps then this behavior already happens but is opt-in and was only added at the request of certain library users, it isn't intended to be the default mode. If your PR is merged then we adopt this more eager mode automatically (I think)

Is that not true?

[esamattis]

All tests now pass.

I believe that this change will make even users of the mapStateToProps with a single arg call that selector on every re-render of the component, not just when state actually changes

If I understand you correctly the test should not invoke mapState when props change if it only has one argument tests for that and it passes. Funny enough I had to change the invocationCount assert from 2 to 1 which would indicate that this does less work. Also that was not the only place I had to assert for less mapState invokes.

I will do some real world tests later today to see if I missed some regressions. I'd love see other test this as well!

I also added a new test which makes sure that the wrapped component does not re-render on every state change. Only when the mapState creates a new state.

[esamattis]

Wrote a failing test for the issue at hand and learnt something new about React. React only batches setState calls during event handlers and because this fix relies on the batching it means that the original issue can still occur when the store is updated from somewhere else. Any ideas how common that is? Could it be possible to somehow force React in to the batching mode when store updates?

In the other news I'm begining to be fairly confident about the performance here. Waiting for feedback.

[esamattis]

Could it be possible to somehow force React in to the batching mode when store updates?

To answer myself: Yes with React.unstable_batchedUpdates(cb). Previous discussion: reduxjs/redux#125

In that issue sebmarkbage mentions that React has a plan to move batching by default. Can you @sebmarkbage confirm that that's still the case?

If so then that issue will be resolved by itself with React update. Until then redux-batched-updates middleware can be used as a workaround.

---

Anyone wanting to help and test this I pushed a compiled version to epeli/react-redux#fix86. Put that in your package.json as the react-redux version number.

[gnoff]

@epeli took a better look and you I retract my earlier statements about regressing performance-wise. However regarding the lack of consistent batched updating is it true then that for this PR to not regress in any case one needs to use either the redux-batched-updates middleware or does the lack of that simply return us to the edge-case broken state that we are in with the reported issue #86 ?

[esamattis]

is it true then that for this PR to not regress in any case one needs to use either the redux-batched-updates middleware or does the lack of that simply return us to the edge-case broken state that we are in with the reported issue #86 ?

It simply returns to the edge-case broken state. So currently this PR just makes the edge-case less likely and goes completely away with the redux-batched-updates middleware.

Incorporating batched updates into Redux was discussed in reduxjs/redux#125 and it seems that the main reason not to do that was the fact that it was available only as a React addon. Could it be considered for react-redux as it's now available in the React object?

[esamattis]

as it's now available in the React object

I'll take this back. It was available only in the 0.14 beta. It has been moved to react-dom in 0.14 RC. I guess we don't want to depend on that because react-redux can be used with React Native and it makes no sense there.

Nevertheless I don't haven't seen / heard any downsides in merging this PR. Without it it is impossible to workaround the issue.

[gaearon]

Is it absolutely necessary to have the intermediate component here?

[esamattis]

Not absolutely necessary. I think the same optimization could be implemented manually in the connector component by invoking the mapState function in shouldComponentUpdate.

[gnoff]

My opinion is leave out the PureWrap and let that be a call site optimization. Lots of people connect pure render mixin'd components so I don't think it grants a whole lot in terms of benefit for the extra complexity of code (unwrap function for instance) plus the variable way the connected component would be represented in the component tree (normal connected components will now have 3 tiers intead of just 2 in the devtools explorer)

Neither of these downsides are huge but then neither is the upside so simplicity wins?

[gnoff]

I think the same optimization could be implemented manually in the connector component by invoking the mapState function in shouldComponentUpdate.

most mapState functions will be fast but i wonder if adding the extra call would be worth the mild improvement you get by avoiding some updates. I can imagine that this might improve the already decently fast cases but make the minority of slower cases even slower. just food for thought

[esamattis]

The devtools argument is really good argument against the PureWrap. I'll remove it on Monday.

Seems that the batching will become default in React at some point:https://mobile.twitter.com/sebmarkbage/status/642366976824864768?refsrc=email&cn=cmVwbHk%3D

[gaearon]

To clarify: I don't promise I'll merge this PR yet :-).
I'm a bit sick now and will review later when I get better.

I definitely want to avoid:

• any performance regressions from current implementation
• adding a component layer (we actually worked to get rid of it before)

[esamattis]

Currently I don't see any reason why both would not be avoidable. Perf should be OK already. I'm now busy for couple days but will work on this after that.

[esamattis]

I rewrote the entire pull request and force pushed it to this PR branch.

• The change is now much simpler. Should be easier to understand
• All existing tests are now untouched and passing
• Extra component layer (PureWrap) is now gone

As far as I understand the performance should be as good as in the master. Although because there is no performance test suite so I cannot be 100% sure.

A new compiled version is again available in epeli/react-redux#fix86 for easy testing in your apps.

[gaearon]

Good job! I've been trying to avoid setState() unless needed because, even with shouldComponentUpdate returning false, it's somewhat of a perf hit in a tight spot. However I agree correctness is more important than performance.

Do you have any idea how we can test perf regression, if any? Maybe we can put this into https://github.com/evancz/todomvc-perf-comparison and hope the benchmark isn't too skewed?

[esamattis]

Good job!

Thanks!

I've been trying to avoid setState() unless needed

Sadly it's exactly setState() (and batching) which makes the state consistent.

Do you have any idea how we can test perf regression, if any?

I think http://benchmarkjs.com/ by the Lo-Dash creator jdalton is the tool for the job. We've used it for underscore.string.

[gaearon]

Do you have the time capacity to try to benchmark before/after?

[esamattis]

So I created #104 for that. Just one simple test to see how store changes perform on a mounted component.

Results of three runs on the current master:

dispatch x 86.78 ops/sec ±0.95% (73 runs sampled)
dispatch batched x 135 ops/sec ±0.76% (77 runs sampled)

dispatch x 76.05 ops/sec ±2.02% (64 runs sampled)
dispatch batched x 114 ops/sec ±1.62% (70 runs sampled)

dispatch x 81.47 ops/sec ±2.01% (68 runs sampled)
dispatch batched x 126 ops/sec ±1.32% (78 runs sampled)

and with this PR

dispatch x 84.80 ops/sec ±1.83% (70 runs sampled)
dispatch batched x 128 ops/sec ±1.63% (77 runs sampled)

dispatch x 84.90 ops/sec ±2.11% (72 runs sampled)
dispatch batched x 132 ops/sec ±1.76% (75 runs sampled)

dispatch x 84.02 ops/sec ±1.72% (69 runs sampled)
dispatch batched x 123 ops/sec ±1.49% (75 runs sampled)

Not seeing any significant differences. But this is just a one test.

[esamattis]

Something still blocking this?

[gaearon]

Can you please write release notes and upgrade instructions for this change?
See Releases for examples.

From my understanding, this potentially can be a breaking change, so we'll jump to 3.0, but we need to tell people which exactly patterns would fail which worked before. And of course we need to explain which patterns, previously failing, would work now with this change.

I'll release it as 3.0.0-alpha today, but it will be labeled as react-redux@next on NPM until people give it some usage.

[gaearon]

Out as 3.0.0-alpha, let's wait for feedback.

[esamattis]

One correction to the release notes:

This is only relevant if you have nested connect()ed components

It's only relevant if you have nested connect()ed components _and you use the ownProps param of mapStateToProps in the nested ones_.

Also I would like to recommend usage of redux-batched-updates middleware until React starts defaulting to batching.

[gaearon]

I released 3.0.0 but I think I'm misrepresenting the change again.
Can you please look at the release notes: https://github.com/rackt/react-redux/releases/tag/v3.0.0

?

[esamattis]

The ownProps and props from the parent component are the same thing. So the current description doesn't make much sense. Cannot complain, this change is really hard to explain simply.

I would put it like this:

Now the map functions (mapStateToProps, mapDispatchToProps and mergeProps) are not called until React starts to render the connect()ed components. Previously the map functions where called immediately when store changed which could cause weird edge case bugs when the ownProps parameter was a derivative of the state. The state from which it was derivative of was a different version than what was passed as the stateparameter. In some cases the states can be incompatible with each other and cause very confusing bugs in user code.

Unfortunately the states stay consistent only when store dispatches are called in batches ie. from DOM handlers or manually from ReactDOM.unstable_batchedUpdates(fn). Luckily redux-batched-updates middleware can be used to force batching for all dispatches.

[esamattis]

Oh, a collaborator hat, thanks :) Updated it.

[gaearon]

Thanks!