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

Single createSelector function doesn't get state passed in. #1558

Closed
michalvankodev opened this issue Feb 15, 2019 · 4 comments · Fixed by #1579
Closed

Single createSelector function doesn't get state passed in. #1558

michalvankodev opened this issue Feb 15, 2019 · 4 comments · Fixed by #1579

Comments

@michalvankodev
Copy link

Hello,
This took me some time to find but I've ran into this issue.
When I have only simple selector

const somethingSelector = createSelector(
  (state: StoreState) => {
    return {
      state.something
    };
  }
);

It will not get the state passed in.

I've found that this PR: #1515 has introduced this issue.
Not very sure why the function is not called with state as an argument but it broke my application immediately.

Minimal reproduction of the bug/regression with instructions:

Expected behavior:

Nothing should be broken by a not-breaking release.
Use of createSelector without any previous selectors is broken.

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s):

Other information:

I would be willing to submit a PR to fix this issue

[ ] Yes (Assistance is provided if you need help submitting a pull request)
[ X] No. I think reverting the previous PR should be done. And a proper fix for the issue it was intented to fix

@timdeschryver
Copy link
Member

timdeschryver commented Feb 19, 2019

I think this was an undesired outcome, due to implementing the selector with props feature (more specifically to allow the createSelector with only a props selector.
If you take a look at an example from v6.0.x, the state within the selector is also
undefined - repro.

The PR #1515 reverts this change, but was indeed a breaking change.

@timdeschryver
Copy link
Member

timdeschryver commented Feb 19, 2019

With the knowledge gained, I think it wasn't a great idea to allow these selectors with props working directly with the projection function without the need of a selector.

Concrete this means that the following won't be possible anymore

const getTodosById = createSelector(
  (state: TodoAppSchema, id: number) => {
    return state.todos.find(p => p.id === id);
  }
);

But would need to be written as:

const getTodosById = createSelector(
  (state: TodoAppSchema) => state,
  (state: TodoAppSchema, id: number) => {
    return state.todos.find(p => p.id === id);
  }
);

// or 

const getTodosById = createSelector(
  (state: TodoAppSchema) => state.todos,
  (todos: Todo[], id: number) => {
    return todos.find(p => p.id === id);
  }
);

This would also be in line with the reduxjs/reselect library which has as API
createSelector(...inputSelectors | [inputSelectors], resultFunc).

This would be a breaking change that we could add for v8.

Would this be OK for you @brandonroberts ?

To clarify, this would also mean that the example snippet would also be an invalid selector and would lead to errors (as pre v6.1.2):

const somethingSelector = createSelector(
  (state: StoreState) => {
    return {
      state.something
    };
  }
);

// should be written as

const somethingSelector = createSelector(
  state => state,
  (state: StoreState) => {
    return {
      state.something
    };
  }
);

@brandonroberts
Copy link
Member

@timdeschryver I'm fine with the breaking change to make the usage more consistent.

@timdeschryver
Copy link
Member

👍 I will submit a PR

brandonroberts pushed a commit that referenced this issue Apr 1, 2019
Closes #1558 

BREAKING CHANGE:

Selectors with only a projector function aren't valid anymore.
This change will make the usage more consistent.

BEFORE:

```
const getTodosById = createSelector(
  (state: TodoAppSchema, id: number) => state.todos.find(p => p.id === id)
);
```

AFTER:

```
const getTodosById = createSelector(
  (state: TodoAppSchema) => state.todos,
  (todos: Todo[], id: number) => todos.find(p => p.id === id)
);
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants