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

🚀[FEATURE]: Provide state slices as selectors out-of-the-box #1653

Closed
Carniatto opened this issue Aug 6, 2020 · 24 comments
Closed

🚀[FEATURE]: Provide state slices as selectors out-of-the-box #1653

Carniatto opened this issue Aug 6, 2020 · 24 comments

Comments

@Carniatto
Copy link
Contributor

Relevant Package

This feature request is for @ngxs/store

Description

Provide state slices as selectors out-of-the-box

Describe the problem you are trying to solve

Please describe the problem. If possible please substantiate it with the use cases you want to address.

When working with ngxs selectors is common t have basic selectors just to slice the basic properties of your state like shown below:

export class TodoStateQueries {
  @Selector([TodoState])
  static selected(state: TodoStateModel) {
    return state.selected;
  }

  @Selector([TodoState])
  static todos(state: TodoStateModel) {
    return state.todos;
  }
}

This can become a tedious process. There is a possibility to improve this using dynamic selectors:

export class TodoStateQueries {
  static getSlice(property: keyof TodoStateModel) {
    return createSelector([TodoState], (state: TodoStateModel) => state[property]);
  }

  // usage
  @Selector([TodoStateQueries.getSlice('selected'), TodoStateQueries.getSlice('todos')])
  static getSelectedTodoos...

}

This is a better approach but been a very common use case would be nice to have something like this out-of-the-box.

Describe the solution you'd like

A possible solution would be to provide a function createPropertySelectors that would split the state in selectors or each property like shown below:

export const TodoStateProps = createPropertySelectors<TodoStateModel>(TodoState)

@Selector([TodoStateProps.selectedIds, TodoStateProps.todos)])
static getSelectedTodos....

Describe alternatives you've considered

There are other alternatives provided by @poloagustin and @joaqcid respectivly

import {getStateSlice} from '@ngxs/store'

@Selector([getStateSlice(AppState, 'users')])
static getUsers....
@joaqcid
Copy link
Contributor

joaqcid commented Aug 6, 2020

last solution can also be

const CounterStateProps = createPropertySelector<CounterStateModel>(CounterState);

@Selector([CounterStateProps('count'), CounterStateProps('loaded')]) static combined(
    count: number,
    loaded: boolean
  ) {
    return loaded && count;
  }

@armanozak
Copy link
Contributor

Hi @Carniatto, @joaqcid,

Take a look at this playground. When every state has its own slicer, you no longer have a slicing problem. ;)

Have a cheerful day.

@joaqcid
Copy link
Contributor

joaqcid commented Aug 7, 2020

that's another interesting way of doing it, we thought of using keyof to select the property to slice

@armanozak
Copy link
Contributor

I prefer functions, because they are more flexible and reusable. That doesn't mean keys can't be used. I'd rather not have an overloaded function though. Please consider introducing separate functions, if you decide having both.

@Carniatto
Copy link
Contributor Author

I think the function defeats the purpose of making it more readable, but maybe we can consider accepting both?

@splincode
Copy link
Member

splincode commented Aug 11, 2020

@markwhitfeld @Carniatto what do you think about:

import { sliceByState } from '@ngxs/store/utils';

@Selector([ sliceByState({ state: AppState, path: 'users' }) ])
static getUsers....

@joaqcid
Copy link
Contributor

joaqcid commented Aug 11, 2020

i think the one i like the most is this one

export const TodoStateProps = createPropertySelectors<TodoStateModel>(TodoState)

@Selector([TodoStateProps.selectedIds, TodoStateProps.todos)])
static getSelectedTodos....

@Carniatto Carniatto self-assigned this Aug 13, 2020
@irowbin
Copy link

irowbin commented Sep 3, 2020

Similarly I've used dynamic properties to select correct slice of state where decorators are not allowed.

interface AppStateModel{
  state1: number
  state2: string
}

