Discussion: Selector Composition and Query Classes

[markwhitfeld]

Discussion

The purpose of this issue is to facilitate a discussion around the possibilities for selectors and querying the application state. Many ideas on this topic will be listed here.

Versions

* ngxs: 3.0.1
* @angular/core: 3.0.0

Items to agree on:

• Query Class concept
• State Class @Selector(...) method signatures
• Query Class @Selector(...) method signatures
• Allowed selector composition parameters
• Support for dynamic @selector arguments
• @selector options

I would like to try some rules to help facilitate voting on ideas ( because... why not! ):

• For a new idea, add a comment and give it a Title in bold at the top of the comment which people will use to reference to it. One idea per comment. If the idea has multiple options please split into multiple comments titled appropriately.
• Vote on ideas using the Reactions to the comment. ie. 👍 for agreement, 👎 for disagreement.
• If you disagree with an idea and/or would like to add to it then add a comment referencing the comment with details as to why you disagree (unless someone has already stated a similar disagreement which you can agree to).

[markwhitfeld]

Query Class concept
Regarding the case for the ability to have @Selector(...) on a non state class, here are some thoughts as a motivation in this regard...
This type of class could be called a Query Class. Your code would look something like this:

export class UserTodosQuery {
  @Selector([UserState.getCurrentUserId, TodoState])  
  getCurrentUserTodos(userId: string, todoState: TodoStateModel) { ... }

  @Selector([UserState.getCurrentUserId, TodoState.getInCompleteTodos])  
  getCurrentUserIncompleteTodos(userId: string, todos: Todo[]) { ... }
}

You would do this where you want to combine two selectors across two states that don't have a common route. I've also seen people mention that they want this sort of thing for feature modules so that they can combine states between 2 distinct feature modules. You wouldn't want to force a dependency inside the feature module to another for the sake of a selector that you only need when you are using both feature modules. This type of class promotes decoupling from the State.

The simplicity of the selectors and selector composition is very valuable when combining state across different parts of the app. Allowing for composed Selectors in non-state classes allows for a clean separation of selectors if someone prefers to keep them separate from the state as a part of their application style (command and query code separation).

An argument against this would be that you could just combine the selectors by using store.select(...) for each selector and doing a combineLatest to bring the data together. In my experience this is quite difficult to get your head around because you need to switch from thinking about pure functions to streams (and combining observables). In addition, a selector combined in this way is also quite challenging to test and would be very awkward to reuse elsewhere.

A comment from @leon in our team discussion:

I agree with allowing to have selectors separated. It would allow grouping selectors into categories, and also creating cross cutting selector classes that are very specific. these would act as the joins between states.
It would also mean the state classes become more single responsibility.
Reading data (querying) is always very opinionated, since it’s almost always connected to some kind of UI, and depending on how that UI looks you need to convert your state into a model that the ui can accept.

Please vote for Query Class concept or against: 👍 or 👎

[markwhitfeld]

State Class parameterless Selector
(This is the existing usage, listed here as the most basic case)
A @Selector decorator with zero parameters is only valid in a state class and provides the container class' state model as the only parameter :

@State<UserStateModel>( ... )
export class UserState {
  @Selector()  
  getUsers(userStateModel: UserStateModel) { ... }
}

Please vote 👍 or 👎

[markwhitfeld]

Query Class parameterless Selector should be invalid
A @Selector decorator with zero parameters should not be valid in a Query (non-state) class.
The reason that this should be invalid is because the only thing that you could pass to the method would be the entire application state and this would make for a fragile selector that would have too much knowledge about the application state structure, violating the separation of concerns that the Query class is trying to promote. To be clear, this would be invalid:

export class AppQuery {
  @Selector()  // Invalid! This should throw an error.
  getSomeInfo(appStateModel: any) { ... }
}

Please vote to agree or disagree that this should be seen as invalid 👍 or 👎

[markwhitfeld]

State Class Selector with parameters (Option 1)
A state class @Selector decorator with other selectors provided as parameters would provide the output of only the provided selectors as the parameters of the method. If you want the state model of the containing class then you need to add it as a parameter. See below:

