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

api changes, thunks support added #103

Merged
merged 18 commits into from
Jan 26, 2016
Merged

api changes, thunks support added #103

merged 18 commits into from
Jan 26, 2016

Conversation

jsdmc
Copy link
Contributor

@jsdmc jsdmc commented Jan 26, 2016

@erikras Breaking change is only for mapStateToProps(key, state, ownProps). Now it's first argument is a multireducerKey passed to component. We can keep old signature and access multireducerKey from ownProps, but as you mentioned earlier, react-redux's connect makes some optimizations - thus it's better not to use ownProps if it's not used. Also now whole state passed to mapStateToProps. I think it's more flexible way. You can still grab needed state slice using key param.
@adailey14 Please take a look as there is a bunch of your changes)

@jsdmc jsdmc mentioned this pull request Jan 26, 2016
@jsdmc jsdmc changed the title api changes, thunks support added api changes, thunks support added (https://github.com/erikras/multireducer/issues/9) Jan 26, 2016
@jsdmc jsdmc changed the title api changes, thunks support added (https://github.com/erikras/multireducer/issues/9) api changes, thunks support added (#9) Jan 26, 2016
@jsdmc jsdmc changed the title api changes, thunks support added (#9) api changes, thunks support added Jan 26, 2016
@erikras
Copy link
Owner

erikras commented Jan 26, 2016

To get the build to work on Node 0.12, please change

action.type.includes(key)

to

~action.type.indexOf(key)

on multireducer.js:17. Such a great js hack, that one. :-)

@jsdmc
Copy link
Contributor Author

jsdmc commented Jan 26, 2016

@erikras fixed. Relied on babel here) Expected includes to work after transpiling. The same issue with find method of Arrays.

erikras added a commit that referenced this pull request Jan 26, 2016
api changes, thunks support added. fixes #9
@erikras erikras merged commit 0111f2f into erikras:master Jan 26, 2016
multireducerBind(actions, multireducerKey)
wrapMapStateToProps(mapStateToProps, multireducerKey),
wrapMapDispatchToProps(mapDispatchToProps, multireducerKey),
...rest
Copy link
Owner

Choose a reason for hiding this comment

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

@jsdmc Somewhere in here, you lost the BUT WITH ONE DIFFERENCE logic that was slicing up the state to only the slice that the reducer wants. Does that make sense?

Copy link
Owner

Choose a reason for hiding this comment

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

Or am I expected, as the user of the library, to slice my own state: state.multireducer[key] in mapStateToProps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erikras Oh, sorry if described it not clear. But yes you need to use that key to access state slice. Yeah, it's not that convenient as in previous version, but at the other hand you have access to app state.

Copy link
Owner

Choose a reason for hiding this comment

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

Okay. Can you do another PR to fix the docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erikras done

@danosaure
Copy link

This change kind of break encapsulation. I was using multireducer to not need to know where in the tree the "sub-state" is, now, it seems that I need to be aware of the whole state.

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