export class AppStateSelectors {

static sliceOf<K extends keyof AppStateModel>(stateKey: K, args?: any) {
    return createSelector([AppState], (state: AppStateModel) => {

      return state[stateKey] as AppStateModel[K] // by doing so, we can have its type when selecting.

    })

}

// to use
stuffsToDo(){
this.store.select(AppStateSelectors .sliceOf('state1')) // returns correct type as  number
this.store.select(AppStateSelectors .sliceOf('state2')) // returns correct type as  string
this.store.select(AppStateSelectors .sliceOf('state100')) // oops, error, key not found

// other stuffs 
}

@rbudnar
Copy link

rbudnar commented Sep 14, 2020

I came up with a similar solution as mentioned in #1653 (comment):

export const createPropsSelector = <TStateModel, K extends keyof TStateModel>(
    stateToken: StateToken<TStateModel>,
    ...keys: K[]
) => {
    const selectors = keys.map(key => createSelector([stateToken], (state: TStateModel) => state[key]));
    return createSelector([...selectors], (...selected) => {
        return keys.reduce((acc, key, i) => {
            acc[key] = selected[i];
            return acc;
        }, {} as Pick<TStateModel, K>);
    });
};

To be used like:

// state token for app state:
export const APP_SETTINGS_STATE_TOKEN = new StateToken<AppSettings>("appsettings");

//
this.store.select(createPropsSelector(APP_SETTINGS_STATE_TOKEN, "prop1", "prop2", "prop3"))

It's a bit more generic and requires a state token but you do get strong typing and intellisense down stream.

@Carniatto
Copy link
Contributor Author

Carniatto commented Sep 29, 2020

Here is a initial proposal for this feature:

import { createSelector, StateToken } from '@ngxs/store';
import { StateClass } from '@ngxs/store/internals';

type SelectorOnly<TModel> = StateToken<TModel> | ((...arg: any) => TModel);

type Selector<TModel> = StateClass<any> | SelectorOnly<TModel>;

export type PropertySelectors<TModel> = {
  [P in keyof TModel]: (model: TModel) => TModel[P];
};

export function createPropertySelectors<TModel>(
  state: Selector<TModel>,
): PropertySelectors<TModel> {
  const cache: Partial<PropertySelectors<TModel>> = {};
  return new Proxy(
    {},
    {
      get(target: any, prop: string) {
        const selector = cache[prop] || createSelector([state], (s: TModel) => s?.[prop]);
        cache[prop] = selector;
        return selector;
      },
    },
  );
}

interface SelectorMap {
  [key: string]: SelectorOnly<any>;
}

type MappedSelector<T extends SelectorMap> = (...args: any[]) => MappedResult<T>;

type MappedResult<TSelectorMap> = {
  [P in keyof TSelectorMap]: TSelectorMap[P] extends SelectorOnly<infer R> ? R : never;
};

export function createMappedSelector<T extends SelectorMap>(selectorMap: T): MappedSelector<T> {
  const selectors = Object.values(selectorMap);
  return createSelector(selectors, (...args) => {
    return Object.keys(selectorMap).reduce((obj, key, index) => {
      (obj as any)[key] = args[index];
      return obj;
    }, {} as MappedResult<T>);
  }) as MappedSelector<T>;
}

export function createPickSelector<TModel, Key extends keyof TModel>(
  state: Selector<TModel>,
  keys: Key[]
) {
  const selectors = keys.map((key) => createSelector([state], (s: TModel) => s[key]));
  return createSelector([...selectors], (...selected: any) => {
    return keys.reduce((acc, key, index) => {
      acc[key] = selected[index];
      return acc;
    }, {} as Pick<TModel, Key>);
  });
}

And here some examples of usage:

/* These are some example usages */

import { createPropertySelectors, createMappedSelector } from "./ngxs-next";

interface AppStateModel { 
  firstName: string, 
  lastName: string 
}

interface AuthStateModel {
  logged: boolean,
  grants: {
    read: boolean,
    write: boolean,
    delete: boolean 
  } 
}


// app.state.selectors.ts
export class AppStateSelectors {
  /* Case1: Creating base props selectors from a state */ 
  static props = createPropertySelectors<AppStateModel>(AppState);
    
