Skip to content

Commit

Permalink
fix(Store): selector with only a projector (#1579)
Browse files Browse the repository at this point in the history 
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)
);
```
  • Loading branch information
@timdeschryver @brandonroberts
timdeschryver authored and brandonroberts committed Apr 1, 2019
1 parent 5581826 commit da1ec80
Show file tree
Hide file tree
Showing 3 changed files with 3 additions and 165 deletions.
81 changes: 3 additions & 78 deletions modules/store/spec/integration.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,8 +179,9 @@ describe('ngRx Integration spec', () => {

it('should use props to get a todo', () => {
const getTodosById = createSelector(
(state: TodoAppSchema, id: number) => {
return state.todos.find(p => p.id === id);
(state: TodoAppSchema) => state.todos,
(todos: Todo[], id: number) => {
return todos.find(p => p.id === id);
}
);

Expand Down Expand Up @@ -252,46 +253,6 @@ describe('ngRx Integration spec', () => {
payload: { id: 2 },
});
});

it('should use the props in the projector to get a todo', () => {
const getTodosState = createFeatureSelector<TodoAppSchema, Todo[]>(
'todos'
);

const getTodosById = createSelector(
getTodosState,
(todos: Todo[], { id }: { id: number }) =>
todos.find(todo => todo.id === id)
);

let testCase = 1;
const todo$ = store.pipe(select(getTodosById, { id: 2 }));
todo$.subscribe(todo => {
if (testCase === 1) {
expect(todo).toEqual(undefined);
} else if (testCase === 2) {
expect(todo).toEqual({
id: 2,
text: 'second todo',
completed: false,
});
} else if (testCase === 3) {
expect(todo).toEqual({
id: 2,
text: 'second todo',
completed: true,
});
}
testCase++;
});

store.dispatch({ type: ADD_TODO, payload: { text: 'first todo' } });
store.dispatch({ type: ADD_TODO, payload: { text: 'second todo' } });
store.dispatch({
type: COMPLETE_TODO,
payload: { id: 2 },
});
});
});

describe('using the select operator', () => {
Expand Down Expand Up @@ -357,42 +318,6 @@ describe('ngRx Integration spec', () => {
expect(currentlyVisibleTodos.length).toBe(0);
});

it('should use props to get a todo', () => {
const getTodosById = createSelector(
(state: TodoAppSchema, id: number) => {
return state.todos.find(p => p.id === id);
}
);

let testCase = 1;
const todo$ = store.pipe(select(getTodosById, 2));
todo$.subscribe(todo => {
if (testCase === 1) {
expect(todo).toEqual(undefined);
} else if (testCase === 2) {
expect(todo).toEqual({
id: 2,
text: 'second todo',
completed: false,
});
} else if (testCase === 3) {
expect(todo).toEqual({
id: 2,
text: 'second todo',
completed: true,
});
}
testCase++;
});

store.dispatch({ type: ADD_TODO, payload: { text: 'first todo' } });
store.dispatch({ type: ADD_TODO, payload: { text: 'second todo' } });
store.dispatch({
type: COMPLETE_TODO,
payload: { id: 2 },
});
});

it('should use the selector and props to get a todo', () => {
const getTodosState = createFeatureSelector<TodoAppSchema, Todo[]>(
'todos'
Expand Down
56 changes: 0 additions & 56 deletions modules/store/spec/selector.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,17 +143,6 @@ describe('Selectors', () => {
});
});

it('should not short circuit to the projector fn if there are no selectors and props', () => {
const projectFn = jasmine.createSpy('projectionFn');
const state = { counter: {} };

const selector = (createSelector(projectFn) as any)(state);

// the projector still fires but without arguments,
// this because there are no selectors and props
expect(projectFn).toHaveBeenCalledWith();
});

it('should be possible to test a projector fn independent from the selectors it is composed of', () => {
const projectFn = jasmine.createSpy('projectionFn');
const selector = createSelector(
Expand Down Expand Up @@ -239,51 +228,6 @@ describe('Selectors', () => {
});
});

describe('createSelector with only a props selector', () => {
it('should deliver the state and the props to the projection function', () => {
const projectFn = jasmine.createSpy('projectionFn');
const state = { counter: {} };
const props = { value: 47 };
const selector = createSelector(projectFn)(state, props);
expect(projectFn).toHaveBeenCalledWith(state, props);
});

it('should be possible to use a projector fn', () => {
const projectFn = jasmine.createSpy('projectionFn');
const selector = createSelector(projectFn);
selector.projector('foo', 'bar');
expect(projectFn).toHaveBeenCalledWith('foo', 'bar');
});

it('should call the projector function when the state changes', () => {
const projectFn = jasmine.createSpy('projectionFn');
const selector = createSelector(projectFn);

const firstState = { first: 'state' };
const secondState = { second: 'state' };
const props = { foo: 'props' };
selector(firstState, props);
selector(firstState, props);
selector(secondState, props);
expect(projectFn).toHaveBeenCalledTimes(2);
});

it('should allow you to release memoized arguments', () => {
const state = { first: 'state' };
const props = { first: 'props' };
const projectFn = jasmine.createSpy('projectionFn');
const selector = createSelector(projectFn);

selector(state, props);
selector(state, props);
selector.release();
selector(state, props);
selector(state, props);

expect(projectFn).toHaveBeenCalledTimes(2);
});
});

describe('createSelector with arrays', () => {
it('should deliver the value of selectors to the projection function', () => {
const projectFn = jasmine.createSpy('projectionFn');
Expand Down
31 changes: 0 additions & 31 deletions modules/store/src/selector.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -472,31 +472,6 @@ export function createSelector<
) => Result
): MemoizedSelectorWithProps<State, Props, Result>;

/**
* @deprecated
* Selectors with only a projector function aren't valid anymore and will be removed in version 8.0.0
*
* BEFORE:
*
* ```ts
* const getTodosById = createSelector(
* (state: TodoAppSchema, id: number) => state.todos.find(p => p.id === id)
* );
* ```
*
* AFTER:
*
* ```ts
* const getTodosById = createSelector(
* (state: TodoAppSchema) => state.todos,
* (todos: Todo[], id: number) => todos.find(p => p.id === id)
* );
* ```
*/
export function createSelector<State, Props, Result>(
projector: SelectorWithProps<State, Props, Result>
): MemoizedSelectorWithProps<State, Props, Result>;

export function createSelector(
...input: any[]
): Selector<any, any> | SelectorWithProps<any, any, any> {
Expand Down Expand Up @@ -570,12 +545,6 @@ export function createSelectorFactory(
});

const memoizedState = defaultMemoize(function(state: any, props: any) {
// createSelector works directly on state
// e.g. createSelector((state, props) => ...)
if (selectors.length === 0 && props !== undefined) {
return projector.apply(null, [state, props]);
}

return options.stateFn.apply(null, [
state,
selectors,
Expand Down

0 comments on commit da1ec80

Please sign in to comment.