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.

Sign up for GitHub

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

fix(Store): selector with only a projector #1579

Merged
merged 2 commits into from
Apr 1, 2019
Merged

fix(Store): selector with only a projector #1579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3447f87
fix(Store): selector with only a projector
timdeschryver Feb 25, 2019
26eab59
Merge branch 'master' into pr/selector-projection
brandonroberts Apr 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 3 additions & 78 deletions modules/store/spec/integration.spec.ts
View file
Open in desktop
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
Open in desktop
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', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This test was to verify the fix for #1501.
Since we don't short circuit anymore, this test became obsolete.

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
Open in desktop
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