New PR for mapStateToProps shorthand syntax

[slorber]

(replaces #323)

[markerikson]

Couple questions:

• what does the shorthand syntax look like with ownProps?
• Does this interact with the "factory" syntax at all? If so, how does that look?

[slorber]

With props, the props should get passed to the selectors if any of the selector takes 2 parameters. I can probably add a new test for that:

const user= (state, props) => state.users[props.userId];
const city= (state) => state.city;

connect({user,city})(Comp)

Not sure what you means by "interact with the factory syntax". It's been a while last time I took a deep look into implementation so I may not be the most up to date. Any unit test to check the expected behavior?

[markerikson]

A basic mapState factory function usually looks like:

const makeMapState = (state, ownProps) => {
    const uniqueSelectorInstance = makeSpecificSelector(state, ownProps.itemId);

    const actualMapState = (state, ownProps) => {
        const someData = uniqueSelectorInstance(state, ownProps.otherProp);

        return {someData};
    }

    return actualMapState;
}

export default connect(makeMapState)(MyComponent);

[slorber]

Current PR code only kicks in when connect is called with an object param so it's unlikely to mess-up with this feature. None of the tests related to factories are failing.

[markerikson]

Okay. What about if the user uses the factory syntax to return an object? I think that'd look like:

const makeMapState = (state, ownProps) => {
    const uniqueSelectorInstance = makeSpecificSelector(state, ownProps.itemId);

    const actualMapState = {
        someData : uniqueSelectorInstance
    };

    return actualMapState;
}

[josepot]

Sorry to get in the middle, but I think that my PR #724 addresses @markerikson concern. Could you please have a look at it? There is a specific test for that.

[slorber]

The subject of factories is IMHO more complicated than that.

There are multiple ways we could define factories, and we could also imagine recursive structures.

1. makeMapState returning an object of selector

const makeMapState = (state, ownProps) => {
  const selector = state => state[ownProps.objectProperty]  
  return {
    propertyValue: selector
  };
}

@connect(makeMapState)(Component)

2. Object containing makeMapState functions

@connect({
  value1: makeMapState,
  value2: regularSelector,  
})

3. Object of makeMapState functions which return makeMapState objects (or vice versa), leading to potential infinite loops

const makeMapState = (state, ownProps) => {
  const selector = state => state[ownProps.objectProperty]  
  return {
    propertyValue: makeMapState
  };
}

@connect({
  value: makeMapState,
})

4. Also worth noting the case where someone wants to inject callbacks through shorthand syntax (quite fancy I know): there's no way to detect the user is not trying to use factories instead of injecting the callback.

const makeMapState = (state, ownProps) => {
  const selector = state => state[ownProps.objectProperty]  
  return state => selector(state);
}

@connect({
  // Is it a factory or a callback injection? Signature looks the same in both cases!
  value1: (state,ownProps) => (e) => alert("This is a callback, not a selector - " + e.target.value)
  value2: makeMapState
})

---

I'd rather merge a simple implementation now that solves most common usecases, and add more fancy and complex options later. It's a shorthand syntax so users that need factories can still fallback to regular syntax

[josepot]

There are multiple ways we could define factories

Sure, but there is a documented way the defines what's supported (from the docs):

Note: in advanced scenarios where you need more control over the rendering performance, mapStateToProps() can also return a function. In this case, that function will be used as mapStateToProps() for a particular component instance. This allows you to do per-instance memoization.

I don't see why the shorthand shouldn't support that option.

If there is an implementation of the shorthand that's compatible with all the currently possible usages of the mapStateToProps argument: why do you think that's best to release an incomplete version first?

Granted that your implementation would work for most people, but I don't understand why you don't want to have the "complete" implementation from the beginning.

I mean, all your objections about possible recursive structures also apply to a "bad implementation" of a factory function that could happen today without using the shorthand... Or even a bad selector implementation:

const mapStateToProps = (state, ownProps) => ({
  someProperty: mapStateToProps(state, ownProps),
});

const ContainerThatWillNeverWork = connect(mapStateToProps)(Component);

The fact that we can't prevent developers from doing things that are absurd or wrong shouldn't stop us from building powerful APIs.

[slorber]

That's true

So the real question is probably to know weither these 2 syntaxes are useful and should be supported or not?

@connect({
  value1: makeMapStateReturningFn,
  value2: regularSelector,  
})

@connect(makeMapStateReturningObject)(Component)

Personally I'll let others decide but I'll probably never use them.

The initial purpose of my PR was to provide the createStructuredSelector of Reselect directly inside React-redux, nothing more.

[josepot]

The only reason why I made my PR is because I would really like to have this shorthand and I was under the impression that the main reason why the previous PR was not moving forward was because some of the members also wanted the shorthand to support the "factory" style, which I thought that was a good point.

About the syntaxes that you are mentioning, I don't see any reason why the following one shouldn't be supported and I'm quite confident that my implementation does support it:

@connect({
  value1: makeMapStateReturningFn,
  value2: regularSelector,  
})

As per the second syntax that you are asking for: @connect(makeMapStateReturningObject)(Component). I really don't know what you mean, isn't that the definition of a normal selector that's already being supported today? What does that have to do with the shorthand?

The way I see it is: if the first argument of the connect function is an object, then it's a shorthand. If its a function then it's either a standard selector or a "factory" selector (what's already being supported today). I wouldn't complicate things any further.

The only change in the docs that I'm suggesting in my PR is this:

If an object is passed, each function inside it is assumed to be a Redux selector or a Factory function.

[slorber]

Also added factories support to this PR.

[josepot]

Hi @slorber!

While your tests are passing I get warnings while running them. I don't get those warnings in the tests of my PR or in the tests of the master branch.

Those warnings are:

      ✓ should pass state and props to the given component
The selector for mapStateToProps of Connect(Container) did not specify a value for dependsOnOwnProps.
mapStateToProps() in Connect(Container) must return a plain object. Instead received bar.
mapStateToProps() in Connect(Container) must return a plain object. Instead received 42.
      ✓ should pass state to given component, with shorthand syntax
The selector for mapStateToProps of Connect(Container) did not specify a value for dependsOnOwnProps.
mapStateToProps() in Connect(Container) must return a plain object. Instead received baz.
mapStateToProps() in Connect(Container) must return a plain object. Instead received bazbaz.
mapStateToProps() in Connect(Container) must return a plain object. Instead received bazthroughbazthrough.

I think that this should be fixed, because otherwise the users will receive misleading warnings while using the shorthand implementation.

Although, that problem has already been addressed in the code of my PR... ¯_(ツ)_/¯

[slorber]

@josepot I've fixed these warnings

Honestly you may probably be wondering why I updated my PR: I think it's uncool to open a PR one hour after mine instead of commenting on mine. I'll let others decide otherwise I'm fine with your PR.

[josepot]

Hi @slorber

I think it's uncool to open a PR one hour after mine instead of commenting on mine

I'm very sorry that you feel like that, but I think that you are being a bit unfair with me. I did share those comments months ago in your original PR and you said that you were ok with me opening another PR.

I guess that there has been a misunderstanding because I've been waiting all this time for the original PR to get closed so that I could open a PR with the code that I shared months ago.

[slorber]

You are right I'm sorry.
I'm closing my PR in favor of yours as they are quite similar.

Still feeling a bit frustrated, not really liking loosing time uselessly :'(

[josepot]

Still feeling a bit frustrated, not really liking loosing time uselessly :'(

Hey @slorber . I'm very sorry, for real. I think that this has been a community effort and I honestly think that if my PR gets merged you should get credit for it. What about you sending a PR to my branch to do the next round of improvements? (I have a couple already in mind)

That way if it gets merged we both get credit for it which is what I think it's fair. ✋

[slorber]

Thanks for that. I'll be fine :) don't have much time left to work on opensource unfortunatly