Is double-connecting a component bad/inefficient?

[jimbolla]

I've found the syntax to be less awkward, especially when using reselect, to connect once to get access to dispatch, then connect a second time to do state stuff and memoize, vs. having to define a mergeProps function. I'm just concerned that double-connecting is going to somehow cause me problems, or that I'm losing some crucial optimization that connect() would be doing if I were doing it with a single connect.

For example, I have this:

const selectOnToggleIsActive = createSelector(
  selectApp,
  selectIsAppsAdmin,
  (state, props) => props.dispatch,
  (app, isAdmin, dispatch) => (
    isAdmin
      ? () => dispatch(actions.setIsActive({ appId: app.id, isActive: !app.isActive }))
      : null
  )
);

export default compose(
  connect(), // + props.dispatch
  connect(createSelector(
    selectApp,
    selectOnToggleIsActive,
    (app, onToggleIsActive) => ({
      appId: app.id,
      onToggleIsActive,
    })
  )),
)(EditMenu);

[markerikson]

Uh... semi-stupid question: why are you needing to use mergeProps? That function is pretty rarely used. Do you have a specific use case you're trying to work with?

[jimbolla]

@markerikson Well basically, because what action gets returned to the component needs to use state. I don't have access to dispatch in mapStateToProps to fully bind the method, and I don't have access to state in mapDispatchToProps to do so, so I have to do it after both have run, either in mergeProps, or using recompose's withProps. In an ideal case, I'd have access to state, dispatch, and ownProps in a single pass.

In this case, the state is the current user's authorization. The result is either the real action, or nothing. Our app has role based authentication (details stored in state.auth) where employees in the admin role can can perform write actions from a screen where other users can only get a readonly version. The components key off whether the event handler functions are null or not to make that determination.

[jimbolla]

I'm think maybe code that uses react+redux+reselect could use a simpler implementation of connect. Since reselect is good and reconciling merging changes together, give the calling code the responsibility to memoize its results on state/props changes. I might mess around with implementing this idea.

[markerikson]

Hmm. A couple thoughts:

• I'm not really following exactly why that "double-connect" is necessary, as such.
  I get that you want state available in mapDispatch, and technically mergeProps is the escape hatch that allow for that, but why is the "double-connect" needed? Feels like there's got to be some other way to do what you want.
• Is there any reason why the component can't call the action creator with the necessary props, rather than having those pre-bound?
• Seems kinda like what you really need is a Higher-Order Component that is specialized for handling auth stuff somehow. You might want to watch @acdlite's talk from React Europe: https://youtu.be/zD_judE-bXk. You're sorta doing that now, but I think it'd be cleaner if you separate that part out.

[jimbolla]

The double connect is the alternative to using mergeProps, because then I have access to dispatch during mapStateToProps and I can do everything in there with one composed createSelector. I can do what I need to do with either double-connect, or using mergeProps. Double-connect makes for slightly cleaner code, which is why I asked if it will cause any issues. Looking at the source for connect, I'm not seeing any reason why, but that's not the easiest code to grok.

Having the component call the method with the parameters prebound avoids extra method create in the component, ex: onClick={() => props.onAction(props.param) vs onClick={props.onAction}. The former will trigger the react/jsx-no-bind eslint rule, which can be solved either by moving the binding into my enhancer or turning my functional component into a full class.

[jimbolla]

I'm working on an alternate version of connect that complements use with reselect library. Since reselect handles all the heavy lifting of caching changes, connect's implementation is much simpler. Here's a gist with a snapshot of my work in progress.. Currently no hotloading support or parameter validation of any kind, but I think the main use case is working correctly.

[markerikson]

Hmm. What's the actual meaningful difference with that approach?

[gnoff]

@jimbolla Your use of dispatch in this way does feel a bit awkward to me but I can see why having dispatch available in the selector can be useful if you want to keep all this kind of thing in selectors exclusively.

connect doesn't add much overhead beyond what any HOC would if you aren't subscribing to anything so the double connect isn't really an issue per se. If you want to make your intent more clear to others I might suggest doing something like this though

const provideDispatch = connect();


export default compose(
  provideDispatch, // + props.dispatch
  connect(createSelector(
    selectApp,
    selectOnToggleIsActive,
    (app, onToggleIsActive) => ({
      appId: app.id,
      onToggleIsActive,
    })
  )),
)(EditMenu);

this way you can swap out the implementation for provideDispatch in the future without finding every place you happened to do the double connect.

[jimbolla]

@gnoff That's a good idea. If I don't end up using my own implementation of connect I would do that.

@markerikson Partly stylistic and syntactical preference, partly being able to leverage the abilities of reselect. It's still in flux (no pun intended), but my current syntax looks like this for a simple use case:

import connectToStore, { selectProps, partialApply } from 'appcenter/utils/connectToStore';
import { toggleIsActive } from '../actions/apps';
import { getApp, getAppId, getIsAppsAdmin } from '../selectors/apps';

// later...

export default connectToStore(
  ({ toggleIsActive }),
  ({ toggleIsActive }) => { // eslint-disable-line no-shadow
    const getToggleIsActive = partialApply(toggleIsActive, getAppId);

    return selectProps(
      [getApp, getToggleIsActive, getIsAppsAdmin],
      (app, onToggleIsActive, isAppsAdmin) => ({
        appId: app.id,
        canCustomize: Boolean(app.customAppConfig),
        isActive: app.isActive,
        onToggleIsActive: isAppsAdmin ? onToggleIsActive : null,
      })
    );
  }
)(EditMenu);

Now I have both state and bound action creators available so I can build my props object in a single pass.