Refactor assigment of mapDispatch in connect.js

[seanstrom]

Hey so a number of us from the ReactVegas meetup spent some time talking about the codebase. We specifically talked about the connect function and the small piece of code that was doing the assignment of mapDispatch. In the end we went over a few ways of refactoring the code and decided that it may serve us well to submit a PR.

Here's where that input got us:

• The multiple assignments have been refactored to a single assignment and function call.
• The new function handles returning a function or an object or defaultMapDispatchToProps.
• The new function uses helpers from lodash to clean up type comparisons.
• The new function uses a switch statement at the moment, this seem to read well and I can provide more context on that decision. A valid alternative would be to use guard statements instead. 😄

Overall we're excited to participate in the development of React-Redux, so any feedback is welcomed and appreciated. Thanks! 🎉

[gaearon]

It’s tiny but we already use typeof x === 'function' elsewhere, let’s keep doing it here as well?

[seanstrom]

👍 sounds good. The main reason I used isFunction was to make the code read similar to pattern matching. Like:

case value of
  Function -> value
  PlainObject -> wrapActionCreators(value)
  _ -> defaultMapDispatchToProps

If that's a style we're interested in then maybe we can talk about it more, but for this PR i'll change it to use the typeof expression.

[gaearon]

Looks good, left one small nit.

[gaearon]

This is a very surprising use of switch statement. At this point I think the original version was clearer.

[seanstrom]

I agree that a switch statement here can be unusual. But we can translate that directly to guard statements:

function getMapDispatch(value) {
  if (typeof value === 'function') {
    return value
  }

  if (isPlainObject(value)) {
    return wrapActionCreators(value)
  }

  return defaultMapDispatchToProps
}

[seanstrom]

Or we can use if/else-if/else as well

function getMapDispatch(value) {
  if (typeof value === 'function') {
    return value
  } else if (isPlainObject(value)) {
    return wrapActionCreators(value)
  } else {
    return defaultMapDispatchToProps
  }
}

[seanstrom]

The main things I would promote are the function extraction, it adds some more clarity imo, and the revision to using isPlainObject as the second guard statement or else-if.

[gaearon]

I’d like to leave it as is. I’m happy to take a more consistent refactoring that clears up some of the convoluted code we have right now. I think the convoluted code is inside the class, not outside it.

Adding something like getMapDispatch when we already have computeDispatchProps, finalMapDispatchToProps, configureFinalMapDispatch, mapDispatch, and mappedDispatch in the scope is only going to make it even more confusing. So I won’t take this, but I’m happy to consider a more comprehensive refactoring.

Thanks!

[seanstrom]

No problem 😄! I'm happy to try a different refactor later.

[amccloud]

@seanstrom I'm working on some of the code in the connect class. Would appreciate your feedback #368