Add state as the third parameter to mapDispatchToProps

[esamattis]

So that I could do this:

var ToggleFollowButton = ({toggleFollow}) => <button onClick={toggleFollow}>Toggle</button>;

ToggleFollowButton = connect(null, (dispatch, ownProps, state) => {

  const toggle = state.postsFollowing[ownProps.id]
    ? unfollowPostActionCreator 
    : followPostActionCreator;

  return {
    toggleFollow: () => dispatch(toggle(ownProps.id)))
  };

})(ToggleFollowButton)

Now I'm fully aware that I can do this in the component code by passing the "follow state" and both action creators to it, but when using function components this would be more performant way to do this. In normal class based component I can do this only once as a method but with a function component I lose some performance because the toggle function is recreated on every render. Also when using react/jsx-no-bind eslint rule it's hard to avoid the warning.

Another point is that if the toggle function is recreated on every render it can interfere with PureRenderMixin optimized components further down in the component tree.

---

Another option would be to add dispatch to mapStateToProps as the third argument. Which actually could be prettier because then the object shorthand for mapDispatchToProps would be still available.

EDIT: Also it would be easier to implement because the reinvoke semantics of mapDispatchToProps would not change. So I'll actually propose this version :)

I can also send a PR if we see this as something we want.

[acjay]

I agree. For full generality, connect should provide some way to generate a props object out of a closure that is injected with both dispatch and getState. Just like redux-thunk does. I allude to this in my answer to a question on StackOverflow. Not sure how this could be accomplished without breaking changes. But I think the existing API is too constrained/opinionated. I'm going to be potentially controversial and suggest that a breaking change should be made, and connect should simply have the signature connect((dispatch, getState, ownProps) => stateProps, options). The current usage could just be a specially case of this more general form. (Maybe I'm missing some performance implications or something?)

[esamattis]

Maybe I'm missing some performance implications or something?

Well now the implementation is very efficient. It will not run mapDispatchToProps when store changes or even when the ownProps changes unless it is actually used in the function. If there would be only one general map function the action creators would be rebound on every prop and state change.

AFAIK adding dispatch to mapStateToProps as the third argument would not have any performance implications nor it would be a breaking change.

[acjay]

That makes sense to me, I've glanced at the implementation of connect, but haven't really traced it out. But I still think getState needs to be passed in, rather than a state snapshot. Otherwise, in the methods of the object you pass in, you won't be able to have async logic that gets the updated state in callbacks.

[esamattis]

I don't think async logic belongs in the map functions at all. They should be only pure functions which transforms the state to props and binds action creators to dispatch. Which means you cannot modify the state during the map state execution so getState would always just return the same state.

Also if you could modify the state from the map state functions it could lead to infinite loops because every update would reinvoke the map functions.

Async logic goes into action creators and middleware. See http://redux.js.org/docs/advanced/index.html

---

On the side note I think react-redux should explicitly deny invoking the dispatch function during map state calls. It should be only allowed to be bound for later invocation.

[esamattis]

If you take a more closer look at the implementation of connect() here

https://github.com/rackt/react-redux/blob/caae2e1/src/components/connect.js#L214-L219

you see that the map state functions are called during render. Would start an async operation from render method of a component?

[acjay]

Async logic goes into action creators and middleware. See http://redux.js.org/docs/advanced/index.html

Sure, I think that's one opinion, but I don't take that as gospel.

In the paradigm you describe, you dispatch impure blobs, and then rely on some magic in the dispatch mechanism to eventually dispatch regular objects through the reducers to compute the new state. But that's not the only way to do it.

redux-saga puts the asynchrony in long-running generators in a side band. They still hook into the dispatch mechanism, but only to listen for actions and to input new actions. So dispatch in this sense is repurposed as an event bus, but you're only ever dispatching POJOs as actions. A proposed "reactions" approach seems to be based around a similar conceptualization, except that it's monitoring the state instead.

