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: extract createSelector function for public api #484

Merged
merged 1 commit into from
Jul 10, 2018

Conversation

markwhitfeld
Copy link
Member

@markwhitfeld markwhitfeld commented Jul 6, 2018

Implements solution proposed in #426 to expose the createSelector function.
Provides a solution for #413.

It allows for this sort of syntax, enabling a static form of parameterized selectors:

@State<ConfigStateModel>({
  name: 'config',
  defaults: { clients: { schema: {} }, incomes: { schema: {} } }
})
export class ConfigState {
  static getSchema(type: keyof ConfigStateModel) {
    return createSelector([ConfigState], (state: ConfigStateModel) => state[type].schema);
  }
}

@Component(...)
export class ClientsListComponent {

  @Select(ConfigState.getSchema('clients'))
  public clientSchema$: Observable<any>;

}

@markwhitfeld markwhitfeld force-pushed the extract-createSelector branch from 8cc7ecc to a0b0b72 Compare July 6, 2018 21:05
@deebloo
Copy link
Member

deebloo commented Jul 6, 2018

I think this is a good idea

@markwhitfeld
Copy link
Member Author

The integration tests failed in a strange way (looks like a server error):
https://circleci.com/gh/ngxs/store/3541

Rewrote commit and pushed again to get a rebuild going and got the same error:
https://circleci.com/gh/ngxs/store/3544

It works locally so I'm not sure what the issue is here. Looks like missing pollyfills or something

@markwhitfeld
Copy link
Member Author

Added more tests as part of this and now we have broken the 100 tests mark!

@deebloo
Copy link
Member

deebloo commented Jul 10, 2018

@markwhitfeld this is actually a bit different then I was thinking. still poking at it

@markwhitfeld
Copy link
Member Author

@deebloo what were you thinking? I'm interested in what other use cases for a createSelector that there may be

@amcdnl
Copy link
Member

amcdnl commented Jul 10, 2018

LGTM - what were u thinking Danny?

Make sure to update the docs.

@deebloo
Copy link
Member

deebloo commented Jul 10, 2018

I still don't like pulling one state class into another. but I also don't see a way around it if we want to do something like this

@markwhitfeld markwhitfeld merged commit b50211a into ngxs:master Jul 10, 2018
@amcdnl
Copy link
Member

amcdnl commented Jul 15, 2018

@markwhitfeld - Can u PR docs for this?

@juliusstoerrle
Copy link
Contributor

Hi! When would I choose this over the earlier introduced dynmic selector pattern (@Selector() static foo(state) { return (arg) => {}; })?

Main benefit I see is the use in @Select() and the reduced boilerplate which was introduced by the .pipe(map(...))? Art there any downsides to using this new method?

@markwhitfeld
Copy link
Member Author

@juliusstoerrle See the docs added in PR #511
It clarifies the difference between lazy and dynamic selectors (the existing type being called 'lazy' and this new type 'dynamic').

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

Successfully merging this pull request may close these issues.

4 participants