Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork3.3k
Rewrite connect() for better performance and extensibility#416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…select'. (6 failing tests to go)
…ops pass. BREAKING CHANGE: requires setting an option parameter mapStateIsFactory/mapDispatchIsFactory to signal factory methods.
…hind-the-scenes behavior of new implementation.
…omponents always subscribe before child components, so that parents always update before children.
…ead of subscribing to the store directly they subscribe via their ancestor connected component. This ensures the ancestor gets to update before its children.
…st compare last rendered props to new selected props
chrisvasz commentedSep 28, 2016
Love this! I just dropped the beta into an existing application that uses Redux to store form information. In my application, each field is connected to the store, similar to With the new |
Mike-Sinistralis commentedSep 30, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Dropped this into the same application I tested earlier. This test was performed on mock data, so the data is slightly different but the structure, relationship, etc of the data are all the same across both. The test was a drag and drop implementation using lists and smart components. The difference with this change is MASSIVE in terms of render count. (This is on Chrome, which seems to be the browser best able to handle this scenario. Mobile fares much, much worse) |
Hi, This is good news, but can someone explain the performance improvements? I'm not sure to understand why it performs better than before as a drop-in replacement |
@jimbolla can explain further, but it's a combination of several factors. Primarily, there's a ton of custom memoization going on using selectors, which avoids the need to call |
@slorber@markerikson The short short version... Forcing the order of components' store subscriptions to trigger top-down allows connect to avoid extra calls to setState, render, and mapStateToProps'n'friends. |
Mike-Sinistralis commentedOct 1, 2016
Doesn't that mean it also fixes the issue where can you end up getting components in in-determinant states because they render before their parent, which could result in a child trying to access non-existent data because they haven't been unmounted yet? I've actually run into this problem, and tested to see if the issue still occurs with v5 and it does not, so another issue fixed? |
Y'know, that's a good point. I've seen that in my own prototype app, where I frequently use connected parents rendering connected list items. I bet there's a good chance v5 resolves that. |
Yep. That was actually the motivation for making the change. The perf was a nice surprise. |
slorber commentedOct 3, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
That's nice! so finally it somehow breaks compatibility (not in a way that's likely to break someone's app) and the new behavior makes connect more performant, in addition to being more flexible. However@jimbolla can you take a look at this usecase of a list of 100k connected elements?http://stackoverflow.com/a/34788570/82609 |
@slorber A few thoughts on that:
|
jide commentedOct 5, 2016
@jide Looks like we're calling |
jide commentedOct 5, 2016
Ok thanx ! |
OK, pushed beta.3 to remove that check for now. |
On deleting comments, the app might want to re-render already removedPostComment before updating PostComments (which would prevent thatby removing that comment ID from the list in the first place).Actually, it will be fixed by migrating to react-redux@5, since thatversion will be forcing the order of components' store subscriptions totrigger top-down (seereduxjs/react-redux#416 (comment)plus next three comments). However, we cannot just sit and wait forstable react-redux@5, right?N.B. It's part of [components-rewiring], since the issue with racecondition was introduced by PostComment rewiring (connecting directlyto the store) in one of previous changes.
| returnfalse | ||
| } | ||
| for(letkeyinb){ | ||
| if(hasOwn.call(b,key))countB++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This could exit before completely iterating through all theb keys by checking ifcountA is less thancountB in each iteration:
for(letkeyinb){if(hasOwn.call(b,key)&&countA<++countB)returnfalse}returntrue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A PR to change that out would be appreciated!
natevw commentedJan 27, 2017
Looks like this has been released. Updating to 5.0.4 fixed some seemingly-bizarre bugs (caused by a previously-incorrect assumption that props were reliably passed from the top down), and haven't noticed any regressions. Thanks for making this happen! |

Uh oh!
There was an error while loading.Please reload this page.
Update: Released as alpha!
See below. You can now install this as
react-redux@next:Please test it out!
TL;DR
Rewrote
connect, same basic API plus advanced options/API, all tests pass, roughly 8x faster, more modular/extensible designOverview
I rewrote
connectinto modular pieces because I wanted to be able to extend with custom behavior in my own projects. Now connect is a facade aroundconnectAdvanced, by passing it a compatibleselectorFactoryfunction.I also was able to greatly improve performance by changing the store subscriptions to execute top-down to work with React's natural flow of updates; component instances lower in the tree always get updated after those above them, avoiding unnecessary re-renders.
Design/Architecture
I split the original
connectinto many functions+files to compartmentalize concepts for better readability and extensibility. The important pieces:connectAdvanced.js: the HOC that connects to the store and determines when to re-renderProvider.js: (hasn't changed)connect.js: composes the other functions into a fully-compatible API, by creating a selectorFactory and options object to pass toconnectAdvanced.that performs memoiztion and detects if the first run returns another function, indicating a factory
mapDispatchToProps.js: used to create a selector factory from themapDispatchToPropsparameter, to be passed toselectorFactory.jsasinitMapDispatchToProps. Detects whethermapDispatchToPropsis missing, an object, or a functionmapStateToProps.js: used to create a selector factory from themapStateToPropsparameter, to be passed toselectorFactory.jsasinitMapStateToProps. Detects whethermapStateToPropsis missing or a functionmergeProps.js: used to create a selector factory from themergePropsparameter, to be passed toselectorFactory.jsasinitMergeProps. Detects whethermergePropsis missing or a function.selectorFactory.js: givendispatch,pure,initMapStateToProps,initMapDispatchToProps, andinitMergeProps, creates aconnectAdvanced-compatible selectorwrapMapToProps.js: helper functions for wrapping values ofmapStateToPropsandmapDispatchToPropsin compatible selector factoriesSubscription.js: encapsulates the hierachial subscription concept. used byconnectAdvanced.jsto pass a parent's store Subscription to its children via contextverifyPlainObject.js: used to show a warning ifmapStateToProps,mapDispatchToProps, ormergePropsreturns something other than a plain objectfile graph
The modular structure of all the functions in
connect/should allow greater reuse for anyone that wants to create their ownconnectvariant. For example, one could create a variant that handles whenmapStateToPropsis an object by using reselect's createStructuredSelector:customConnect.js:
ExampleComponent.js
And for scenarios where connect's three-function API is too constrictive, one can directly call, or build a wrapper around,
connectAdvancedwhere they have full control over turningstate+props+dispatchinto a props object.Performance
I'm using amodified version of react-redux-perf to performance test+profile the changes. It's configured to try to fire up to 200 actions per second (but becomes CPU bound), with 301 connected components. There are 2 scenarios being tested:
shouldComponentUpdate.I measured the milliseconds needed to render a frame using thestats.js used by react-redux-perf:
On the conservitive end, the rewrite is about 8x faster under these circumstances, with Firefox even doubling that improvement. Much of the perf gains are attributed to avoiding calls to
setState()after a store update unless a re-render is necessary.In order to make that work with nested connected components, store subscriptions were changed from "sideways" to top-down; parent components always update before their child components. Connected components detected whether they are nested by looking for an object of type
Subscriptionin the Reactcontextwith the keystoreSubscription. This allowsSubscriptionobjects build into a composite pattern.After that I've used Chrome and Firefox's profilers to watch for functions that could be optimized. At this point, the most expensive method is
shallowEqual, accounting for 4% and 1.5% CPU in Chrome and Firefox, respectively.connectAdvanced(selectorFactory, options) API
In addition to the changed related to performance, the other key change is an additional API for
connectAdvanced().connectAdvancedis now the base forconnectbut is less opinionated about how to combinestate,props, anddispatch. It makes no assumptions about defaults or intermediate memoization of results, and leaves those concerns up to the caller. It does memoize the inbound and outbound props objects. A full signature forconnectAdvancedwith itsselectorFactorywould look like:A simple usage may look like:
An example using
reselectto create a bound actionCreator with a prop partially bound:An example doing custom memoization with actionCreator with a prop partially bound:
Note these are meant as examples and not necessarily "best practices."
Pros/cons
I understand there is great risk to accepting such drastic changes, that would have to be justified with significant benefits. I'll reiterate the two main benefits I believe these changes offer:
Despite passing all the automated tests as well as week of manual testing, there is risk of impacting users dependent on implicit behavior, or that performance is worse in some unexpected circumstances. To minimize risk of impacting end users and downstream library authors, I think it would be wise to pull these changes into a "next" branch and first release an alpha package. This would give early adopters a chance to test it and provide feedback
Thanks
I'd like to thank the other github users who have so far offered feedback on these changes in#407, especially@markerikson who has gone above and beyond.