MapStateToProps shorthand syntax: Supporting factory selectors

[josepot]

Like #723 but supporting factory selectors... Because I'm of the opinion that the object-shorthand syntax should also support factory selectors like @jimbolla suggested in the past. 😄

cc: @gaearon, @jimbolla , @markerikson , @slorber, @timdorr

[pcwinters]

don't forget on line 55 to document the argument as [mapStateToProps(state, [ownProps]): stateProps] \(*Object* or *Function*)

[josepot]

Done! Thanks!

[pcwinters]

Am I right to assume that, using this shorthand, a selector could not return a function that wasn't intended to be a factory?

@connect({
  getterFnForFooState: (state) => () => state.foo
})

I would remove the factory function feature from the PR because I think it would be simpler and more easily justify the inclusion of the shorthand. If you're looking to construct selectors per component instance, you can use @connectAdvanced. It looks like you have the last PR standing, but I bet it's going to be here for awhile with such a substantial change to the API.

[markerikson]

Yeah, I'm afraid I've been focusing on other tasks and not actively trying to get this (and other long-standing PRs) pushed through to completion.

I can't promise a specific deadline, but I will put reviewing this as a high priority and will do my best to get to it within the next week-ish.

[josepot]

Am I right to assume that, using this shorthand, a selector could not return a function that wasn't intended to be a factory?

Yes.

I would remove the factory function feature from the PR because I think it would be simpler and more easily justify the inclusion of the shorthand. If you're looking to construct selectors per component instance, you can use @connectadvanced

That's fair, but you could also argue that in that case you could just use the normal connector:

@connect(state => ({
  getterFnForFooState: state => () => state.foo
}))

Not that I've actually ever run into a situation where I need to do that...

On the other hand I use very often factory selectors. I would find it very handy to be able to have some properties that come from a factory selector and some properties that come from a normal selector, that's a situation that I run pretty often into and this PR would make that possible.

I bet it's going to be here for awhile with such a substantial change to the API.

I don't think that this is a substantial change to the API at all as these changes are backwards compatible. This is just a small addition to the API, just an arguably handy shorthand... The API remains almost the same. Notice that the changes into the documentation are minimal, that itself indicates that the API doesn't change much.

cc @pcwinters

[jmar777]

It would be great to see this get landed - I'd love to help if there's anything remaining to be done.

I did want to weigh in against the factory properties, though:

1. It seems like a POLA violation, as functions are perfectly valid values for properties.
2. Relatedly, it's inconsistent with the existing API, which allows function values in the object returned from mapStateToProps.
3. The current mapStateToProps implementation already lends itself well to nested factory functions, if desired.

[markerikson]

Well, I clearly didn't put reviewing this as a high priority, unfortunately. Sorry :(

But, I am actually trying to review this now. I'm not 100% familiar with the current React-Redux v5 implementation to begin with, so I'll need some time to understand how this actually changes things.

[markerikson]

Initial thoughts:

1. Not immediately related to this PR: while I understand what React-Redux v5 does at a high level, I'll admit that trying to trace what's going on through the maze of internal factories is kinda painful. Fortunately the code shouldn't be changing much going forward, but it does make it more difficult to figure out how this change affects things. (Although now that I think about it, v6 will likely be a big change as we figure out how to deal with React's async behavior.)

2. I don't think we actually have any tests related to the existing object shorthand capability for mapDispatch. That seems like a problem. It would be great if you could add some in this PR, or another separate PR.

3. I would like to see some additional tests added that cover the object shorthand case for mapState in more detail. This PR does add a test to check the behavior of per-field factory functions in an object, but I'd appreciate having some more tests to cover the "basic" selector object behavior as well.

4. The documentation update needs to add more detail. Please describe what will happen if that object of selectors is passed in, what arguments they will be called with, and how factory selectors interact with that. We also need some examples added further down the page to demonstrate how to correctly create and pass an object of selectors as mapState.

5. What will this do to Flow and TypeScript type defs? I don't use either of those myself, so I generally stay out of any of the related discussions / issues / PRs. (To be honest, I don't even know whether the "official" TS and Flow typedefs are actually in the repo here, or in DefinitelyTyped / FlowTyped.)