  @Selector([AppStateSelectors.props.firstName, AppStateSelectors.props.lastName])
  welcomeMsg(fistName: string, lastName: string) {
    return `Welccome ${firstName} ${lastName};
  }
}

// auth.state.selectors.ts
export class AuthStateSelectors {
  static props = createPropertySelectors<AuthStateModel>(AuthState);
}
   
/* Case2: Creating a mapped selector from selectors */ 
   
const authUser = createMappedSelector({
  firstName: AppStateSelectors.props.firstName,
  logged: AuthStateSelectors.props.logged
})
   
/* Case3: Creating a selector by pick properties */
      
const readWriteGrants = createPickSelector(AuthStateSelectors.props.grants, ['read', 'write'])
   
const adminGrants = createPickSelector(AuthStateSelectors.props.grants, ['read', 'write', 'delete'])

I've also created this Gist here where is possible to copy ngxs-next.ts and use it in your own project.

Please try it out. We are looking for feedback to refine this before integrating out of the box

@markwhitfeld
Copy link
Member

@Carniatto Could you improve your createPickSelector in your gist and the comment above with the function from @rbudnar #1653 (comment)
Just change the name to createPickSelector.
PS. Do you think the second argument of this function should rather be an array as opposed to a spread array? I think it leaves room for the function signature to grow if it needs to one day.

@rbudnar
Copy link

rbudnar commented Oct 1, 2020

@markwhitfeld That seems like a reasonable update and would reduce chances of breaking changes in the future if the signature does need to change 👍

@Carniatto
Copy link
Contributor Author

@markwhitfeld @rbudnar I've updated the Gist and the comment to match your suggestions.

@armanozak
Copy link
Contributor

armanozak commented Oct 9, 2020

Hi @Carniatto,

How do we select baz when the state model is as follows?

interface SomeStateModel {
    foo: {
        bar: {
            baz: string;
        };
    };
}

Please take a look at the updated playground. I believe you will find it valuable.

@richarddavenport
Copy link

@armanozak @Carniatto I left the same feedback/question in the slack channel. I looked at possibly using something like lodash's get method to possibly selected deeply nested properties. Not sure how to implement it though.

@richarddavenport
Copy link

richarddavenport commented Oct 21, 2020

@armanozak I think I figured it out.
https://stackblitz.com/edit/ngxs-state-slice-af6qas?file=src%2Fapp%2Fapp.state.ts

The idea is to just keep creating selectors from the property selectors initially made. It's working really well for me so far.

  // This is from the final proposal
  static props = createPropertySelectors<Model>(AppState);

  // Keep creating selectors
  static fooProps = createPropertySelectors(AppState.props.foo);
  static barProps = createPropertySelectors(AppState.fooProps.bar);

@armanozak
Copy link
Contributor

Hi @richarddavenport,

This does not look practical. It is a lot of work for not so much of a gain. Besides, you will end up creating selectors that you will never use anywhere (other than within the same state class).

@Carniatto
Copy link
Contributor Author

Carniatto commented Oct 21, 2020

Hi @richarddavenport,

This does not look practical. It is a lot of work for not so much of a gain. Besides, you will end up creating selectors that you will never use anywhere (other than within the same state class).

@armanozak

I believe creating selectors that are only used in the state class is not a bad practice. Matter of fact I'd encourage using this to promote smaller and easier to test selectors.

About the necessity to deeply create slices, I believe that's not the purpose of these helpers and it can be achieved using createSelector instead.

@richarddavenport
Copy link

richarddavenport commented Oct 21, 2020

@armanozak It's funny to me, but creating selectors that you will never use anywhere (other than within the same state class). actually sounds like the practical approach.

I understand the desire to reduce the number of selectors and to only have as many as you may need, and by using this approach I went from 14 selectors (all of them used) to 4 selectors.

before

  public static getSlice(property: keyof RaterQuoteFormStateModel) {
    return createSelector([RATER_QUOTE_FORM_STATE_TOKEN], (state: RaterQuoteFormStateModel) => state[property]);
  }

  @Selector([RaterQuoteFormState.getSlice('form')])
  private static _getYear(form: RaterQuoteFormModel) {
    return form.year;
  }

  @Selector([RaterQuoteFormState.getSlice('form')])
  private static _getDealerCode(form: RaterQuoteFormModel) {
    return form.dealerCode;
  }

  @Selector([RaterQuoteFormState._getDealerCode])
  public static getDealerCode(dealerCode: string) {
    return dealerCode;
  }

  @Selector([RaterQuoteFormState.getSlice('errors')])
  public static getQuoteResponseErrors(errors: RaterQuoteFormErrors) {
    return errors.quoteResponse;
  }

  @Selector([RaterQuoteFormState._getYear])
  public static getYear(year: string) {
    return year;
  }

  @Selector([RaterQuoteFormState.getSlice('form')])
  private static _getMake(form: RaterQuoteFormModel) {
    return form.make;
  }

  @Selector([RaterQuoteFormState._getMake])
  public static getMake(make: string) {
    return make;
  }

  @Selector([RaterQuoteFormState.getSlice('form')])
  private static _getVin(form: RaterQuoteFormModel) {
    return form.vin;
  }

  @Selector([RaterQuoteFormState._getVin])
  public static getVin(vin: string) {
    return vin;
  }

  @Selector([RaterQuoteFormState.getSlice('form')])
  public static getMileage(form: RaterQuoteFormModel) {
    return form.mileage;
  }

  @Selector([RaterQuoteFormState.getSlice('form')])
  private static _getModel(form: RaterQuoteFormModel) {
    return form.model;
  }

  @Selector([RaterQuoteFormState._getModel])
  public static getModel(model: string) {
    return model;
  }

  @Selector([
    RaterQuoteFormState.getVin,
    RaterQuoteFormState.getYear,
    RaterQuoteFormState.getMake,
    RaterQuoteFormState.getModel,
  ])
  public static getVehicleFormInfo(vin: string, year: string, make: string, model: string) {
    return { vin, year, make, model };
  }

after

  public static props = createPropertySelectors(RATER_QUOTE_FORM_STATE_TOKEN);
  public static formProps = createPropertySelectors(RaterQuoteFormState.props.form) as PropertySelectors<RaterQuoteFormModel>;
  public static errorProps = createPropertySelectors(RaterQuoteFormState.props.errors) as PropertySelectors<RaterQuoteFormErrors>;

  @Selector([
    RaterQuoteFormState.formProps.vin,
    RaterQuoteFormState.formProps.year,
    RaterQuoteFormState.formProps.make,
    RaterQuoteFormState.formProps.model,
  ])
  public static getVehicleFormInfo(vin: string, year: string, make: string, model: string) {
    return { vin, year, make, model };
  }

So, while it may seem at first like a lot of work for not so much of a gain, that is not the case for me and actually the opposite (less work, less code, more gain). Maybe for you though... but I'd encourage you to try it.

As a side note, the reason I have private selectors is that it is my attempt at creating an optimized selector:

  // this is not optimized enough for me... I don't want this selector to evaluate anytime the form changes
  @Selector([RaterQuoteFormState.getSlice('form')])
  private static _getMake(form: RaterQuoteFormModel) {
    return form.make;
  }

  // this will only evaluate if make has changed, not the entire form.
  @Selector([RaterQuoteFormState._getMake])
  public static getMake(make: string) {
    return make;
  }

So far I have noticed that the potentially new property selectors are optimized, so that is another reason to continue to try this new approach.

@armanozak
Copy link
Contributor

Hi @richarddavenport,
This does not look practical. It is a lot of work for not so much of a gain. Besides, you will end up creating selectors that you will never use anywhere (other than within the same state class).

@armanozak

I believe creating selectors that are only used in the state class is not a bad practice. Matter of fact I'd encourage using this to promote smaller and easier to test selectors.

About the necessity to deeply create slices, I believe that's not the purpose of these helpers and it can be achieved using createSelector instead.

Well, everything “can be achieved using createSelector”. That is not the point, isn’t it?

Never mind. You have asked about my opinion and here it is: Selecting only the first level of properties is definitely not worth discussing. So, please proceed as you prefer.

@markwhitfeld
Copy link
Member

markwhitfeld commented Oct 21, 2020

I'll just add my 2c here 😉
I think that the first step is to reduce the first level selectors that I have seen proliferating many apps. It is not meant to solve all cases, but rather to provide a convenient helper to reduce the number of selectors that look exactly the same other than a slight change in the name of the property being looked up.
All of these build upon createSelector so it is true that you can build anything with that 👍.
Our apps have vastly different needs and here we are trying to address the most common basic need to slice up the state into smaller selectors that through memoization will give a great tool for optimising your composite selectors.
Also, as a side note, that the createPropertySelectors creates a proxy that will create selectors as you access them, so technically it doesn't create any selectors that you don't use.

I think that the topic of deep selectors is a separate one to what is being proposed here.
That is not to say that it is not a valid topic to discuss 😄! Although we are encouraged to keep our states as flat as possible, it is not always practical or feasible. Selecting deep properties can be another challenge in itself. There are some ideas that I am playing with that will allow for the dynamic creation of deep selectors from any part of an object graph, but I think that would definitely be a labs project.

I hope that this makes sense and thank you so much for all the feedback and ideas!!!
This can only improve with more feedback

@markwhitfeld
Copy link
Member

markwhitfeld commented Oct 21, 2020

@richarddavenport Just a further optimisation you could make to your code with the other proposed selectors:
Before:

  public static props = createPropertySelectors(RATER_QUOTE_FORM_STATE_TOKEN);
  public static formProps = createPropertySelectors(RaterQuoteFormState.props.form) as PropertySelectors<RaterQuoteFormModel>;
  public static errorProps = createPropertySelectors(RaterQuoteFormState.props.errors) as PropertySelectors<RaterQuoteFormErrors>;

  @Selector([
    RaterQuoteFormState.formProps.vin,
    RaterQuoteFormState.formProps.year,
    RaterQuoteFormState.formProps.make,
    RaterQuoteFormState.formProps.model,
  ])
  public static getVehicleFormInfo(vin: string, year: string, make: string, model: string) {
    return { vin, year, make, model };
  }

After (using createMappedSelector):

  public static props = createPropertySelectors(RATER_QUOTE_FORM_STATE_TOKEN);
  public static formProps = createPropertySelectors(RaterQuoteFormState.props.form) as PropertySelectors<RaterQuoteFormModel>;
  public static errorProps = createPropertySelectors(RaterQuoteFormState.props.errors) as PropertySelectors<RaterQuoteFormErrors>;

  public static getVehicleFormInfo = createMappedSelector({
    vin: RaterQuoteFormState.formProps.vin,
    year: RaterQuoteFormState.formProps.year,
    make: RaterQuoteFormState.formProps.make,
    model: RaterQuoteFormState.formProps.model
  });

After (using createPickSelector):

  public static props = createPropertySelectors(RATER_QUOTE_FORM_STATE_TOKEN);
  public static formProps = createPropertySelectors(RaterQuoteFormState.props.form) as PropertySelectors<RaterQuoteFormModel>;
  public static errorProps = createPropertySelectors(RaterQuoteFormState.props.errors) as PropertySelectors<RaterQuoteFormErrors>;

  // note that this doesn't even make use of your formProps selector, and achieves the same optimised selector!
  public static getVehicleFormInfo = createPickSelector(RaterQuoteFormState.props.form, ['vin', 'year', 'make', 'model']);

Thanks for the example to try this out on!

@markwhitfeld
Copy link
Member

These are finally landing in the next release!
See PR: #1824

@markwhitfeld
Copy link
Member

Great news! v3.8.0 has been released and it includes a fix for this issue.
We are closing this issue, but please re-open it if the issue is not resolved.
Please leave a comment in the v3.8.0 release discussion if you come across any regressions with respect to your issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants