New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Add shorthand syntax for mapStateToProps #323

Closed
wants to merge 7 commits into
base: master
from

Conversation

@slorber

slorber commented Mar 17, 2016

Hi,

This is a proposal for a shorthand syntax for connect.

I use Reselect and connect and I almost never use the props parameter in mapStateToProps.

I'd like this shorthand syntax so that instead of writing:

MenuUserPicture = connect(state => ({
    currentUser: currentUser(state)
}))(MenuUserPicture);

we could write:

MenuUserPicture = connect({currentUser})(MenuUserPicture);
@slorber

This comment has been minimized.

slorber commented Mar 18, 2016

Just to give another example from my app.
I find it annoying to have to pass the state attribute to all selectors, and also I often forget the () and write connect(state => {object}) which does not work. The shorthand syntax would permit me to not think about these things.

FolderContentContent = connect(state => ({
    selectedTags: AppCommonStore.selectedTagsSelector(state),
    folderContent: FolderContentStore.folderContentDataSelector(state),
    stamples: FolderContentStore.stamplePagesSelectors.stamples(state),
    stamplesLoading: FolderContentStore.stamplePagesSelectors.stamplesLoading(state),
    stamplesFirstPageLoaded: FolderContentStore.stamplePagesSelectors.stamplesFirstPageLoaded(state)
}))(FolderContentContent);

becomes:

FolderContentContent = connect({
    selectedTags: AppCommonStore.selectedTagsSelector,
    folderContent: FolderContentStore.folderContentDataSelector,
    stamples: FolderContentStore.stamplePagesSelectors.stamples,
    stamplesLoading: FolderContentStore.stamplePagesSelectors.stamplesLoading,
    stamplesFirstPageLoaded: FolderContentStore.stamplePagesSelectors.stamplesFirstPageLoaded
})(FolderContentContent);

But maybe this can be done outside of react-redux. As this is intended to be used with selectors maybe it is better to add this shorthand syntax to Reselect?

FolderContentContent = connect(combineSelectors({
    selectedTags: AppCommonStore.selectedTagsSelector,
    folderContent: FolderContentStore.folderContentDataSelector,
    stamples: FolderContentStore.stamplePagesSelectors.stamples,
    stamplesLoading: FolderContentStore.stamplePagesSelectors.stamplesLoading,
    stamplesFirstPageLoaded: FolderContentStore.stamplePagesSelectors.stamplesFirstPageLoaded
}))(FolderContentContent);
@gaearon

This comment has been minimized.

Contributor

gaearon commented Mar 18, 2016

Interesting. I’m not opposed to adding this as it mirrors mapDispatchToProps shortcut. Curious what others have to say.

@slorber slorber changed the title from Add shorthand syntax for mapStateToProps to [RFG] Add shorthand syntax for mapStateToProps Mar 20, 2016

@slorber slorber changed the title from [RFG] Add shorthand syntax for mapStateToProps to [RFC] Add shorthand syntax for mapStateToProps Mar 20, 2016

@slorber

This comment has been minimized.

slorber commented Mar 20, 2016

@tgriesser

This comment has been minimized.

Contributor

tgriesser commented Mar 20, 2016

I would be +1 to adding this - I agree it's a very common pattern to use and would be great to mirror mapDispatchToProps.

As for the implementation, it would be great to check whether props are needed in any of the selectors (while performing the invariant check), and if so automatically optimize by returning a factory for the selector:

connect.js:

const mapState = isPlainObject(mapStateToProps) ?
    wrapMapStateObject(mapStateToProps) :
    mapStateToProps || defaultMapStateToProps

utils/wrapMapStateObject.js:

import invariant from 'invariant'

function mapValues(obj, fn) {
  return Object.keys(obj).reduce((result, val, key) => {
    result[key] = fn(val, key)
    return result
  }, {})
}

