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

feat: allow properties in selectors #1194

Merged
merged 2 commits into from
Jul 27, 2018

Conversation

timdeschryver
Copy link
Member

This allows to provide provide extra properties inside selectors.

const selector = createSelector(
  state => state.count * 10,
  (state, props) => state.count + props.value,
  (one, two) => one + two
);

const result = selector(
  {
    count: 10,
  },
  { value: 47 }
);

// Result = 157.

If we agree on this I can add a commit to enhance the docs.

Closes #1152.

@coveralls
Copy link

coveralls commented Jul 23, 2018

Coverage Status

Coverage increased (+0.02%) to 88.359% when pulling 2cefab4 on timdeschryver:pr/selector-with-properties into 22284ab on ngrx:master.

}
export type Selector<State, Result> = (state: State) => Result;

export type SelectorWithProperty<State, Property, Result> = (
Copy link
Member

Choose a reason for hiding this comment

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

Property should be plural because we're passing a set of them. Props or Properties?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would say Props?

@@ -123,6 +123,106 @@ describe('Selectors', () => {
});
});

describe('createSelector with props', () => {
Copy link
Member

Choose a reason for hiding this comment

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

We need some integration tests using store.select also.

Copy link
Member Author

@timdeschryver timdeschryver Jul 26, 2018

Choose a reason for hiding this comment

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

Thanks for the input!
Creating this test made me realize we also have to change the select function.
Doing this I bumped into a small problem, see my comment in the following commit.

Copy link
Member

Choose a reason for hiding this comment

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

I think only having props for selector functions is reasonable.

let mapped$: Observable<any>;

if (typeof pathOrMapFn === 'string') {
mapped$ = source$.pipe(pluck(pathOrMapFn, ...paths));
mapped$ =
typeof props === 'string'
Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can't support having props on a string selector e.g. this.store.select('counter1'), right?

Copy link
Member

Choose a reason for hiding this comment

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

Right

@timdeschryver
Copy link
Member Author

I just did a rebase and some of the tests are failing, let me fix these first.

@timdeschryver timdeschryver force-pushed the pr/selector-with-properties branch from 12faaea to 6991562 Compare July 27, 2018 11:05
@timdeschryver
Copy link
Member Author

timdeschryver commented Jul 27, 2018

I think I found a bug in the existing selector.

This fails, because it gets called 3 times

it('should memoize the function', () => {
      const firstState = { first: 'state' };
      const secondState = { second: 'state' };
      const projectFn = jasmine.createSpy('projectionFn');
      const selector = createSelector(
        incrementOne,
        incrementTwo,
        incrementThree,
        projectFn
      );

      selector(firstState);
      selector(firstState);
      selector(firstState);
      selector(secondState);
      selector(secondState); // if I add this line the test fails, I would expect it to succeed?

      expect(incrementOne).toHaveBeenCalledTimes(2);
      expect(incrementTwo).toHaveBeenCalledTimes(2);
      expect(incrementThree).toHaveBeenCalledTimes(2);
      expect(projectFn).toHaveBeenCalledTimes(2);
});

This seems related to #1175, before the above would only be called 2 times.

return createSelectorFactory(defaultMemoize)(...input);
}

export function defaultStateFn(
state: any,
selectors: Selector<any, any>[],
props: any,
selectors: any[],
Copy link
Member

Choose a reason for hiding this comment

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

Don't remove the type if possible

Copy link
Member Author

@timdeschryver timdeschryver Jul 27, 2018

Choose a reason for hiding this comment

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

I think this isn't possible because we're adding the props when we invoke the selector.
fn(state, props)

Copy link
Member

Choose a reason for hiding this comment

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

For selectors?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, we're invoking every selector with state and props.

The selector type only accepts one argument, which is the state.

return createSelectorFactory(defaultMemoize)(...input);
}

export function defaultStateFn(
state: any,
selectors: Selector<any, any>[],
Copy link
Member

Choose a reason for hiding this comment

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

Move props below selectors

const memoizedState = defaultMemoize(function(state: any) {
return options.stateFn.apply(null, [state, selectors, memoizedProjector]);
const memoizedState = defaultMemoize(function(state: any, props: any) {
// createSelector works directly on state
Copy link
Member

Choose a reason for hiding this comment

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

Is this related or a different feature? Feels like this should be a separate PR

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought you meant this with your comment "I think only having props for selector functions is reasonable."
But you probably meant:

createSelector(
   getX,
   (props) => props...
)

Is this possible to do? Or do shouldn't we pass the state to the 'props selectors'?

Copy link
Member

Choose a reason for hiding this comment

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

What I meant was we don't need to try to support using string selectors with props like below.

select('something', 5);

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah gotcha 😅 !
Do you think having the option to create a selector solely based on props would be worth it?

createSelector((state, props) => ...)

If so I can pull out the commit out of this PR and create a new one, or I could clean up these commits into 2 commits.

Copy link
Member

Choose a reason for hiding this comment

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

2 commits is fine

pathOrMapFn: ((state: T) => any) | string,
): (source$: Observable<State>) => Observable<Result>;
export function select<State, Props, K>(
pathOrMapFn: (state: State, props?: Props) => any | string,
Copy link
Member

Choose a reason for hiding this comment

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

(state ...) => any should be wrapped in parenthesis

let mapped$: Observable<any>;

if (typeof pathOrMapFn === 'string') {
mapped$ = source$.pipe(pluck(pathOrMapFn, ...paths));
const pathSlices = [<string>props, ...paths].filter(Boolean);
mapped$ = source$.pipe(pluck(pathOrMapFn, ...pathSlices));
Copy link
Member

Choose a reason for hiding this comment

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

Don't pass in props with a string selector

Copy link
Member Author

Choose a reason for hiding this comment

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

We have to, because when you're using strings to get a piece of your store state, the first property (foo in the example below) will be passes as the Prop value. If this isn't done, it will try to access the property as state.bar.baz instead of state.foo.bar.baz.

this.store.select('foo', 'bar', baz')

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. Just remove the filter then

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't if we only pass one path, the props value is undefined, we have to filter this one out.

this.store.select('foo')

Copy link
Member

Choose a reason for hiding this comment

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

Right 👍

} else if (typeof pathOrMapFn === 'function') {
mapped$ = source$.pipe(map(pathOrMapFn));
mapped$ = source$.pipe(map(source => pathOrMapFn(source, <Props>props)));
Copy link
Member

Choose a reason for hiding this comment

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

Why do props need to be typed here?

Copy link
Member Author

@timdeschryver timdeschryver Jul 27, 2018

Choose a reason for hiding this comment

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

Because the type is defined as Props | string. (it is a Prop if we use a selector, and it is a string when we select a piece of store state based on the property names)
Therefor I had to cast it to Props.

export function select<T, K>(
mapFn: (state: T) => K
): (source$: Observable<T>) => Observable<K>;
export function select<State, Result, Props>(
Copy link
Member

Choose a reason for hiding this comment

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

Leave the types as T, K in this PR. Its confusing to switch between T, K and State, Result

const memoizedState = defaultMemoize(function(state: any, props: any) {
// createSelector works directly on state
// e.g. createSelector((state, props) => ...)
if (selectors.length === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed in this PR?

Copy link
Member Author

@timdeschryver timdeschryver Jul 27, 2018

Choose a reason for hiding this comment

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

It is, this will be part of the second commit to allow createSelector((state, props) => ...).
This is needed because createSelector only has one argument which is the projection function (there are no selectors). Meaning the selectors would be empty, thus our projector wouldn't get invoked.

@@ -33,6 +33,9 @@ export interface StoreFeature<T, V extends Action = Action> {
metaReducers?: MetaReducer<T, V>[];
}

export interface Selector<T, V> {
Copy link
Member

Choose a reason for hiding this comment

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

Leave as T, V in this PR.

@brandonroberts
Copy link
Member

We don't want to change the tests to match our intended result. If #1175 is related, lets figure out why it causes the tests to fail.

@timdeschryver timdeschryver force-pushed the pr/selector-with-properties branch 2 times, most recently from 8273abe to b5d588d Compare July 27, 2018 17:51

Verified

This commit was signed with the committer’s verified signature.
alexsapran Alexandros Sapranidis
@timdeschryver timdeschryver force-pushed the pr/selector-with-properties branch 2 times, most recently from 61a5307 to 23ed4c1 Compare July 27, 2018 18:36

Verified

This commit was signed with the committer’s verified signature.
alexsapran Alexandros Sapranidis
@timdeschryver timdeschryver force-pushed the pr/selector-with-properties branch from 23ed4c1 to 2cefab4 Compare July 27, 2018 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants