RFC: I've created a variant of connect() that is a tailored for using with reselect

[jimbolla]

I wasn't quite happy with the API for connect() so I went about recreating my own. A few design goals:

• Simpler API. Replace the 3 functions with 1, by having access to state, props, and dispatch all at once.
• Make the API reselect friendly, and use it to cache results
• Be able to partially apply state/props as args of an action creator

An example of starting with no reselect selectors and building up from there:

Version 1: this works, but the onLoad arrow function will be recreated everytime props/state change:

  connectToStore(() => (state, props, dispatch) => ({
    Branding: customization.signIn[state.account.signIn.customSignIn] || MainBranding,
    loaded: state.account.signIn.loaded,
    onLoad: () => dispatch(actionCreators.getSignInConfig(props.location)),
  }))(SignIn);

Version 2: uses reselect's createSelector to solve the function issue, but gets verbose:

  connectToStore(() => createSelector(
    state => customization.signIn[state.account.signIn.customSignIn] || MainBranding,
    state => state.account.signIn.loaded,
    createSelector(
      (_, __, dispatch) => dispatch,
      (_, props) => props.location,
      (dispatch, location) => () => dispatch(actionCreators.getSignInConfig(location)),
    ),
    (Branding, loaded, onLoad) => ({ Branding, loaded, onLoad })
  ))(SignIn);

Version 3: this version users createStructuedSelector to make it less verbose:

  connectToStore(() => createStructuredSelector({
    Branding: state => customization.signIn[state.account.signIn.customSignIn] || MainBranding,
    loaded: state => state.account.signIn.loaded,
    onLoad: createSelector(
      (_, __, dispatch) => dispatch,
      (_, props) => props.location,
      (dispatch, location) => () => dispatch(actionCreators.getSignInConfig(location)),
    ),
  }))(SignIn);

Version 4: utilize dispatchable to make the actionCreator call less verbose:

  connectToStore(() => createStructuredSelector({
    Branding: state => customization.signIn[state.account.signIn.customSignIn] || MainBranding,
    loaded: state => state.account.signIn.loaded,
    onLoad: dispatchable(actionCreators.getSignInConfig, (_, props) => props.location),
  }))(SignIn);

Version 5: make createStructuredSelector implicit if a plain object is returned instead of a function

  connectToStore(() => ({
    Branding: state => customization.signIn[state.account.signIn.customSignIn] || MainBranding,
    loaded: state => state.account.signIn.loaded,
    onLoad: dispatchable(actionCreators.getSignInConfig, (_, props) => props.location),
  }))(SignIn);

Current source with some explanatory comments is here

The main exports are connectToStore(selectorFactory, options) and dispatchable(actionCreator, ...selectorsToPartiallyApply). connectToStore is the variant of react-redux's connect() while dispatchable wraps action creators in a selector that binds it to dispatch.

The signature for selectorFactory is () => (state, props, dispatch) => {newProps}. Any reselect selectors, including those created by dispatchable created inside the factory function are scoped to that component instance.

selectorFactory has a shorthand that I use 99% of the time: () => {inputSelectors}, where inputSelectors is passed to createStructuredSelector to create the selector.