6. @jmar777 , can you clarify what you mean by "inconsistent with the current API"? A mapState function shouldn't be used to return functions, just data. I suppose it's technically valid to include functions in the output of mapState, but that's not something we've encouraged, and I don't particularly remember seeing people do that. But, even if someone is doing that, how are the factory properties "inconsistent" with the API? The factory selectors would still be ultimately creating a "real" selector, which would then presumably be returning an actual value at that key.

I'm not ready to merge this in yet, but I think it's on the right path. I think if we can add some more tests around both the existing behavior and the new behavior, and improve the docs appropriately, we can get this in.

Poking @jimbolla to get his thoughts on the PR.

[jmar777]

@markerikson

can you clarify what you mean by "inconsistent with the current API"

I probably should have worded this more clearly. What I mean is that I would expect this:

connect(state => ({
  foo: 'bar',
  hello: () => console.log('world')
}))

...to be consistent equivalent to the following shorthand:

connect({
  foo: 'bar',
  hello: () => console.log('world')
})

In other words, I would expect connect({ ...someProps }) to always be equivalent to connect(() => ({ ...someProps })). Unless I'm mistaken, I believe that's how the shorthand for mapDispatchToProps() behaves as well.

I see your point that function property values aren't an intended use case, but FWIW, I've personally shoved function properties in there in cases where I didn't actually need direct access to dispatch().

Aside from introducing what I'd argue is unexpected behavior, I don't really see the benefit of property-level factory functions. From a verbosity perspective, they're essentially a tie when you have a single property:

// 54 chars
connect(state => ({
  foo: selectors.getFoo(state)
}))

// vs...

// 53 chars
connect({
  foo: state => selectors.getFoo(state)
})

...and once you get past a single property, the new shorthand syntax starts to lose badly on that front.

TL;DR: it might be a corner case, but the property level factory methods seem surprising to me, and they render the "shorthand" syntax more verbose if people were to actually use them.

[markerikson]

@jmar777 : if you want to pass in a function, why not just pass it in as a normal prop? <ConnectedComponent hello={() => console.log("World")} />.

The use case for the property-level factory functions here, I think, is the same as the overall factory function capability already supported by connect(): being able to create unique selector instances per component instance. Here's the test that's added by this PR:

    it('should allow providing a factory function to mapStateToProps object', () => {
      let updatedCount = 0
      let memoizedReturnCount = 0
      const store = createStore(() => ({ value: 1 }))

      const mapStateFactory = () => {
        let lastVal, lastProp
        return (state, props) => {
          if (props.name === lastProp && lastVal === state.value) {
            memoizedReturnCount++
            return lastProp
          }
          lastVal = state.value
          return lastProp = props.name
        }
      }

      @connect({ name: mapStateFactory })
      class Container extends Component {
        componentWillUpdate() {
          updatedCount++
        }
        render() {
          return <Passthrough {...this.props} />
        }
      }

      TestUtils.renderIntoDocument(
        <ProviderMock store={store}>
          <div>
            <Container name="a" />
            <Container name="b" />
          </div>
        </ProviderMock>
      )

      store.dispatch({ type: 'test' })
      expect(updatedCount).toBe(0)
      expect(memoizedReturnCount).toBe(2)
    })

So, {name : mapStateFactory} is a selector factory function, and both instances of <Container /> get a unique selector instance. That way, they each return the exact same values as before, and the components don't re-render. (The more realistic use case would be looking up something along the lines of return state.items[ownProps.itemId].) It's not about verbosity, it's about consistent memoization per each component instance.

[slorber]

connect({
  foo: 'bar',
  hello: () => console.log('world')
})

This seems not a good idea because on every state change, the connect component would always re-render because it gets a new hello function everytime.

[markerikson]

@slorber : to be honest, I'm not even sure exactly what that syntax would do. I think, given this PR, it would be interpreted as a selector, and wind up returning a prop of {hello : undefined}.

[jmar777]

@markerikson

if you want to pass in a function, why not just pass it in as a normal prop?

Code organization / separation of concerns, primarily. Obviously depends on the specific semantics of the component and the function in question.

@slorber

This seems not a good idea because on every state change, the connect component would always re-render because it gets a new hello function everytime.

Right - this was just a minimal example that would exhibit what I believe is surprisingly behavior. The same issue would be exhibited by, e.g.:

const logWorld = () => console.log('world');

connect({ foo: 'bar', hello: logWorld });

Edit: on second thought, I'm not 100% that's true with the shorthand syntax. I'd need to trace through the PR, but it seems likely that the same function reference would be reused.

@markerikson

I think, given this PR, it would be interpreted as a selector, and wind up returning a prop of {hello : undefined}.

Correct, and that's what I think would be surprising, as returning that same object from the factory function produces different behavior:

const obj = { onClick: someHandler };

// this:
connect(obj); 

// ...behaves differently than this:

connect(() => obj);

---

At any rate, I seem to be in the minority here in expecting that objects supplied for the shorthand syntax should behave the same as objects returned from factory functions. While I appreciate the utility of property-level factory functions, inasmuch as they don't enable anything that the existing factory functions don't already enable, IMHO this just seems more likely to introduce unexpected behavior, and I'd advocate for the least surprisingly implementation.

/2-cents

[slorber]

agree with @jmar777

A simple solution that solves 90% of usecases can be merged right now without risk, until we figure out if we can support more advanced API. Honestly it's just reducing boilerplate so I'm not even sure it's so interesting to support more advanced usecases as it seems not so easy and those usecases are already enabled by more verbose syntax.

The initial simple PR I made was ready 1.5 year ago.
I feel like this feature will never get merged because the scope is too broad.

Why not reducing the scope and merging something simple now?
Is there any chance the next steps wouldn't be retrocompatible?

[markerikson]

@slorber : I already said I'm pretty much okay with the implementation and scope of this PR, but I want to see more docs and tests added around this. If you'd like to see this merged, you're welcome to help out with those.

[josepot]

@markerikson : sorry that it took me a while to get back to you on this. I've been quite busy these days. I will start working on this tomorrow... I'm hopping to address your comments by the end of this week. Thanks for your feedback!

[markusjwetzel]

@josepot : any updates on this?

[josepot]

Hi and sorry @markerikson @markusjwetzel for leaving this PR a bit stale... I've been beyond busy these last months.

So, I just did a rebase, added a new test and updated the docs to the best of my abilities. About the improvements that @markerikson said that were necessary before merging this PR. This is what I think:

I don't think we actually have any tests related to the existing object shorthand capability for mapDispatch. That seems like a problem. It would be great if you could add some in this PR, or another separate PR.

IMO that should be done in a separate PR. I would be very happy to help with that, but first I would like to know what exactly is that we want to test and to what extend. That being said, I just added an extra test that testes the basic functionality of both shorthands.

I would like to see some additional tests added that cover the object shorthand case for mapState in more detail. This PR does add a test to check the behavior of per-field factory functions in an object, but I'd appreciate having some more tests to cover the "basic" selector object behavior as well.

I don't really understand what's missing... I mean, I made sure that everything that worked with the "normal" mapState function also worked with its shorthand counterpart... That's why I created the HOF withMapStateToProps. I mean, I don't mind adding more tests, the more the merrier, for sure 😄 . That's why I added a new test... is that enough? If it's not: if you give me the descriptions of the tests that you think that are missing I will gladly implement them.

The documentation update needs to add more detail. Please describe what will happen if that object of selectors is passed in, what arguments they will be called with, and how factory selectors interact with that. We also need some examples added further down the page to demonstrate how to correctly create and pass an object of selectors as mapState.

I'm on it. It's possible that by the time you read this comment I've already taken care of that. Done. However, please bear in mind that english is not my first language. So, I will try my best but do not hesitate in correcting me. Thanks in advance.

What will this do to Flow and TypeScript type defs? I don't use either of those myself, so I generally stay out of any of the related discussions / issues / PRs. (To be honest, I don't even know whether the "official" TS and Flow typedefs are actually in the repo here, or in DefinitelyTyped / FlowTyped.)

I have no idea. But I imagine that since these changes are 100% backwards compatible they shouldn't break anything... I mean, yes, Flow and TS users won't be able to use the mapState shorthand until the definitions are updated. But do they need to be updated in this repo? Where exactly? I wouldn't mind creating a PR to FlowTyped once this is in...

Ok, lets finish this already!

[josepot]

