Skip to content
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

Accept stores as an object of prop key to store #31

Merged
merged 1 commit into from
Jun 4, 2015

Conversation

fson
Copy link
Contributor

@fson fson commented Jun 3, 2015

This makes stores and actions symmetric and lets us specify custom
prop names easily.

container decorator now takes a second argument transformProps,
which is a function that is used to transform the state and actions
props before passing them to the component. It defaults to
({ state, actions }) => ({ ...state, ...actions })

I removed some nesting from the states of counterStore and todoStore examples, because it didn't seem necessary after this change and made prop passing simpler.

Fixes #22.

This makes `stores` and `actions` symmetric and lets us specify custom
prop names easily.

`container` decorator now takes a second argument `transformProps`,
which is a function that is used to transform the state and actions
props before passing them to the component. It defaults to
`({ state, actions }) => { ...state, ...actions }`
@gaearon
Copy link
Contributor

gaearon commented Jun 4, 2015

Neat! Gonna test this tomorrow morning!

@ooflorent
Copy link
Contributor

LGTM. Maybe replacing typeof checks by isFunction would improve readability.

@fson
Copy link
Contributor Author

fson commented Jun 4, 2015

@ooflorent: Maybe we could replace those invariants altogether, with stricter propTypes?

static propTypes = {
  children: PropTypes.func.isRequired,
  actions: PropTypes.objectOf(PropTypes.func).isRequired,
  stores: PropTypes.objectOf(PropTypes.func).isRequired
}

We would not get a custom error message, but I think it would still be pretty obvious what's wrong, propTypes are the standard way to validate props after all. That approach doesn't let us validate that the object is a plain object though, unless we make a custom propType validator for it.

@ooflorent
Copy link
Contributor

@fson I haven't thought about that. Seems to be a viable option. Let's see Dan's opinion.

stores.every(s => typeof s === 'function'),
'"stores" must be an array of functions. Did you misspell an import?'
isPlainObject(stores) &&
Object.keys(stores).every(key => typeof stores[key] === 'function'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addind:

import every from 'lodash/collection/every';
import isFunction from 'lodash/lang/isFunction';

and changing the test to:

isPlainObject(stores) && every(stores, isFunction)

Would make code more readable.

@gaearon
Copy link
Contributor

gaearon commented Jun 4, 2015

I'd rather keep the invariants. Prop types are cool but out code will crash if you supply something else. The actual error will be more noticeable than propTypes warning and you might struggle longer than you have to.

@gaearon
Copy link
Contributor

gaearon commented Jun 4, 2015

Although I sure don't mind stricter propTypes. Just want to also have invariants instead of weird crashes if user doesn't notice the warning.

@gaearon
Copy link
Contributor

gaearon commented Jun 4, 2015

This is awesome. Thanks to everybody for a thoughtful discussion.

gaearon added a commit that referenced this pull request Jun 4, 2015
Accept stores as an object of prop key to store
@gaearon gaearon merged commit 3e299c5 into reduxjs:master Jun 4, 2015
@fson fson deleted the transform-props branch June 4, 2015 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants