From da1ec80c05e6819f6d3148b81a115cbc3c7c4311 Mon Sep 17 00:00:00 2001 From: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com> Date: Mon, 1 Apr 2019 15:41:59 +0200 Subject: [PATCH] fix(Store): selector with only a projector (#1579) 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) ); ``` --- modules/store/spec/integration.spec.ts | 81 +------------------------- modules/store/spec/selector.spec.ts | 56 ------------------ modules/store/src/selector.ts | 31 ---------- 3 files changed, 3 insertions(+), 165 deletions(-) diff --git a/modules/store/spec/integration.spec.ts b/modules/store/spec/integration.spec.ts index 18120b42af..a1b0885755 100644 --- a/modules/store/spec/integration.spec.ts +++ b/modules/store/spec/integration.spec.ts @@ -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); } ); @@ -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( - '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', () => { @@ -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( 'todos' diff --git a/modules/store/spec/selector.spec.ts b/modules/store/spec/selector.spec.ts index cc0c176abb..cd6559eb87 100644 --- a/modules/store/spec/selector.spec.ts +++ b/modules/store/spec/selector.spec.ts @@ -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( @@ -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'); diff --git a/modules/store/src/selector.ts b/modules/store/src/selector.ts index 332d3d01d0..b369e0cef1 100644 --- a/modules/store/src/selector.ts +++ b/modules/store/src/selector.ts @@ -472,31 +472,6 @@ export function createSelector< ) => Result ): MemoizedSelectorWithProps; -/** - * @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( - projector: SelectorWithProps -): MemoizedSelectorWithProps; - export function createSelector( ...input: any[] ): Selector | SelectorWithProps { @@ -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,