export default function wrapMapStateObject(mapStateToProps) {

  const needsProps = Object.keys(mapStateToProps)
    .reduce((useProps, key) => {
      const type = typeof mapStateToProps[key]
      invariant(
        type === 'function',
        'mapStateToProps object key %s expected to be a function, instead saw %s',
        key,
        type
      )
      return useProps || mapStateToProps[key].length !== 1
    }, false)

  return needsProps 
    ? (state, props) => mapValues(mapStateToProps, val => val(state, props))
    : state => mapValues(mapStateToProps, val => val(state))
}
@slorber

This comment has been minimized.

slorber commented Mar 20, 2016

@tgriesser thanks for your feedback.

Yes it looks like passing props too does not cost much :)

Also I'm surprised that you comment positively on this because I've just discovered "structured selectors" of Reselect. I don't find the name very good but it was built after your issue and seems to almost do what I propose here...

What would my PR help you solve that you can't already with:
connect(createStructuredSelector({selector1, selector2}))

Not sure to understand your comment about using a factory. Is this the little thing added recently that almost nobody has to use but that can leverage better performances? ^^

@tgriesser

This comment has been minimized.

Contributor

tgriesser commented Mar 20, 2016

What would my PR help you solve that you can't already with:
connect(createStructuredSelector({selector1, selector2}))

It would just prevent the need to wrap with createStructuredSelector each time you wanted to use this pattern, along with not requiring adding the reselect library.

Is this the little thing added recently that almost nobody has to use but that can leverage better performances?

Yes, but actually now that I think about it again, this wouldn't work in this situation. It'd require the entire selector memoized at the top level, which can only happen with something like createStructuredSelector. Still would be a useful addition though I think.

@slorber

This comment has been minimized.

slorber commented Mar 23, 2016

thanks for your feedback @tgriesser

I have no strong opinion on this as the createStructuredSelector is already in Reselect and I already use it, but being directly inside Redux would be a bonus.

@gaearon tell me when you have decided, I may rework a little my PR with some code of @tgriesser before merge.