Hi @markerikson! Just a friendly reminder that I'm waiting for your feedback here... Sorry if you find this comment annoying, it's just that I don't want this PR to become stale again. Thanks!

[markerikson]

Afraid I'm on travel at the moment, and don't have a lot of time free. I'll try to find time to look at it within the next few days.

[josepot]

Hi @markerikson ! Just another friendly reminder that I'm waiting for your feedback. Sorry if you find these comments annoying... I'm aware that you must be busy and that this is probably a low-priority PR, but as I said before: I don't want this PR to become stale again. Thanks!

[markerikson]

It's on my radar, but I'm still on business travel and don't have much spare time. I'll try to take a look at it after I get back from this trip.

[ericanderson]

connect({
  foo: 'bar',
  hello: () => console.log('world')
})

This seems not a good idea because on every state change, the connect component would always re-render because it gets a new hello function everytime.

Actually, the object, and thus the function, are created once.

@slorber : to be honest, I'm not even sure exactly what that syntax would do. I think, given this PR, it would be interpreted as a selector, and wind up returning a prop of {hello : undefined}.

From my read, it will not wrap the object because typeof { foo: 'bar' }.foo !== 'function'. See: https://github.com/reactjs/react-redux/pull/724/files#diff-ed7b415fc4798ed5c269487e2477aef2R17

I don't even recall how I stumbled upon this PR, but I am not a fan. The shorthand for mapDispatchToProps is really just a helper to bindActionCreators.

function mapDispatchToProps(dispatch) {
  return { foo: () => dispatch(foo()) };
}
// to
function mapDispatchToProps(dispatch) {
  return bindActionCreators({ foo }, dispatch);
}
// to
const mapDispatchToProps = { foo };

The analogy for props with selectors:

function mapStateToProps(state, props) {
  return { foo: getFoo(state, props) };
}
// to
function mapStateToProps(state, props) {
  return bindSelectors({ foo: getFoo }, state, props);
}
// to
const mapStateToProps = { foo: getFoo };

I have to agree with the other people on this thread about the factories. They seem to complicate things and there is no analogy for mapDispatchToProps. I think its obvious what happens when bindSelectors gets 2 or 3 arguments. However, if given one argument(bindSelectors(obj)), returned (state, props) => bindSelectors(obj, state, props), then you can fairly easily use the existing factory pattern to solve your issues:

function mapStateToProps(initialState, initialProps) {
  // do things
  return bindSelectors({ stuff });
}

This feels easier to follow. And it also has the nice property that all of these are the effectively the same:

const mapStateToProps1 = (state, props) => bindSelectors({ foo }, state, props);
const mapStateToProps2 = bindSelectors({ foo });
const mapStateToProps3 = { foo };

Which just feels nice IMHO.

[cellog]

I have to say, I am not a fan of this PR. the number of possibilities for what can be passed for mapStateToProps and mapDispatchToProps is already a bit like reading C++ operator overloading. Better would be to provide different versions of connect that encapsulate the flexibility, or a fluent interface that makes the expected behavior explicit and errors in development if the passed argument varies from what is expected

[markerikson]

I do agree that connect has gotten too heavily overloaded, and there are certainly userland ways around this (primarily use of Reselect's createStructuredSelector ).

I'll leave this PR open for a bit longer, but at the moment I'm leaning towards closing it.

[chrisjallen]

Hi @markerikson, can see feature this has been floating in and out of prs for last couple years. What is the status on this pr? It would a really beneficial feature, is there anything I can do to see this in?

cc: @gaearon, @jimbolla , @markerikson , @slorber, @timdorr

[markerikson]

I think the last comment I'd made on here was that I primarily wanted someone to contribute tests and documentation before it would be considered for merging.

However, at this point I'm feeling hesitant to accept it. As @cellog said, the internals of connect are already pretty hairy, and we've got too many overloads of behavior for connect as it is.

Oh hey, that was actually the last comment I'd made on here.

Okay. At this point, I think someone needs to give me some good reasons why this is something we really need to merge in and add to the API, otherwise I'll close it in the near future (especially since we're focusing our attention on v6 right now).

[cellog]

A quick note: in the interim, if you need this functionality, you could write a selector factory that provides it, and pass that to connect options

[timdorr]

I'll take the bullet.