@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.

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 theworst case compared to the current implementation because in itmapState 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 themapState does not produce any changes.

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 themapStateToProps 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 formapStateToProps 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?

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 testshould not invoke mapState when props change if it only has one argument tests for that and it passes. Funny enough I had to change theinvocationCount 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 anew test which makes sure that the wrapped component does not re-render on every state change. Only when the mapState creates a new state.

Wrote afailing 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.

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

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

In that issue sebmarkbagementions 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 thenredux-batched-updates middleware can be used as a workaround.

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

@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 ?

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 inreduxjs/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?

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.

Is it absolutely necessary to have the intermediate component here?

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

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?

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

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

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)

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.

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 inepeli/react-redux#fix86 for easy testing in your apps.

Good job! I've been trying to avoidsetState() unless needed because, even withshouldComponentUpdate returningfalse, 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 intohttps://github.com/evancz/todomvc-perf-comparison and hope the benchmark isn't too skewed?

Good job!

Thanks!

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

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

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

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

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

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.

Something still blocking this?

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

From my understanding, thispotentially can be a breaking change, so we'll jump to 3.0, but we need to tell peoplewhich 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 as3.0.0-alpha today, but it will be labeled asreact-redux@next on NPM until people give it some usage.

Out as3.0.0-alpha, let's wait for feedback.

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 theownProps param ofmapStateToProps in the nested ones_.

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

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

?

TheownProps 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 andmergeProps) are not called until React starts to render theconnect()ed components. Previously the map functions where called immediately when store changed which could cause weird edge case bugs when theownProps parameter was a derivative of the state. The state from which it was derivative of was a different version than what was passed as thestateparameter. 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 fromReactDOM.unstable_batchedUpdates(fn). Luckilyredux-batched-updates middleware can be used to force batching for all dispatches.

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

Thanks!