I think it's coming along nicely, although I don't yet have hot reloading support (I'm not sure what that will entail) or any argument validation. Any constructive criticism would be welcome.

[markerikson]

You've clearly put a bunch of work into this, but to be honest I'm still not really clear on what your issues are with the standard connect() function. What specific use cases were you having trouble with? How is this implementation more "Reselect-friendly" than the current version?

The one thing I'm grasping so far is that you want to state available when binding action creators, but as far as I've seen that's a pretty niche use case, and connect() supports that through use of mergeProps(), as you've seen.

[jimbolla]

@markerikson In addition to what you mentioned, the other issue I started running into was when I was using createSelector in mapStateToProps, mapDispatchToProps and mergeProps, because they each have different parameters, I had to organize my selectors by which method they aligned with. (Using createSelector to memoize my functions is useful for partially applying args from state/props, so that a new function isn't created every time an unrelated prop is changed.) It also looks like mergeProps doesn't have the same "advanced" option like the other two, so any selectors I want to use in there have to be singletons instead of being bound to the lifetime of the component instance.

So I started looking at the source for connect() to see if it would be possible to modify it to just supply dispatch as a 3rd param to mapStateToProps, but I found the source for connect() to be very difficult to mentally parse. There's a lot of magic in there to track all the various moving parts. I don't think I could make the changes I'd like to connect() without breaking something. There's 10 "cache" fields on the component and another 5 temp variables in the render() method mixed inside a lot of if/else blocks.

That's why I decided to create my own implementation using reselect to track all those moving parts instead.

[markerikson]

Fair enough.

The implementation of connect() has certainly grown rather complex. It's trying to do caching and other optimizations, allow per-instance selector memoization, etc. I think there's some related issues that are trying to rework things, like #269 , #368 , and #373. But, that said, it's the public API we've got, and it does cover the vast majority of use cases. Given that you've spent some time looking into this, I'd be a bit curious if you have any suggestions for improvements to the current implementation that would maintain the public API.

Beyond that, I'm not seeing anything specifically actionable about this issue. It's not a bug or feature request against React Redux itself, and since it's basically a one-off implementation for your own use case, I'm not quite sure what feedback you're looking for. I'll leave the issue open for the moment just so it's a bit more visible if someone's browsing the repo issues list, but will probably close soon if there's no further discussion.

[jimbolla]

I just whipped this up and haven't tested it at all, but this could be wrapper around my connectToStore that gives a compatible API to the current connect (except for the advanced scenarios for mapStateToProps and mapDispatchToProps. I'd say people that need those, probably just use connectToStore directly.

export function connectClassic(
  mapStateToProps,
  mapDispatchToProps,
  mergeProps,
  {
    pure = true,
    withRef = false,
  }
) {
  function selectorFactory() {
    const ownPropsSelector = createShallowEqualSelector(
      (_, props) => props,
      props => props
    );

    function getStatePropsSelector() {
      if (!mapStateToProps) return () => null;

      return createSelector(
        state => state,
        ownPropsSelector,
        mapStateToProps
      );
    }

    function getDispatchPropsSelector() {
      if (!mapDispatchToProps) return (_, __, dispatch) => ({ dispatch });

      if (typeof mapDispatchToProps === 'function') {
        return createSelector(
          (_, __, dispatch) => dispatch,
          ownPropsSelector,
          mapDispatchToProps
        );
      }

      return createSelector(
        (_, __, dispatch) => dispatch,
        dispatch => bindActionCreators(mapDispatchToProps, dispatch)
      );
    }

    return createShallowEqualSelector(
      getStatePropsSelector(),
      getDispatchPropsSelector(),
      ownPropsSelector,
      mergeProps || defaultMergeProps
    );
  }

  return connectToStore(
    selectorFactory,
    {
      pure,
      withRef,
      getDisplayName: name => `Connect(${name})`,
      recomputationsProp: null,
      shouldIncludeOriginalProps: !mergeProps,
      shouldUseState: Boolean(mapStateToProps),
    }
  );
}

I still need to hoist statics, support hot reloading, and add some argument validation; but at that point I think I'd have feature parity with the existing implementation.

I'll need a moment to look at those linked issues to see how they'd impact my implementation. I agree there's nothing directly actionable here for react-redux, so the issue could be closed, but unfortunately I don't know of a more appropriate place to have this discussion. I would love to get more eyeballs on this from devs that have different requirements than my own.

[markerikson]

Yeah, I do follow the reasoning for posting this here. Perhaps raise the question over in Reactiflux, just to see if anyone else has feedback?

[jimbolla]

@markerikson So I wrote a version of connect() that builds a selector from its args and passes along to connectToStore(). All react-redux tests pass with 1 breaking change for when mapStateToProps / mapDispatchToProps are factory methods and a couple tweaks to some spy counts because my implementation does things slightly different behind the scenes.

Fork is here