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

WIP Selectors #22

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

WIP Selectors #22

wants to merge 3 commits into from

Conversation

stasm
Copy link
Owner

@stasm stasm commented Sep 20, 2017

Here's a WIP of my proposal to fix #19. I still need to add more tests and fix the failing ones. In the interest of gathering feedback early I'm submitting the PR now.

I split the change into three commits and it might be helpful to review them one by one.

@stasm stasm mentioned this pull request Sep 20, 2017
Copy link
Contributor

@bcruddy bcruddy left a comment

Choose a reason for hiding this comment

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

I'll give this a closer look in the morning but at first glance this looks good. An example using combineReducers would be nice but certainly not necessary in this PR.

CHANGELOG.md Outdated

You can now pass an optional selector function to `connect`. It will be
passed the `state` and should return an object which will be merged with
the component's `props` using `Object.assign({}, props, substate`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a closing )

@stasm
Copy link
Owner Author

stasm commented Sep 21, 2017

I'll give this a closer look in the morning but at first glance this looks good. An example using combineReducers would be nice but certainly not necessary in this PR.

Thanks. I'll add an example and tests for combineReducers tonight or tomorrow. I also need to change the tests for connect. The ones I added in the first commit of this PR fail because the second commit changed the expected signature of connected components.


You can now pass an optional selector function to `connect`. It will be
passed the `state` and should return an object which will be merged with
the component's `props` using `Object.assign({}, props, substate)`.
Copy link
Owner Author

Choose a reason for hiding this comment

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

The order of first props then substate mimics what Redux does. I've wondered why they implement it this way. substate, props would allow overriding the props from state which sounds like it could be useful sometimes.

Copy link
Contributor

@bcruddy bcruddy Sep 21, 2017

Choose a reason for hiding this comment

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

I definitely think this makes more sense but I am curious as to what trade off the redux team made when they designed it the other way. There has to be a reason behind it.

@bcruddy
Copy link
Contributor

bcruddy commented Sep 21, 2017

I also need to change the tests for connect. The ones I added in the first commit of this PR fail because the second commit changed the expected signature of connected components.

Once this is fixed 👍

@@ -44,9 +44,10 @@ export function createStore(reducer) {
roots.set(root, component);
render();
},
connect(component) {
connect(component, selector = state => state) {
Copy link
Contributor

Choose a reason for hiding this comment

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

just a nit / question -- is your preference to keep the selector as the second arg, vs the react-redux style

const ConnectedFoo = connect(state => ({ bar: state.foo.bar }))(Foo);

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess (to kinda answer my own question) your way allows connecting to the state without any selectors.

Copy link
Contributor

@bsouthga bsouthga Sep 21, 2017

Choose a reason for hiding this comment

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

which would need to be something like the following in react-redux style,

const ConnectedFoo = connect()(Foo);

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.

Add combineReducers
4 participants