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!

[jimbolla]

@jonkelling Would you be interested in helping me test my changes? At least by trying them in your app?

[jonkelling]

@jimbolla I'll do what I can! I was able to get it functioning, but the first go at it saw decreased performance. I may not be doing it the way you intended, so I can try it again a bit differently if this seems off.

connectAdvanced(() => (state, props, dispatch) => ({
active: state.on && state.activeId === props.id,
    id: props.id,
    children: props.children,
    onMouseOver: (id, left, top, width, height) => dispatch(toggleHighlight(id, true, left, top, width, height)),
    onMouseOut: id => dispatch(toggleHighlight(id, false))
}))(HighlightSomething);

A couple of things:

1. I had to add children: props.children to get things to render.
2. I think shouldComponentUpdate is always returning try because it's comparing all of the properties returned by the selector, including the onMouseOver and onMouseOut functions--I really only want it to update if "active" is different, in this case.
3. I love the intentions of reselect in general, but as I'm only after a single boolean property, caching would technically be less efficient, and would sort of defeat the purpose of what reselect does.

Let me know if I can help any other way!

[jimbolla]

@jonkelling Ah yes, if you're going to use connectAdvanced, you probably want to pre-build your event handlers at least, otherwise react sees those as new every time. It also doesn't assume you want to pass alll ownProps through by default, so you either have to cherry pick them, or use spread. You could write:

connectAdvanced(({ dispatch }) => {
  const onMouseOver = (id, left, top, width, height) => dispatch(toggleHighlight(id, true, left, top, width, height));
  const onMouseOut = id => dispatch(toggleHighlight(id, false));
  return (state, props) => ({
    ...props,
    active: state.on && state.activeId === props.id,
    onMouseOver,
    onMouseOut,
  })
})(HighlightSomething);

Note that with the spread operator (...props) approach, all props will be considered for equality check, so if your component gets other things passed to it that it's not interested in, that would still trigger a rerender. That would be motivation to cherry pick your props so that only the ones you're interested in.
You could also use the standard connect() API as well:

connect(
  (state, props) => ({
    active: state.on && state.activeId === props.id,
  }),
  (dispatch) => ({
    onMouseOver: (id, left, top, width, height) => dispatch(toggleHighlight(id, true, left, top, width, height)),
    onMouseOut: id => dispatch(toggleHighlight(id, false)),
  })
)(HighlightSomething);

And if the id param of your handlers is the same id from props, you could do this:

connect(
  (state, props) => ({
    active: state.on && state.activeId === props.id,
  }),
  (dispatch, props) => ({
    onMouseOver: (left, top, width, height) => dispatch(toggleHighlight(props.id, true, left, top, width, height)),
    onMouseOut: () => dispatch(toggleHighlight(props.id, false)),
  })
)(HighlightSomething);

I could go on about how to use reselect to build your selector as well, but I don't want to go to far off tangent right here. Feel free to submit an issue on my fork if you want to go in depth there.

[jimbolla]

I removed the dependency on reselect and handrolled memoization for the relevant parts. I'm using this project to perf test the difference between current and this rewrite. With 331 connected components, i'm currently seeing 5-7 FPS with current, and 50-53 FPS with the rewrite.

[markerikson]

Well, that's certainly a very intriguing result.

I very briefly skimmed the new code in the fork. There's definitely a lot more code overall, but on the flip side, a lot of that seems to be selectors. The individual bits of code also seem fairly well-written at first glance. Will need to spend time reading through that more carefully to see what's actually going on. (Could also use something of a diagram or dependency tree to tell me how all these different selectors fit together.)

Again, I'm certainly not the guy who makes the final call, just the one who happens to be actively responding to the issue. That said, at this point I'd say the proposal seems to have some potential benefits, and warrants serious consideration and discussion.

[jimbolla]

@markerikson I'm still refactoring for clarity, and need to add some comments, but I'm getting close to being ready to submit as a PR. Any feedback you have is greatly welcome.

[markerikson]

Haven't had a chance to really dig through the code yet. My main suggestion at this point would be to document this to ridiculous levels:

• Overall motivation and intended benefits
• High-level conceptual overview of the new approach
• Conceptual comparison with the existing approach
• Document any new APIs, their purpose, and their usage
• Walkthrough of the new data flow sequence
• Some kind of dependency tree-ish thing for all the new selector functions
• Benchmarks, benchmarks, benchmarks (numbers, graphs, and actual runnable examples)

I know that's asking for a good chunk of work, but given that you're proposing to swap out an entire existing implementation that's been reasonably battle-tested with a brand-new codebase, the burden of proof is basically on you to fully justify the switch and make sure that it's sufficiently maintainable in the future.

[jimbolla]

@markerikson I totally agree, and was already thinking all those things. It's just a matter of time. I'm hoping to have all that by some time this weekend, optimistically. The code is more-or-less complete, barring any renames to make things clearer. Next up is comments to clarify things at the local level, where needed. Then I'll work on the "conceptual" documentation. Do you think it makes sense to write that as an .md file to be included with the code?

[aweary]

Could you possibly write up the documentation in a gist and just link it here? If it's beneficial to include it in the docs I'm sure it could be pulled in.

[jimbolla]

If I wanted to make a diagram to show the relationship between the various functions, what would be a good tool to do so?

[markerikson]

Not sure on the diagram thing.

This FPS measurement utility might be useful: https://www.reddit.com/r/reactjs/comments/4os0lu/looking_for_a_reliable_way_to_measure_framerate/

[jimbolla]

The one used by react-redux-perf is this one. It seems to do a good job. At this point I'm feeling pretty strong about significant perf gains, by avoiding unnecessary calls to setState/render. I'll elaborate in the eventual gist.

[jonkelling]

I'm with @jimbolla on avoiding unnecessary calls to setState and render. Might I also suggest calling out the specific cases and adding unit tests for them. My own change to avoid extra calls to setState breaks one of existing tests in react-redux. When I get the time, I hope to understand the intention behind that test case and the original code so I can decide how I really want to/should handle it. I look forward to seeing what you come up with!

[markerikson]

@jonkelling : yeah, the snippet you commented out is to specifically handle the case where a parent component is rendering a connected child component, and passing some props in. For example, the parent might render something like:

return <SomeConnectedChild itemID={someID} />

If the child's mapState indicates that it takes the second argument of ownProps, then the return value of mapState is probably dependent on the incoming parent props, usually along the lines of:

const mapState = (state) => state.someData[ownProps.itemID]

In that case, every time the parent re-renders, the connected child would need to re-run mapState in case the incoming props changed, even if the state itself hasn't changed.

And yes, MOAR UNIT TESTS!!!! :)

[markerikson]

Hmm. More use of context? Do we need to be concerned with any of the "context doesn't deal well with shouldComponentUpdate" comments I've seen? (Not clear on all the details myself, just know that it's one of the general concerns with use of context.)

[markerikson]

Also, does this make any changes to the approach for testing connected components? Does it still handle looking for this.context.store or this.props.store?

[jimbolla]

@markerikson I never modify the context so no need to trigger an update. That should be a non issue. I followed the same "props.store" or "context.store" pattern like the original.

[markerikson]

Cool. Just tossing that out as a thought to be aware of.

[jimbolla]

It's all good. The more scrutiny the better. Believe me, I'm more worried that I missed something than you are. haha.

Current benchmarks using jimbolla/react-redux-perf, all 6 tabs opens at once, 331 connected components each:

Frames per second:

FPS	current	rewrite
Chrome	5	46
Firefox	2	45
IE 11	3	21

Milliseconds to render a frame:

MS	current	rewrite
Chrome	180	20
Firefox	400	23
IE 11	320	40

Same tests, this time with a Blocker component between the parent and children that always returns false for shouldComponentUpdate:

Frames per second:

FPS	current	rewrite
Chrome	5	51
Firefox	2	45
IE 11	3	25

Milliseconds to render a frame:

MS	current	rewrite
Chrome	190	19
Firefox	500	20
IE 11	380	37

Fascinatingly, current did worse with the blocker component, while the rewrite does even better. I think this is because in the rewrite, my current bottleneck is the shallowEqual compare on ownProps. With the Blocker component, the old props === new props and avoids the complicated part of shallowEqual.

[jimbolla]

How do you all feel about using jsdoc for commenting code? Or is there something newer/better?

[markerikson]

Don't think there's any specific tool used for code docs at the moment in any of the ReactJS repos. I'd say go ahead for now, and worst case someone might change it later (but I'd think that would probably be okay as-is).

Changing topics: I'd like to pre-emptively thank you for the time and effort you've put into this, and the attitude and approach you've taken. When I saw your first issue about the mapDispatch+state question, I felt like you were really pushing a niche use case and not making use of the available option (mergeProps). When you said you were implementing your own store connection function, I kinda mentally groaned and thought, "Oh, chalk up another entry for the "Variations" page in my addons catalog", which is where I list libraries that seem to be going their own way or going against the idiomatic concepts of Redux.

But: when the suggestion of making your implementation API-compatible with connect() came up, you went for it. You've answered every question I've come up with so far. You've done the legwork to prove that there are indeed some very potential benefits from the new implementation. You've engaged pretty freely in discussion in the issues list.

Now, as I said earlier, as the guy proposing we take on a big chunk of code you've written to replace an existing implementation, the burden of proof and work is indeed on you. But, you've definitely won me over on the basis of your approach and your code.

Again, I don't have the final say on whether or not this would finally get merged in, but at this point I'm certainly in favor of a very serious discussion on this approach at a minimum, and think it has a good shot at being accepted overall.

[gnoff]

@jimbolla

having gone through your implementation of connectAdvanced (and to some extend the connect api reimplementation) I have the following very disjointed thoughts and questions.

1. in many respects connectAdvanced seems simpler than connect to me. would you intend to ever use the original api over the advanced api. If the benefits of the new api consistently outweigh those of the existing one I wonder if this framing is the correct one long term.
2. Should connectAdvanced have sensible defaults for when say you just want dispatch? Obviously one can use the connect wrapper for advanced but this adds a lot of internal complexity (for the backwards compat sake which is great) and if users are using the advanced api already there may be some value in providing defaults
3. using options to control depends on state feels clunky to me. Can the selector arity be used to modulate this behavior whereby (state, props, dispatch) => depends on state and (props, dispatch) => does not? scratch that, babel transpilation can alter fn arity when using default values. explicit is fine if a bit clunky
4. would the perf boost from enforcing subscription updates top down make the performance parity more equal? I think as great as the new implementation is, I'm worried that the parent subscription tracking feature is doing the heavy lifting perf wise and that a similar implementation in the current form may show possibly even higher performance than your new implementation (not that I hope it does one way or the other)

I'm torn because the risk of major reimplementations is real but the simplicity of the new implementation is valuable too. Part of me feels like if the new implementation is worth adopting we ought to deprecate the existing api in a major and move over to the new api in another major. In my mind most of the complexity in the new implementation is in trying to keep backwards compatibility which is a valuable target but if the new API turns out to be genuinely better (which we would of course need plenty of time in production use to adequately gauge this) then why hide it behind a veil of 'advanced-users-only' connotations.

I don't make decisions, just been around this project for a bit. take my opinions for what little they are worth :)

[jonkelling]

@jimbolla, I've got your latest code. Kudos on making it work with connect()! My own performance problems resolved 👍 I'll work on test cases for ya this weekend. Thanks for all of your work on this!

[jimbolla]

@markerikson Thank you for your all your help. Your participation has helped me flesh out my ideas and force me to critically think about what I'm trying to do and how to do it. I understand the potential impact of such a large change, and therefore the need to make a strong justification for it. I'm working on that "justification" doc now, and even as I'm writing it, I'm tweaking my design to make the explanation less awkward. I'm still hoping to get it done by this weekend.

@gnoff connectAdvanced appears simpler than connect because it does less for you. But the responsibility to memoize intermediate results is now on the caller. This is especially important for any actionCreators being bound to dispatch, for example. I don't want to make assumptions about what "sensible defaults" are for anyone's usage; instead I'd like an API that's flexible enough so that anyone can wrap connectAdvanced in their own method that makes assumptions that work for them. Using function arity is somewhat brittle and is not obvious. I'm sure if the current implementation of connect was modified to use hierarchical subscriptions, it could gain a lot of the same perf improvements. If someone else wants to take that approach, they're more than welcome to copy any of my code/ideas. But to me the performance is just a bonus, because what I really wanted was a more flexible API. Sepcifically, I'm interested in defining my selector using Reselect's createStructuredSelector function. I still think the connect API should be the official API for 99% of use cases, and then connectAdvanced is there when you want to get clever, eek out performance, or want to use it to integrate with another library.

@jonkelling That's great. I'm encouraged by your positive feedback. Test cases are certainly welcome.

[migueloller]

I was about to write an RFC issue to discuss rewriting connect() to provide more functionality. Given how healthy this discussion has been I think this is the best place to post it.

My approach was going to be a bit different than @jimbolla's. My inspiration came from wanting to write a lightweight version of apollo-client, call it react-redux-graphql, and to avoid having to do something like this:

import { connect as connect1 } from 'react-redux';
import { connect as connect2 } from 'react-redux-graphql';
import MyComponent from './components/MyComponent';

/* ...define props maps... */

const container = connect1(mapStateToProps, mapDispatchToProps)(MyComponent);

