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

Selector proposal #169

Closed
wants to merge 5 commits into from
Closed

Conversation

speedskater
Copy link

Hi,

Our proposal for issue 47. It should support the same functionality as nuclear-js getters.

Robert

@@ -6,3 +6,4 @@ lib
coverage
react.js
react-native.js
.idea
Copy link
Contributor

Choose a reason for hiding this comment

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

For non-project specific things like this, you might consider a global gitignore: https://help.github.com/articles/ignoring-files/#create-a-global-gitignore

Copy link
Author

Choose a reason for hiding this comment

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

removed

expect(arrayEqual(array, undefined)).toBe(false);
});
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

@speedskater can you check your editor settings? It's best to end files w/ newlines. There are a few files in here that don't have them.

Copy link
Author

Choose a reason for hiding this comment

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

New lines fixed.

render() {
return (
//use your predefined selectors
<Connector select={state => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

What if select actually took selectors in the same format as nuclear-js getters, that is an array. So this would be:

select={['todos', todos => todos.length]}

You could still import an array selector from somewhere else, and testing it would require you to call createSelector, but it would allow the individual connector to worry about memoziation (not sure if this is better or not...)

Copy link
Contributor

Choose a reason for hiding this comment

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

@gaearon something like this would require some sort of support in redux itself, which could just be something along the lines of connector/select middleware

Copy link
Author

Choose a reason for hiding this comment

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

Our decion was that a selector is a function which translates an input state into an output representation. The best solution would be to just combine the selectors as plain functions.

let todoSelector = state => state.todos;
let todoCountSelector = state => todoSelector(state).length;

The main drawback in this case is that you can only memoize on individual function parameters not on the parts of the state which are used in the selector itself.
To preserve functional semantics the construct using the createSelector was chosen in favor of passing arrays around.

The reason that it was chosen to make memoization explicit was to make it a decision to the user wether she wants to have a material view on her state or not.

@gaearon
Copy link
Contributor

gaearon commented Jun 23, 2015

This is interesting. We need to choose what we want to support and what we want to push into userland, and whether we want to have “blessed” userland packages.

If @acdlite, @faassen, @goatslacker could give some feedback on what they think what core/non-core distinction would be appropriate, that would be awesome. Personally, I'm leaning to create a Github org and have a few supported add-ons like this one that would be versioned separately.

@aaronjensen
Copy link
Contributor

nuclear-js keypaths can be arrays too, so you can do something like:

// state: { foo: { bar: 1 }, baz: { fab: 2 } }
[['foo', 'bar'], ['baz', 'fab'], (foobar, bazfab) => ...]

@leoasis
Copy link
Contributor

leoasis commented Jun 23, 2015

I agree with @aaronjensen that there needs to be some kind of Connector level memoization. Say you generally use the same selector in two components but with different parameters, then you won't be making use of the cached results since one will be invalidating the other.

@speedskater
Copy link
Author

@leoasis That's true but isn't it a different selector in this case?
Because both selectors operate on the same global state. Either it is the same or not.

@leoasis
Copy link
Contributor

leoasis commented Jun 23, 2015

Yeah but I was talking mainly about parametrized selectors. Something like isTodoCompleted(todoId)

If I use createBuffered to define that selector, it will get invalidated by different todoIds

@speedskater
Copy link
Author

@leoasis That's true but i would argue that this still can be solved using the proposed selector approach.
If todoId is part of the central state, there must be a selector available selecting it.
In case the selector depends on a component property it would have to be composed in the component itself or created on the fly as an input of the selector depends on the component property instead of the central state.

createSelector(todoSelector, state => todoId, createBuffered(isTodoCompleted));

It could furthermore be argued, that it isn't a selector at all as it is not a pure function on the central redux state.

@leoasis
Copy link
Contributor

leoasis commented Jun 23, 2015

Yeah i guess you're right, makes sense.

@faassen
Copy link

faassen commented Jun 24, 2015

I started to write a lot of stuff on extensions and core, but that's going to be rather long so I may write a blog entry about that sometime. A separate github org with core and extensions in it makes sense. There is some expectation that the maintainers of the core care about the extensions the organization. There's no need to go for an extension just so you get modularity, though.

Oh and don't create some kind of contrib or addons directory in your package as that's the worst of both worlds. See here http://blog.startifact.com/posts/against-contrib.html

@gaearon
Copy link
Contributor

gaearon commented Jul 6, 2015

Hi again!

I don't currently plan to bring it as is to Redux, but I asked @faassen and @ellbee to play around with it, and after tweaking the API, they came up with reselect. I think it solves the same problem as this PR, so I'm closing it in favor of reselect.

Thanks again for your PR, as it served as the starting point for reselect.

@gaearon gaearon closed this Jul 6, 2015
@speedskater
Copy link
Author

Nice that @faassen and @ellbee have taken over. But imho a comment to our original source code would have been fair.

@ellbee
Copy link
Contributor

ellbee commented Jul 6, 2015

@speedskater I absolutely agree. We'll get that sorted out today. Sorry.

cc @faassen

@speedskater
Copy link
Author

@ellbee thanks :). And nice work that you streamlined the selector approach into a pure functional one.

@faassen
Copy link

faassen commented Jul 6, 2015

@speedskater I agree as well you deserve a lot of the credit: we basically just looked at your code and refactored it a bit. We discussed adding credits in the codebase already, but we did this as the hackathon before React Europe and after that there was the conference, so we had little chance for followup yet.

@faassen
Copy link

faassen commented Jul 6, 2015

@speedskater, @ellbee already added your github nick to the README. I now also added a CREDITS file to track contributions. I've used your github nick as I don't know your real name; if you want me to put in your real name let me know.

@speedskater
Copy link
Author

Thanks for adding. Regarding the realnames could you please add my realname Robert Binna and the realname of my coworker Philip Spitzlinger to the credits as the initial draft was the result of a pair programming session.

@faassen
Copy link

faassen commented Jul 7, 2015

Ah, I see someone already added the real names to CREDITS. Great!

@clearjs
Copy link
Contributor

clearjs commented Jul 7, 2015

Formally, you'd also need to agree on the license (MIT?).

@faassen
Copy link

faassen commented Jul 7, 2015

Ah, good point. I've just added a MIT license. Thanks!

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.

7 participants