An alternative I've been experimenting with uses vanilla Redux's pure, synchronous action creators (before async is introduced in the guide) and an middleware-free dispatch mechanism, which only manages state mutations, rather than serving as a bus. In this model, you create an action POJO, and then the entire flow from action creation -> Redux dispatch -> reducers -> new normalized state -> new denormalized state (with computed data) -> react-redux connect -> React props/context -> render is synchronous and pure. There's nothing out of band going on in that entire flow.

For asynchrony, you give functions that live out of that flow a callback to dispatch actions and a callback to check current state. You mix those functions in to props so that your UI's event handlers can trigger them. render just sets up event handlers, it doesn't directly trigger anything. This is the idea behind react-redux-controller.

I don't see my approach as mutually exclusive with something like redux-saga. The latter sounds like a smart approach to having long-running processes outside of the UI that share state with the UI and can generate changes, like websocket listeners, automation, etc. But for stereotypical MVC controller logic, like bread-and-butter input/output, I think my approach is conceptually simpler and cleaner.

[acjay]

The proposed "reactions" approach actually works differently than I first reported. My current understanding is that it uses the store to represent effects that should happen and the state of their consequences. Then, a new reactions stage realizes those effects. This stage runs after the reducers produce the next state.

So, for example, an API call needs to be made. Dispatching the action that should trigger it sets an "effect" value in the state, which expresses that "data should be getting fetched right now from /myApi/data". The reducers merge that effect into the state. The reactions system should check whether that request is pending, and if not, start it.

When the XHR completes, its continuation would dispatch an action that should have the result of removing that fetch effect from the state.

This approach is intriguing because it refies the state of ongoing process in the store. So you could serialize the state of your application down to having a specific async fetch in progress, and rehydrate it in that exact state. Wild!

[gaearon]

connect() accepts a third argument called mergeProps precisely for this use case.

var ToggleFollowButton = ({toggleFollow}) => <button onClick={toggleFollow}>Toggle</button>;

ToggleFollowButton = connect(
  ({ postsFollowing }, { id }) => ({
    isFollowing: postsFollowing[id]
  }), (dispatch) => ({
    dispatch
  }), ({ isFollowing }, { dispatch }, { id, ...otherProps }) => ({
    ...otherProps,
    toggleFollow: isFollowing ?
      () => dispatch(unfollowPostActionCreator(id))) :
      () => dispatch(followPostActionCreator(id)))      
  })
})(ToggleFollowButton)

Or, in a more readable way:

I think adding getState() to mapDispatchToProps has the potential of being very confusing and people might start using it instead of mapStateToProps and wonder why it doesn't redraw.

[gaearon]

Same example in a less terse way:

function mapStateToProps(state, ownProps) {
  return {
    isFollowing: state.postsFollowing[ownProps.id]
  };
}

function mergeProps(stateProps, dispatchProps, ownProps) {
  const { isFollowing } = stateProps;
  const { dispatch } = dispatchProps;
  const { id } = ownProps;

  const toggle = isFollowing ?
    unfollowPostActionCreator :
    followPostActionCreator;

  return {
    ...ownProps,
    toggleFollow: () => dispatch(toggle(id)))
  };
}

ToggleFollowButton = connect(
  mapStateToProps,
  null,
  mergeProps
})(ToggleFollowButton)

[gaearon]

I'm going to be potentially controversial and suggest that a breaking change should be made, and connect should simply have the signature connect((dispatch, getState, ownProps) => stateProps, options). The current usage could just be a specially case of this more general form. (Maybe I'm missing some performance implications or something?)

Please take a look at #1. This option has been considered and rejected. Yes, it's very bad for performance to re-bind action creator on every dispatch, which is what will happen if we let people access the state in the same place they bind action creators. Technically they can still do it now with mergeProps but it's hidden well enough that people who aren't experienced with Redux don't discover it by mistake.

[gaearon]

Another point is that if the toggle function is recreated on every render it can interfere with PureRenderMixin optimized components further down in the component tree.

