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

Allow Selector to accept arguments besides state #1152

Closed
tsugitta opened this issue Jun 25, 2018 · 13 comments
Closed

Allow Selector to accept arguments besides state #1152

tsugitta opened this issue Jun 25, 2018 · 13 comments

Comments

@tsugitta
Copy link

tsugitta commented Jun 25, 2018

Thanks for this awesome library!

With Reselect, we can call a selector with arguments like below (almost copied from Reselect's README):

import { createSelector } from 'reselect'

const getVisibilityFilter = (state, props) =>
  state.todoLists[props.listId].visibilityFilter

const getTodos = (state, props) =>
  state.todoLists[props.listId].todos

const getVisibleTodos = createSelector(
  [ getVisibilityFilter, getTodos ],
  (visibilityFilter, todos) => {
    switch (visibilityFilter) {
      case 'SHOW_COMPLETED':
        return todos.filter(todo => todo.completed)
      case 'SHOW_ACTIVE':
        return todos.filter(todo => !todo.completed)
      default:
        return todos
    }
  }
)

const todos = getVisibleTodos(state, props)

but ngrx's selector does not seem to have such a feature. It will be helpful for me if it's possible.

Other information:

In the above example of Reselect, the props is not used in createSelector, but actually it also can be used like below:

import { createSelector } from 'reselect'

const shootingById = createSelector(
  [shootings,(state, shootingId) => shootingId],
  (shootings, shootingId) => head(shootings.filter(s => s._id === shootingId))
);

const shootings = shootingById(state, someId)

ref: reduxjs/reselect#272 (comment)

If accepted, I would be willing to submit a PR for this feature

[x] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No

@brandonroberts
Copy link
Member

Selectors can already be created using arrays. See https://github.com/ngrx/platform/blob/master/modules/store/spec/selector.spec.ts#L126

@tsugitta
Copy link
Author

tsugitta commented Jun 26, 2018

@brandonroberts
The point I wanted to say is not passing multiple selectors to createSelector, but passing arguments besides State (props in the example) to a selector created by createSelector.

const getUser = (state, id) => {
  return state.userLists[id]
}

const user = getUser(state, someId);

in this example, the selector function getUser cannot be created with ngrx's createSelector.

@tsugitta
Copy link
Author

@brandonroberts
Hi, I would be grateful if you could reply.

@brandonroberts
Copy link
Member

What's your proposed change?

@tsugitta
Copy link
Author

tsugitta commented Jul 14, 2018

@brandonroberts
I've assumed these.
https://github.com/reduxjs/reselect/blob/master/test/test_selector.js#L141-L153
https://github.com/reduxjs/reselect/blob/master/test/test_selector.js#L201-L219

and update Selector interface like below:

interface Selector<T, V, P = {}> {
  (state: T, props?: P): V;
}

@timdeschryver
Copy link
Member

If this is something we want, I can provide a PR.

@brandonroberts
Copy link
Member

Seems reasonable to me. Let @tsugitta put in the PR if they still want to work this change.

@tsugitta
Copy link
Author

thanks, let me try to submit the PR.

@timdeschryver
Copy link
Member

I got one ready tho 😅
@tsugitta you can checkout the commit if this is what you want you can let me know and I'll open a PR. If not you can submit a PR.

@tsugitta
Copy link
Author

@timdeschryver
seems great, please open a PR.

@Abhay-Joshi-Git
Copy link

Does documentation of the selectors reflect this change ? I tried to check there but couldn't find it out.

@timdeschryver
Copy link
Member

It's documented at https://ngrx.io/guide/store/selectors#using-selectors-with-props @Abhay-Joshi-Git . Are the docs missing something?

@Abhay-Joshi-Git
Copy link

ok, thanks for sharing this.

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

No branches or pull requests

4 participants