Pass props into state selector

[jhollingworth]

It would be really handy if you could have access to the props when selecting state. This PR passes them in as a second argument to mapStateToProps

@connect((state, props) => state.users[props.userId])
class User extends React.Component {
   render () {
      const {user} = this.props
      return <div className='User'>{user.name}</div>
   }
}

React.render(<User userId={123} />)

[gaearon]

Please see this discussion: #41.
Does having the third argument help you?

[jhollingworth]

Sorry, I might be missing something but #41 seems to be a bug about props being stale rather than making them accessible to mapStateToProps.

Does having the third argument help you?

Do you mean mergeProps? Yes it does, in that I can work around your API to achieve what I want. However it doesn't seem like a very nice API to me. Maybe thats because I'm not thinking in the 'redux' way?

Say I've got a users store and I want to render a single user from that store but I don't want to pass in any action creators. Based on your example I would have to do this:

@connect(
   state => { users: state.users },
   null,
   (stateProps, dispatchProps, defaultProps) => { user: stateProps[defaultProps.userId] }
)
class User extends React.Component {
   render () {
      const {user} = this.props
      return <div className='User'>{user.name}</div>
   }
}

React.render(<User userId={123} />)

With my proposed change all you'd need to do is

@connect((state, props) => { user: state.users[props.userId] })
class User extends React.Component {
   render () {
      const {user} = this.props
      return <div className='User'>{user.name}</div>
   }
}

React.render(<User userId={123} />)

Maybe you're usage is different but selecting something from state based on the parent props is something I do in nearly every 'smart' component. This proposed change makes this use case significantly simpler.

Do you have any specific motivations for keeping the current API?

[danharper]

+1 I found the new API quite verbose for this (seemingly common) use-case.

But I'm not if there's a specific reason that I'm missing for why props aren't provided to mapStateToProps.

[gaearon]

So you need to select state by ID but not action creators? Why? Wouldn't you need that in real apps, as opposed to simple examples?

My only concern with adding props to first parameter is because people will expect it to be available to mapDispatchToProps too. But we can't give it there—this means we have much worse performance because we re-bind action creators on every prop change regardless of whether user uses this parameter.

[danharper]

Ah, makes sense to me.

On Sun, 9 Aug 2015 13:31 Dan Abramov notifications@github.com wrote:

So you need to select state by ID but not action creators? Why? Wouldn't
you need that in real apps, as opposed to simple examples?

My only concern with adding props to first parameter is because people
will expect it to be available to mapDispatchToProps too. But we can't
give it there—this means we have much worse performance because we re-bind
action creators on every prop change regardless of whether user uses this
parameter.

—
Reply to this email directly or view it on GitHub
#36 (comment).

[jhollingworth]

So you need to select state by ID but not action creators?

About 50% of our components don't need action creators at all, they just render data.

For those that we just pass the prop value from within the component

@connect((state, props) => { user: state.users[props.userId] }, { updateUser })
class User extends React.Component {
  render () {
    const {user} = this.props
    return (
      <input
        type='text'
        ref='name'
        value={user.name}
        onChange={::this.updateUser} />
    )
  }

  updateUser (e) {
    const {userId} = this.props

    this.props.updateUser(userId, {
      name: e.target.value
    })
  }
}

To me this is much more understandable since its clear where an action creators arguments are coming from.

[jhollingworth]

My only concern with adding props to first parameter is because people will expect it to be available to mapDispatchToProps too. But we can't give it there—this means we have much worse performance because we re-bind action creators on every prop change regardless of whether user uses this parameter.

Why not leave that as a note in the documentation for mapDispatchToProps?

[gaearon]

To me this is much more understandable since its clear where an action creators arguments are coming from.

But it's asymmetric. You pass just one user, but updateUser requires its ID as a parameter? This is an implementation detail leaking into the component. If component is the encapsulation boundary, this is not the minimal API for it.

[jhollingworth]

But it's asymmetric. You pass just one user, but updateUser requires its ID as a parameter? This is an implementation detail leaking into the component. If component is the encapsulation boundary, this is not the minimal API for it.

I still personally prefer to know precisely where all arguments to a function come from rather than having to trace through multiple function calls but I completely see where you're coming from.

What about when your component doesn't need action creators? A common use case in my code base but maybe I'm in a minority. Unless I'm missing something you still have to do this

@connect(
   state => { users: state.users },
   null,
   (stateProps, dispatchProps, defaultProps) => { user: stateProps[defaultProps.userId] }
)
class User extends React.Component {
...

[gaearon]

We can reorder parameters in mergeProps and it would be

@connect(
  state => ({
    users: state.users
  }),
  null,
  (ownProps, stateProps) => ({
    user: stateProps.users[ownProps.userId]
  })
)

Is it really that bad?

We get both symmetry and better performance because we don't have to recompute mapStateToProps every time props change. I think it's the way to go..

[gaearon]

With destructuring:

@connect(
  ({ users }) => ({ users }),
  null,
  ({ userId }, { users }) => ({  user: users[userId] })
)

[jhollingworth]

Personally that doesn't exactly feel intuitive to me

What about if you could pass in an object literal to connect?

@connect({
  bind: { updateUser }, // default null
  select: state => { users: state.users }, // default state => state
  merge (props, state, actionCreators) {
    return {
      user: state.users[props.userId],
      updateUser: (text) => actionCreators.updateUser(userId, { text })
    }
  }
})

With those defaults and your suggestion of moving props to being the first argument we can reduce it down to

@connect({
  merge (props, state) {
    return {
      user: state.users[props.userId]
    }
  }
})

[jhollingworth]

also, sorry, don't want to be annoying. I'd just like to find a nice API :)

[gaearon]

state => state is a very bad default performance-wise. It means that, as your app grows, existing parts of it start to work much slower without any apparent reason.

Performance is more important to this library than succinctness. We can sacrifice some convenience if that means people will get more performant apps by default. We will also never suggest "bad perf" patterns as defaults.

[jhollingworth]

Fair enough, will close this then

[jgavanleeuwen]

Dont want tot reopen this request on purpose; but this use-case is very much like mine. Looking ahead I was wondering whether Reselects (memoized) selectors can then be applied tot the filter in the mergeProps?
Is that a good use for selectors in the new react-redux api?

Thanks in advance

[gaearon]

Dont want tot reopen this request on purpose; but this use-case is very much like mine. Looking ahead I was wondering whether Reselects (memoized) selectors can then be applied tot the filter in the mergeProps?

Can you ask this in Reselect repo? I'm not sure how to do that, but I'd sure like to know :-)

[ForbesLindesay]

I am finding this to be a really frustrating use case for me. I almost always want props to be available to both the selectState and mapDispatchToProps functions. Could we have @conenct behave as it does currently, but have a second connector @connectWithProps that gave props to both functions? We could still avoid re-binding when props haven't changed by checking shallowEqual on the props.

Another option, we could check fn.length and not pass props if that returns 1.

[gaearon]

We could still avoid re-binding when props haven't changed by checking shallowEqual on the props.

But we would still have to rebind on every prop change when they aren't equal, but we don't really know if user even used props.

Another option, we could check fn.length and not pass props if that returns 1.

Yes, I guess we'll have to resort to this to satisfy everybody. :-) Normally I hate to do this, but this seems like a valid use case. Want to make a PR?

[gaearon]

I created #52, let's discuss there.

[gaearon]

Released as 0.9.0, please verify.