@tgriesser btw, now that you removed your optimisation that finally does not work (don't know why but I trust you), couldn't we just use mapValues(mapStateToProps, val => val(state, props)) in all cases?

@slorber slorber referenced this pull request Mar 31, 2016

Closed

Provide Selector? #5

@slorber

This comment has been minimized.

slorber commented Apr 15, 2016

@gaearon are you still interested by this PR?

Tell me and I'll update my PR (because with recent optims I now understand better code suggested by @tgriesser )

@gaearon

This comment has been minimized.

Contributor

gaearon commented Apr 15, 2016

I can’t promise to merge it but I’d very much like to see a final version of this and play with it.

@slorber

This comment has been minimized.

slorber commented Apr 23, 2016

@gaearon I've updated my PR with @tgriesser suggestions

@slorber

This comment has been minimized.

slorber commented Apr 25, 2016

People did not comment here, so comments received on twitter are:

@CyrilSilverman: the convenience gained is not worth the clarity lost.

@natenorberg: I like it. I actually keep accidentally writing it like that already

@sebas5384: I think the first choice is more easy to understand. Because you should code for other people

@jayphelps

This comment has been minimized.

jayphelps commented Apr 25, 2016

I think it's helpful to mention that this PR still allows the current form, so if you don't like it this proposed one, don't use it.

@erikras

This comment has been minimized.

Contributor

erikras commented Apr 25, 2016

👍 from me.

@OMantere

This comment has been minimized.

OMantere commented Jul 27, 2016

👍 from me too, is this being acknowledged by anyone?

foiseworth pushed a commit to foiseworth/react-redux that referenced this pull request Jul 30, 2016

@udnisap

This comment has been minimized.

udnisap commented Aug 14, 2016

We use a wrapper like that within our code so that we dont have to pass mapDispatchToProps all the time.

@slorber

This comment has been minimized.

slorber commented Oct 4, 2016

@jimbolla @gaearon @timdorr I suspect this will be doable in userland with connectAdvanced no?

@jimbolla

This comment has been minimized.

Contributor

jimbolla commented Oct 4, 2016

@slorber Yes. A couple thoughts:

  1. It's certainly implementable with connectAdvanced, and that was my intent behind extracting that method from connect, so that different custom connect APIs could be achieved in userland.
  2. There are extension points in connect that could make this achievable in userland with less work than connectAdvanced, but they are not part of the official API at this time. Exposing those extension points is more about the effort of writing tests and docs than the app code.
  3. I still think this feature would be nice as part of the official API, and I think the code is refactored well to implement this fairly easily. Again, not a lot of app code, but more work to write tests and documentation.
@slorber

This comment has been minimized.

slorber commented Oct 4, 2016

Ok so I'll let the issue open for now

Also I think it would be convenient to have a convention and have "selector" in name be automatically stripped. This would allow to write:

MenuUserPicture = connect({currentUserSelector, dataSelector})(MenuUserPicture);

and to inject {currentUser: ..., data: ...}

maybe not enough explicit but would significantly reduce boilerplate, and allow to keep "selector" in name of methods

@slorber

This comment has been minimized.

slorber commented Jan 12, 2017

@gaearon @jimbolla @tgriesser it's been a while that this PR is open and it seems I'm not the only one wanting this

Would this get merged if I port the PR to work on master, polish it a bit and add better documentation?

@jimbolla

This comment has been minimized.

Contributor

jimbolla commented Jan 12, 2017

I would like this. Can it also support "factory" style? For example:

connect({
  someThing: (intialState, initialProps) => (state, props) => value
}

I'm thinking this would be optimal for use with reselect where one will probably do:

const makeGetSomeThing = () => createSelector( ... )

connect({
  someThing: makeGetSometThing
}
@slorber

This comment has been minimized.

slorber commented Jan 13, 2017

Hi @jimbolla

For now I've not really used these factories, do you refer to this?

Because what I see as signature in doc is (dispatch => (nextState, nextOwnProps) => stateProps which does not really look like your example signature. Can you give me a doc reference? It seems to me only connectAdvanced takes a factory, and only one while your exemple assume we can provide as many factories as props we want. Can you give me a unit test that showcase this feature?

Also could we mix both styles?

const makeGetSomeThing = () => createSelector( ... )
const getCurrentUser = (state) => state.user

connect({
  someThing: makeGetSometThing,
  user: getCurrentUser,
}

I'm not sure it would be easy to detect weither makeGetSometThing is a normal selector or a factory as both are functions

@markerikson

This comment has been minimized.

Contributor

markerikson commented Jan 13, 2017

@slorber : yeah, that's what Jim is referring to. I have an example of using the factory syntax here : https://www.reddit.com/r/reactjs/comments/5dxasp/any_deepdiveadvanced_tutorials_on_reselect/

@slorber

This comment has been minimized.

slorber commented Jan 13, 2017

@markerikson I see your example but still it's different from what @jimbolla show in his code snipped

connect(makeMapState)

VS

connect({
 prop1: makeMapState1,
 prop2: makeMapState2,
])

Maybe I'm missing something, but can you give me the verbose version of the following snippet?

connect({
  someThing1: (intialState, initialProps) => (state, props) => state.something1,
  someThing2: (intialState, initialProps) => (state, props) => state.something2,
  someThing3: state => state.something2,
}
@markerikson

This comment has been minimized.

Contributor

markerikson commented Jan 13, 2017

I know. What I'm saying (and I think Jim is too) is that currently, passing an actual function as the mapState or mapDispatch arg supports the "factory function" approach. Passing an object as the mapDispatch arg currently does not support "factory functions". He's asking if your notional "pass an object for mapState would be capable of supporting a "factory function per field" approach.

@slorber

This comment has been minimized.

slorber commented Jan 14, 2017

Hmmm as far as I know if I get an object with functions as values, I can't know if the function will be a selector or a selector factory. At least I don't know how to make a distinction between (intialState, initialProps) => (state, props) => state.something1 and (state, props) => state.something1 because in both cases it's a function that takes 2 params

I took a look at current factory implementation and noticed this comment:

On first call, handles mapToProps if returns another function, and treats that
new function as the true mapToProps for subsequent calls.

This works fine because mapState is assumed to return an object, so when it does not and return a factory, we can init the selector and call it, which will end-up by returning the object we want to inject.

But in current scenario, the final selector is for a given propName, not for all the component props, so the expected value can be of any type, including functions, so it's harder to detect we are in the case of a factory (current test is typeof props === 'function').

What I mean is that if we are going to implement this feature, it will not work if the user is currently injecting functions through mapStateToProps like this:

connect(state => {
  fn: () => (...)
})

I don't really have a usecase for why a user might do something like that so please tell me if there is any?

for mapDispatchToProps it's clear that the user will want to inject functions so how can we know if that function is a factory or the function the user wanted to inject?

So, I we could support factory functions in mapState but not mapDispatch, but it requires assuming that user is not injecting functions through mapState (who's doing that anyway?)

@josepot

This comment has been minimized.

josepot commented Mar 8, 2017

Hi @slorber @markerikson and @jimbolla ,

I wasn't aware that this PR was here and I opened a similar PR that -obviously- got immediately closed by @markerikson. Sorry about that, I did check in the opened issues if there was a feature request for this and I saw nothing. I should have also checked in the opened PRs... my bad!

I would really like to see this enhancement shipped and I'm pretty sure that I know how to solve the "factory style" issue that seems to be blocking this PR. Is it cool if I solve that issue in my branch? Or should I try to add a commit to this branch?

@josepot

This comment has been minimized.

josepot commented Mar 8, 2017

Hi again @slorber @markerikson and @jimbolla !

I've added the following commit into my branch which makes the mapStateToProps object argument compatible with factory selectors as @jimbolla suggested.

I've also added the following test to make sure that it works... and it does 😄

Since this PR seems to be stale, would it be ok for me to suggest that I make the PR from my branch which also has no conflicts with master?

@slorber

This comment has been minimized.

slorber commented Mar 9, 2017

as long as it works i'm ok with that but it's not my decision to merge :)

@bstro

This comment has been minimized.

bstro commented Jun 13, 2017

Would love this right about now! :)

@markerikson

This comment has been minimized.

Contributor

markerikson commented Jun 20, 2017

@timdorr , @jimbolla , @gaearon : do we have any actual objections to this in principle, or is it just a matter of making sure it's implemented well and giving the thumbs-up? Obviously it's been sitting around for a while. We ought to either push it through, or say we're not going to go with it.

@slorber

This comment has been minimized.

slorber commented Jun 20, 2017

Hi,

Still wanting this feature and it seems I'm not alone :)

I'm free to rework the PR if needed, just ping me when the decision is made.

@jimbolla

This comment has been minimized.

Contributor

jimbolla commented Jun 20, 2017

I have no objections.

@timdorr

This comment has been minimized.

Member

timdorr commented Jun 20, 2017

I'm fine with it to. This PR just needs a rebase, since things have changed quite a bit since it was created.

slorber added a commit to slorber/react-redux that referenced this pull request Jun 20, 2017

@slorber

This comment has been minimized.

slorber commented Jun 20, 2017

The rebase was a bit complicated so I merged manually all changes to a new PR: #723

@baptistemanson

This comment has been minimized.

baptistemanson commented Aug 24, 2017

connect(map<selectors>, map<actionCreators>) seem like a good starting point for beginners. It is:

  • symmetrical,
  • uses concepts already introduced by Redux main documentation.
  • expressive and readable,
  • powerful enough (100% of our current usage on ~400 components).

It seems to be a saner starting point than connect(kindOfSelector, map<actionCreators> ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment