Could mapStateToProps be an object?

[selbekk]

I'm playing around with selectors, and most of my mapStateToProps functions look like this:

const mapStateToProps = state => ({
  isAuthenticated: selectors.isAuthenticated(state),
  isSomethingElse: selectors.isSomethingElse(state),
});

Since I always pass on the state, would it be okay to do the same thing as mapDispatchToProps does, and just allow sending an object of functions? Like so:

const mapStateToProps = {
  isAuthenticated: selectors.isAuthenticated,
  isSomethingElse: selectors.isSomethingElse,
};

Then, connect could do the work required to call each of these with the state.

I'd be happy to submit a PR doing this - but what are your thoughts? I'm sure there's a reason the API is the way it is :)

[timdorr]

It makes the logic around running selectors much more complex. We would have to iterate through all the keys and check for shallow equality there, which could be slow. Also, it becomes more confusing for users. Would this be expected to work?

const mapStateToProps = {
  isAuthenticated: selectors.isAuthenticated,
  foo: {
    isSomethingElse: selectors.isSomethingElse,
  }
}

I don't think so, but I could see someone being confused or tripped up by the way they import things.

It's significantly simpler for us to implement this as just a pure function and also simpler to present to users as an API. So, I don't think we'll add something like this.

[selbekk]

Fair enough :-)

[markerikson]

We had a potential PR over at #724 that sat around for a while before we finally closed it. There's a lot of potential edge cases around the "factory function" syntax that mapState supports, and eventually it became more complex than we wanted to deal with.

[selbekk]

Understandable! Thanks for taking the time to explain! :-)

[jchook]

I also wondered about this. Seemed lopsided to have mapDispatchToProps accept an object, but not mapStateToProps.

@selbekk I guess we could use a function like this:

const bindSelectors = (selectors) => (state) => {
  const props = {}
  Object.keys(selectors).forEach(key => {
    props[key] = selectors[key](state)
  })
  return props
}

Naively seems like a PR could essentially get by with:

if (isPlainObject(mapStateToProps)) {
  mapStateToProps = bindSelectors(mapStateToProps)
}

[markerikson]

I'm willing to potentially reconsider this as long as we keep it narrowly scoped in behavior and implementation.

No factory functions. No nesting.

I can see two useful forms:

• An array of string key names
• An object of selectors

Both are doable if we keep the behavior limited to basically what @jchook showed. Question is, whether there's enough benefit for including that functionality.

Tweeted this question: https://twitter.com/acemarke/status/1087568080052596736

I'll reopen this issue for the sake of discussion.

[manuelbieh]

Would love to see a shorthand notation where I can just pass an object of selectors. It can be so cumbersome to pass state to every single selector over and over again.

[timdorr]

I really really don't want to do this. Our selector code is complex enough as it is. Just write a combineSelectors function and leave it at that. This API is confusing enough as it is.

[jchook]

Tim, do you think it would be worse than adding whenMapStateToPropsIsAnObject()?

Again, forgive my naive perspective, but I see we have:

mapDispatchToProps.js

• whenMapDispatchToPropsIsFunction()
• whenMapDispatchToPropsIsMissing()
• whenMapDispatchToPropsIsAnObject()

mapStateToProps.js

• whenMapStateToPropsIsFunction()
• whenMapStateToPropsIsMissing()
• whenMapStateToPropsIsAnObject() ❓

Can we simply add this "missing" factory?

export function whenMapStateToPropsIsObject(mapStateToProps) {
  if (mapStateToProps && typeof mapStateToProps === 'object') {
    return bindSelectors(mapStateToProps)
  }
}

[timdorr]

The issue isn't necessarily hooking it up, it's when it comes to memoizing output and perf issues. That sort of stuff adds a lot more complexity to the code.

[jchook]

What if we converted it to a function before reaching any of that code?

For example, adding bindSelectors() logic here before anything else.

Pros:

• Conversion runs once, not on each render
• Fully supports existing performance enhancements + memoizing

Cons:

• Might break custom factory stuff?
• Could require additional option to turn it off?

To illustrate, consider this workaround that shouldn't cause any render performance impact since it simply preps mapStateToProps before giving it to connect():

import { connect as realConnect } from 'react-redux'

export const connect = (Component) => (mapStateToProps, ...args) => (
  realConnect(Component)(
    mapStateToProps && typeof mapStateToProps === 'object'
      ? bindSelectors(mapStateToProps)
      : mapStateToProps,
    ...args
  )
)

[markerikson]

Part of me still kinda wants do to this, but I'll go along with Tim and say it's not worth it to build it in.

[jchook]

Yeah on second look I agree.