@State<UserStateModel>( ... )
export class UserState {
  @Selector([UserState, Companies.getCompanies])  
  getUsers(userStateModel: UserStateModel, companies: Company[]) { ... }
}

The motivation for dropping the first function parameter containing the state is so that the method signature for composite Selectors is consistent if they are on a State or a Query class. It would allow this selector to move to the Query class from the State class or visa versa without any modification.
I think that I prefer this because there is no magic first parameter that is implied from the container context which would also make the typescript typings a lot more straightforward.
Please vote 👍 or 👎

[markwhitfeld]

State Class Selector with parameters (Option 2)
A state class @Selector decorator with other selectors provided as parameters would add the output of those selectors as additional parameters of the method after the inferred container state's model:

@State<UserStateModel>( ... )
export class UserState {
  @Selector([Companies.getCompanies])  
  getUsers(userStateModel: UserStateModel, companies: Company[]) { ... }
}

Please vote 👍 or 👎

[markwhitfeld]

Query Class Selector with parameters (Option 1)
A query class @Selector decorator with selectors provided as parameters would provide the output of only the provided selectors as the parameters of the method. There would be no inferred state model from the context of the @Selector . See below:

export class UserTodosQuery {
  @Selector([UserState.getCurrentUserId, TodoState])  
  getCurrentUserTodos(userId: string, todoState: TodoStateModel) { ... }
}

Please vote 👍 or 👎

[markwhitfeld]

Query Class Selector with parameters (Option 2)
A query class @Selector decorator with selectors provided as parameters would add the output of those selectors as additional parameters of the method after the application state:

export class UserTodosQuery {
  @Selector([UserState.getCurrentUserId, TodoState])  
  getCurrentUserTodos(appState: any, userId: string, todoState: TodoStateModel) { ... }
}

I disagree with this because it promotes a special knowledge of the application state structure (for which there would have typically been no interface defined). This breaks encapsulation and would make for fragile code.
Please vote 👍 or 👎 to indicate if you agree with this structure or not

[markwhitfeld]

Allowed Selector composition parameters (State Class)
A State Class can be provided to the selector in combination with zero or many other valid selector parameters to request that the value represented by the model of the state class be passed in to the selector function. For example:

export class UserTodosQuery {
  @Selector([UserState, TodoState])  
  getCurrentUserTodos(userState: UserStateModel, todoState: TodoStateModel) { ... }
}

Please vote 👍 or 👎

[markwhitfeld]

Allowed Selector composition parameters (Selector)
Another Selector method can be provided to the selector in combination with zero or many other valid selector parameters to request that the value returned by the provided Selector method be passed in to the selector function. For example:

export class UserTodosQuery {
  @Selector([UserState.getCurrentUserId, TodoState])  
  getCurrentUserTodos(userId: string, todoState: TodoStateModel) { ... }
}

Please vote 👍 or 👎

[markwhitfeld]

Allowed Selector composition parameters (lambda)
A Lambda function can be provided to the selector in combination with zero or many other valid selector parameters to request that the value returned by the lambda function be passed in to the selector function. The lambda function would only be valid in the context of a containing state class and its argument would be of the same type as the containing state class. For example:

@State<UserStateModel>( ... )
export class UserState {
  @Selector([(userState) => userState.currentUserId])  
  getUsers(userStateModel: UserStateModel, currentUserId: string) { ... }
}

This parameter type does not really make sense to me but has been included for the sake of completeness to reflect the ways in which the store.select(...) method can be used. You wouldn't want to break encapsulation here by allowing access to the application state object, but it also seems very random to provide a lambda for the container class if you have access to that class directly anyway.
Please vote 👍 or 👎

[markwhitfeld]

Approach for dynamic @selector arguments
A number of users have asked for the ability to pass a parameter into a selector. They were mentioning additions to the @Selector in order to do this. This would lead to a few complications, like the fact that a decorator does not have access directly to the containing class' instance (this). As a result this would become a static parameterized selector which is much less useful than the original requests intended.

