Completely rewrite connect() to offer advanced API, separate concerns, and (probably) resolve a lot of those pesky edge cases

[jimbolla]

I rewrote connect() and extracted a connectAdvanced() It now uses reselect to handle merging/caching of the state/props/dispatch. It also extracts out a connectAdvanced() function that connect() just delegates to.

I'm not quite ready to ready to submit a PR (still tweaking), but I'd like to suggest that this eventually become the new implementation.

It passes tests and linting, with the only breaking change to the existing API being related to the "advanced scenarios" for factories that require setting another options flag. (I haven't figured out how to detect the factory methods automatically... will give it another go, but it's been tricky given how radically different my approach is.) solved

Things I'm still need to do:

• support factorying map*ToProps functions without an explcit flag
• add more comments
• performance testing. the tests run slightly faster on my machine but we're talking insignificant amounts (250ms vs 230ms). do performance tests exist for this?
• reconcile with pending pull requests, as there would be zero chance of a successful merge. I've already covered the changes and new features in several of them, but some of the others will need reimplemented.
• keep tinkering. there's some functionality in connect() that I'd like to make reusable for someone who wants extend its functionality in userspace

[markerikson]

The first concern would probably be that you're explicitly depending on Reselect. While Reselect is heavily used in the Redux ecosystem, I'd be hesitant to make it a direct dependency. (Of course, I'm also just an opinion, hardly the final arbiter.)

[jimbolla]

What would be the cause for hesitation?

[btipling]

The connect code is a bit hard to work with in tests. It would be really nice to be able to set withRef to true just for tests, but there's so much hidden inside that connect code, not accessible outside of it. Even the Connect component itself is defined inside the connect function. That's just crazy to me, why was it written that way? It's taking important bits of information from the closure it's in and making it impossible to extend that Connect component or do anything with it to make testing easier.

So I think a bit of a refactor there would be nice but your own advancedConnect has this problem too. So I'm not sure this is the way to do it.

[jimbolla]

@btipling I'm all for opening it up as much as possible. But I'm not sure what you're not able to access that you expect to. The Connect component has to be inside the closure function because it depends on the functions parameters. It can't be outside because each call to connect creates a unique class.

[markerikson]

@reactjs/redux, @Phoenixmatrix, @aweary: would be particularly interested in other thoughts and opinions on this, especially from people who have actually been hands-on with connect().

I wrote up my initial thoughts in Reactiflux (chat transcript at https://gist.github.com/markerikson/561ce2d8830a34c35701ea77564c7073). Way TL;DR:

at this point I would probably be against unless there's some specific demonstrated improvements in speed, maintainability, or use cases. And, given that I'm just "an opinion", I'd really like to hear from others who have actually worked on the code and know what they're doing with it (Dan in particular)

But:

full API compat / all tests passing is certainly a good start

[aweary]

Do you have any documentation on the new advanced API and/or a changelog? It's a pretty big change and kind of hard to parse everything at once, it will likely be easier to provide feedback once we can see an actual diff when you open your PR.

I'm also a little hesitant about using Reselect by default. What if someone wanted to implement their own caching/memoization strategy, would they be able to do that?

[timdorr]

Here's your diff, but it's basically all new.

I'm not sure this is actually more performant too. connect() currently does some intelligent caching of the ReactElement instance when comparing updated state. It looks like this will create more spurious updates, moving the caching over to the selector, but potentially falling through more often due to different semantics.

[markerikson]

Prior discussion for reference: #403 , #405

[markerikson]

Also, repos that could be relevant benchmarks:

• https://github.com/mweststrate/redux-todomvc
• https://github.com/jscriptcoder/stressing-redux
• https://github.com/broadsw0rd/react-redux-perf

[aweary]

It definitely depends on whether this brings any real-world advantages. connect() is already performant and the current implementation has the advantage of being well tested in production, so any performance/usability improvements will have to be more than marginal to justify a full rewrite.

[jimbolla]

@aweary I've been working on adding comments to the code to explain as much as possible, but I can provide an executive overview:

• The API for connect() is exactly the same, but its internals are completely different, ultimately delegating to connectAdvanced().
• The API for connectAdvanced() is connectAdvanced(selectorFactory, {connectOptions}). selectorFactory is a func with the signature ({factoryOptions}) => (state, props, dispatch) => {finalProps})
• While Reselect is utilized internally, there's no requirement for a user to use it as their memoization engine for the functions they pass to either connect or connectAdvanced, nor is memoization even a requirement.
• Exposing an advanced API will allow end users to customize the connect functionality by building their own wrappers. For example, the whole reason I started this was so that I could define a wrapper that let me define my mapping using reselect's createStructuredSelector. Another example would be [RFC] Add shorthand syntax for mapStateToProps #323.

Looking at a diff probably won't be super useful, since it's a total rewrite. It's probably better to look at the new files as a whole.

@timdorr Can you elaborate on why you think this implementation would fall through more often? Unless I'm missing something, I expect render to NEVER execute unnecessarily.

[jimbolla]

First performance test using https://github.com/broadsw0rd/react-redux-perf.

1. First I modified the fill-pairs action to increase the count from 60 to 600. (Otherwise both were capping out at 60fps)
2. Then built using current react-redux
3. Then copied the built version of my rewrite into the perf's node_modules and rebuilt

Running each version in their own Chrome window simultaneously on their own monitor, full screen
(Ultra-HD).

Stat	Current	Rewrite
Stats FPS	33-35	35-38
Stats Memory	13MB	11MB
Chrome CPU	8-9%	8-9%
Chrome Memory	99,000KB	63,000KB

So for this particular test, my rewrite is getting a few FPS better, and is using slightly less RAM. But this test is fairly simple. It's a single connected component that passes props to many child components. I'm gonna tinker with the code and see if I can make it run 100s of connected components and see what kind of stats I get.

I haven't been able to get https://github.com/jscriptcoder/stressing-redux to work. It just throws an error on startup. (With official react-redux... didn't even make it to trying my own)

[markerikson]

The examples in http://somebody32.github.io/high-performance-redux/ would also be very relevant here. Code for the examples is at https://github.com/somebody32/high-performance-redux/tree/gh-pages/assets, i think.

[jimbolla]

Latest test... changed https://github.com/broadsw0rd/react-redux-perf. so that each item was a connected component (181 total connected). First results were that my rewrite was getting about 1-2 FPS less, so looking into it... excess setState() calls. I solved this by recomputing the selector and conditionally firing setState only if changes. This led to a problem with the backwards order of parent-child subscriptions. I changed this so that parent components always subscribe before children. This probably needs further reviewed, BUT the payoff....

Stat	Current	Rewrite
FPS	14-15	47-49

[jonkelling]

I made a small change in handleChange that fixes my own performance issues. The key being to avoid excess setState(...) calls, which @jimbolla mentioned.

if (pure) { // removed && !this.doStatePropsDependOnOwnProps)
    var haveStatePropsChanged = tryCatch(this.updateStatePropsIfNeeded, this);

I'm not aware of the repercussions of this change, as I just start messing with the code here, well, 30 mins ago. I just started using React, Redux, etc. etc. this week, so I'm not quite up-to-speed on the performance problems here, but it didn't take long to run into them. Glad I'm not alone!

Anyway, I'm happy to see that this thread is pretty active, so I wanted to take the opportunity to post my 2 cents and see what others thought before I spend the rest of the weekend catching up on this particular issue. Thanks, all!