Absolutely correct, this is a performance nightmare scenario where nothing really changes but bound functions kill the performance optimizations.

In fact it's so bad I don't recommend what I wrote above. Bind action creators as rarely as possible, and prefer to not bind them to particular IDs. Instead, just let your components call appropriate props with appropriate arguments:

handleClick() {
  if (this.props.isFollowing) {
    this.props.unfollow(this.props.id);
  } else {
    this.props.follow(this.props.id);
  }
}

This is not as elegant but it's much more performant and also easier to understand than a complex connect() declaration.

I'm closing this because I think the two workarounds I offered above will suffice. If you believe React Redux API is not good enough please create another library that better fulfills your goals. The goal of this library is to encourage performant patterns because otherwise people will say “Redux is slow!” even if the cause is their suboptimal function binding code. We don’t want this to happen, so we’d rather make certain unperformant cases harder to implement.

[acjay]

@gaearon Thanks for the info.

With that taken into consideration, I still think having getState in mapDispatchToProps is an important missing piece. Obviously, not everyone agrees with putting asynchrony downstream of the store. But as long as that's not an unreasonable architectural choice, then the downstream still needs a way to get the state after it may have been updated. As far as I can tell, it would add a possibility that doesn't exist today. I may be missing something, but I don't think the solutions you presented account for this, because I don't think mergeProps allows for it. Also, It doesn't seem like having a getState callback would ruin performance, as it could be injected once and always work.

react-redux is a great API, and I hope my comments haven't come off to the contrary. But I hope you understand that it makes more sense to try to advocate for ideas in an existing library than to maintain a fork or entirely separate product. I realize that a great deal of thought has been put into this already, so no disrespect intended to the work that's already been done. I haven't read #1 yet, but I definitely will, because I want to present productive ideas that haven't already been beaten to death.

[gaearon]

I'm hesitant adding it because people might get the idea that mapDispatchToProps itself is a good place to read the state by calling getState. These people will be surprised that mapDispatchToProps is not called when the state is updated, breaking their apps. We won't have a way to protect against this misusage either.

[esamattis]

Oh yeah. Somehow I didn't realize mergeProps can be used for this.

In fact it's so bad I don't recommend what I wrote above. Bind action creators as rarely as possible, and prefer to not bind them to particular IDs. Instead, just let your components call appropriate props with appropriate arguments:

Yep, this what I'm actually doing now. It just makes me bit sad that the class based version is actually more performant and I maybe even more terse than the functional version.

[SimenB]

In fact it's so bad I don't recommend what I wrote above. Bind action creators as rarely as possible, and prefer to not bind them to particular IDs. Instead, just let your components call appropriate props with appropriate arguments

The documentation only states the thing you you say you don't recommend here. Update it perhaps?

For reference, the sentence I'm referring to (emphasis mine):

You may specify this function to select a slice of the state based on props, or to bind action creators to a particular variable from props.

[gaearon]

People asked for this, so I added this API. That I don't recommend doing this is my personal opinion. Most projects can do this just fine; it's only a problem when you want to squeeze that last bit of performance. Feel free to PR the docs to make them more nuanced!

[pke]

@gaearon Dan, I wonder, the default mergeProps is Object.assign({}, ownProps, stateProps, dispatchProps) as stated in the docs.
Doesn't that also create a new object all the time, just like when one would override mergeProps with a custom function for binding action creators?

Absolutely correct, this is a performance nightmare scenario where nothing really changes but bound functions kill the performance optimizations.

But when nothing really changed, the default mergeProps still creates a new object on every render. How does that not matter?
Maybe I'm confused, where the pure-render optimisation kicks in and which props it checks.

[acjay]

Yeah, I'm guessing he was referring to the idea that the component passed to connect is configured to shallow-compare it's props to their previous values. The suggested method of writing reducers would produce a state object that is shallow-equivalent to the previous generation. And since this object is merged into the new props object, assuming nothing else changed, it would be shallow-equivalent to the old props. But freshly bound action creators would disrupt this equivalence.