That being said, we can achieve this at the moment using a pattern often used for redux selectors...
The @Selector decorator can be written so that it returns a function with the desired parameter.
This enables the desired dynamic selector arguments as well as late resolution of the selected state. For Example:

@State<UserStateModel>( ... )
export class UserState {
  @Selector()  
  getFilteredUsersFn(userStateModel: UserStateModel) {
    return (filter: string) => userStateModel.users.filter((user) => user.indexOf(filter) >= 0)
  }
}

And then the component would contain:

@Component({...})
export class AppComponent {
  @Select(UserState.getFilteredUsersFn) filteredUsersFn$: Observable<(filter: string) => User[]>;

  get currentFilteredUsers$() {
    return this.filteredUsersFn$.pipe(map(filterFn => filterFn('myFilter')));
  }
}

This approach can be taken with the current version if NGXS.
This item is being used to represent the approach and would be added to the docs if deemed to be a good approach.
Please vote if you think that this is a good approach or not: 👍 or 👎

[markwhitfeld]

Enhanced support for dynamic @selector arguments (inner function memoization)
Following on from the dynamic @Selector idea above, the inner function returned by the @Selector could be memoized automatically. This would mean that in the following example if the function returned by the selector was called multiple times with the same filter parameter it would return the same result without reprocessing until the outer function received different input:

@State<UserStateModel>( ... )
export class UserState {
  @Selector()  
  getFilteredUsersFn(userStateModel: UserStateModel) {
    return (filter: string) => userStateModel.users.filter((user) => user.indexOf(filter) >= 0)
  }
}

Please vote 👍 or 👎

[markwhitfeld]

Selector options
Regarding the possibility later of adding options; I think that the options could be expressed as an object and because the selectors are expressed as an array the decorator could detect if an options object or an array was passed to it or both. As an example the only thing I could think of now that may be an option we could add in future is a different memoizer, so this is what this could look like:

export class SomeStateClassOrRegularService {
  @Selector([UserState.getName, TodoState], { memoizer: customMemoizer(3) })  
  getUserTodos(userName: string, todoState: TodoStateModel) { ... }
}

PS. selector option given as an example but I don't think we need to implement the options part until there is an actual need for it.
Please vote 👍 or 👎 in support or disagreement with this approach

[markwhitfeld]

Hi @leon @Dyljyn @paulstelzer
You expressed that you disagree with the Approach for dynamic @Selector arguments above.
Could you add your comments as to what your concerns were about the approach and if you have an alternative idea for an approach please add it to the mix here.

(This 'Discussion' issue is a 'Monologue' until somebody else posts in it. Please help me change that 😉 )

[BeaveArony]

Hi @markwhitfeld
Great discussion! About the dynamic selectors: I think this is difficult to tackle no matter if you use redux, ngrx or ngxs. In my experience you often need this, when you the need to get a list of parametrized selectors, but then you loose the memoize effect, since the outer selector changes with every request.
I see this when I'm having a page of related blocks/cards that have other lists of items and each of those blocks has an id I want to select. The outer function would alway get a different id in this case.
What I tend to do is to either return new Map of all those objects I want to the component or to create a stateful selector that stores all the parametrized inner selectors in a Map by id.
I usually go with the first approach and just built Maps by id to pass to further selectors down the chain. Unfortunately they all need to recompute each time any of those states change. For performance reasons I should optimise by memoizing the memoized selectors.

I hope this makes some sense.

I like the approach of having Query Classes separate from the state classes. What I often do is something like Todd Moto presented at ng-conf 2018. Start with the router selector and build on top of the url parameters with many further selectors in chains. This keeps my container component very clean and the selector files get pretty big. The good thing is they are easily testable.

[amcdnl]

Feedback:

Query Class concept

I like the idea, I don't feel like it should be a decorator on a misc class though. What if we made something like redux does where its just a function and then you can make it into whatever you want?

State Class parameterless Selector

I'm opposed to make a breaking change.

Query Class parameterless Selector should be invalid

See Query Class concept notes.

State Class Selector with parameters (Option 1)

I don't like this one because its just more code. Plus it deviates from the way it works now.

State Class Selector with parameters (Option 2)