export default connect2(mapQueriesToProps, mapMutationsToProps)(container);

Ideally it would be something like this: (inspired by redux store enhancers)

export default connect(
  mapStateToProps,
  mapDispatchToProps,
  mapQueriesToProps,
  mapMutationsToProps,
  connectEnhancer,
)(MyComponent);

Or something similar (this doesn't have to be the final signature).

Now, I could just use another higher order component and do:

export default connectEnhancer(
  mapStateToProps,
  mapDispatchToProps,
  mapQueriesToProps,
  mapMutationsToProps,
)(connect)(MyComponent);

You can basically do anything you want with higher order components but I thought that maybe baking that functionality into connect() out of the box would leave room for anybody to write componentEnhancers, be consistent with Redux, and bend connect()'s functionality to do whatever you want, more so than advancedConnect().

So should we leave it for other developers to create higher order components or should we bake in functionality to enhance connect() out of the box. Thoughts?

[markerikson]

At the moment I'd say that that would be going too far to be an acceptable set of changes. The current PR is being looked at because it's been made API-compatible with the existing version of connect(), and Dan actually asked that the underlying "advanced" function in the new implementation not be exposed for now. Adding anything GraphQL specific would certainly be out of the picture for now. Adding additional extension hooks may be considerable down the road, but not likely to happen soon.

I'd say HoCs are your best bet for now.

[migueloller]

I apologize if I wasn't clear but I wasn't suggesting that we add GraphQL specific functionality, but more so adding the ability to add an extra argument to connect() that would allow other developers to extend connect()'s functionality. This wouldn't be a breaking change since it would be added either as an option or as the last argument.

It would also allow for advancedConnect() to be written separately (maybe as a separate package) and passed in as an option without having to completely rewrite connect() as it is now. This change would be 100% backwards-compatible.

People that want to opt-in to this new argument would write:

connect(mapStateToProps, mapDispatchToProps, mergeProps, { connectEnhancer });

or leave the option out if they want to.

This would allow advancedConnect() to be used like so:

connect(mapStateToProps, mapDispatchToProps, mergeProps, { connectEnhancer: advancedConnect });

The change in react-redux code would be minimal to allow for this, it would be similar to the change in Redux that introduced store enhancers.

Correct me if I'm wrong, but the best option would be to minimally change the current code instead of an entire rewrite? Doing something like this would require minimal source code change but allow complete customization.

EDIT: I'm going to do a quick implementation to show what I mean and will post here.

[markerikson]

Yeah, that's my point. We've already got several overloads in React Redux's API (like being able to pass an object full of functions as the mapDispatch argument), and based on the comments Dan has made already, I don't foresee any more specific API changes happening any time soon. And, if you want an example of how a "small" change can have some pretty unforeseen consequences and incompatibilities, read through reduxjs/redux#1813.

Like I said, the only reason #416 is being considered right now is that despite the major internal changes, the external API is staying the same, and the benchmarks are indicating some pretty major potential performance improvements. Also note Dan's comments at #416 (comment) and reduxjs/redux#1813 (comment) regarding new/different APIs.

[migueloller]

Sounds good. And thanks for the prompt replies @markerikson!

Here's the change migueloller@1c8d940

All tests are passing. Just wanted to get some comments on what people thought.

The change now allows for enhancing connect (if you want to) in any way you want, allowing for what I mentioned above.

If this ends up being ignored in the end that's ok. Just thought that this would be a good improvement. 😁

[migueloller]

I forgot to mention that the arguments of connect() aren't changing, the changes just allow for a new option to be passed in. This should make it compatible with anything in the ecosystem.

[jimbolla]

@migueloller If #416 gets accepted, then you will probably be able to create your desired API by wrapping the new connectAdvanced method with your own custom connect method, without needing to modify the standard connect. That's what I'm trying to accomplish in my own project... I have my own connectToStore method that calls connectAdvanced while providing a significantly different API than connect.

[migueloller]

@jimbolla That would be an option but it's not much different from wrapping connect because the library would still have react-redux as a dependency. It would be nice to simply export a function that takes in connect as a parameter and outputs a new version of connect. This is how store enhancers work (Redux libraries don't normally have Redux as a dependency).

It doesn't look like there is interest to mirror store enhancer functionality for react-redux, though.

EDIT: It's probably worth mentioning that all the benefits that you get from advancedConnect can be achieved with my implementation simply by doing:

connect(/* advancedConnect arguments */, { enhancer: advancedConnect })(SomeComponent)

[jimbolla]

I'm gonna close this now that #416 exists.