Personally i like this one. It combines the concept we have now with the ability to add N* selectors to it. The code looks quite nice in real world too... Here is a real world case I'm using it.

  @Selector([PreferencesState.grid, PreferencesState.user])
  static items(state: ItemsStateModel, { sort }, { nameSortOrder, sizeSortOrder, hideAnnoyingFiles }) {
    const items = hideAnnoyingFiles ?
      state.items.filter(i => i.name[0] !== '.') :
      [...state.items];

    return items.sort((a: any, b: any) =>
      sort.direction === 'asc' ?
        sortItems(a, b, sort, nameSortOrder, sizeSortOrder) :
        sortItems(b, a, sort, nameSortOrder, sizeSortOrder)
    );
  }

Query Class Selector with parameters (Option 1)

See thoughts on Query Class concept.

Query Class Selector with parameters (Option 2)

Not a fan.

Allowed Selector composition parameters (State Class)

Ya I'd expect this.

Allowed Selector composition parameters (lambda)

I don't mind this. Can't think of a good idea to use it but hey.

Approach for dynamic @selector arguments

I was actually thinking the same thing for this.

Enhanced support for dynamic @selector arguments (inner function memoization)

Ya i'd expect this.

Selector options

I had this idea when i made this feature but never had a great use case for it so i didn't see the point.

[amcdnl]

Thinking about this some more, here is a use case I can think of:

Lets say I have 2 states; a Zoo and a Theme Park. I want to have a page that would show both zoos and theme parks for a given location. In that scenario, these 2 are disconnected states where I want to pass an argument(zip code) to a selector and join the two together.

After thinking about that, I have these questions/thoughts:

1. Joining these in a 'parent' state doesn't really make sense since they are so highly disconnected entities.
2. Making a combineLatest is not really reusable or memoized and you need to know RX well. But do i really want to reuse that selector since its so specific? If its used a lot shouldn't these states be merged?
3. What if they are lazy loaded? That would be odd.

[amcdnl]

I can say, I'm on board with the following:

1. Ability to return a function to accept parameters
2. Passing other selectors into a selector to join them

The query class idea just feels like a bad idea to me though.

[deebloo]

I think that now that selector functions work as expected, calling them that parent classes is enough. You can define selectors in child states and then call those from the parent. then combining selectors is just function composition. If we add a special synax for composing I am afraid we are adding unneeded complexity. But I certainly seem to be in the minority in this regard.

My personal feeling is that as long as we provide the ABILITY to combine selectors function (which previously didn't work) that should be where the library's responsibility ends

[rhutchison]

I like the query class concept, as I'm all for refactoring the complexity away from the components, but without handing all of the responsibility to ngxs. The one idea that stands out is Enhanced support for dynamic @selector arguments (inner function memoization), which seems to have less 👍 than the others.

[amcdnl]

This is almost 100%, still need ability to do pass arguments. @markwhitfeld - can u look at that?

[markwhitfeld]

@amcdnl I'll do that one

[maxencefrenette]

Hi, there is a problem with the current design of the library that should be addressed in future releases. Let's say there is a state that consist of 3 properties.

@State({
    name: 'foo',
    defaults: {
        a: 0,
        b: 0,
        c: 0,
}
})
export class FooState {

    // actions

    @Selector()
    static selector(state) {
        return { a: state.a, b: state.b };
    }
}

The problem with this code is that whenever c changes, the selector will be reevaluated and this will trigger a rerender. The cleanest solution to individually select a and b and then put them together in this selector. However, this is not possible at the moment because the whole state slice is forcefully passed to all selectors created inside a state class. This is why it is very important to adopt "State Class Selector with parameters (Option 1)".

Since it would a breaking change that will have to wait for NGXS 4, we could potentially make another decorator such as @ExplicitSelector() that has the functionnality described above. Another option to solve this problem (albeit in a hackish manner) would be to give access to the memoization via the second parameter of the @Selector() decorator.

Finally, as a bonus, something along the lines of reselect's createStructuredSelector would be amazing in the future.

[amcdnl]

@maxencefrenette - can you open a new issue to discuss this?