ngxs / store

🚀 NGXS - State Management for Angular
http://ngxs.io
MIT License
3.52k stars 400 forks source link

feat(signals): provide `@ngxs/signals` package with signals utilities #2141

Closed arturovt closed 3 months ago

nx-cloud[bot] commented 4 months ago

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 0c0c064c02ced54a94c96eb5fe5166b60d3cfdf0. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 5 targets - [`nx lint-types signals`](https://cloud.nx.app/runs/6Qf4eerLn5?utm_source=pull-request&utm_medium=comment) - [`nx lint-types store`](https://cloud.nx.app/runs/LSrAMV3NOa?utm_source=pull-request&utm_medium=comment) - [`nx run-many --target=test --all --configuration=ci --maxWorkers=4`](https://cloud.nx.app/runs/dmpPWUp9sk?utm_source=pull-request&utm_medium=comment) - [`nx run-many --target=lint --all`](https://cloud.nx.app/runs/Lu1FmQL9UB?utm_source=pull-request&utm_medium=comment) - [`nx run-many --target=build --all`](https://cloud.nx.app/runs/KuRARKZ06y?utm_source=pull-request&utm_medium=comment)

Sent with 💌 from NxCloud.

bundlemon[bot] commented 4 months ago

BundleMon (Integration Projects)

Unchanged files (2) Status | Path | Size | Limits :------------: | ------------ | :------------: | :------------: :white_check_mark: | Main bundles(Gzip)
hello-world-ng17/dist-integration/main.(hash)
.js
| 68.49KB | +1% :white_check_mark: | Main bundles(Gzip)
hello-world-ng16/dist-integration/main.(hash)
.js
| 67.03KB | +1%

No change in files bundle size

Final result: :white_check_mark:

View report in BundleMon website ➡️


Current branch size history | Target branch size history

rhutchison commented 4 months ago

There are 3 utilities: select, produceActions, and produceSelectors

select:

@Component({...})
export class SomeComponent {
  user = select(AuthQueries.getUser);
}

produceActions/produceSelectors:

Allow users to quickly adapt ngxs store to @ngrx/signals (signalStore). We can provide the following recipes in the documentation:

export function withSelectors<T extends SelectorMap>(selectors: T) {
  return signalStoreFeature(withComputed(() => produceSelectors(selectors)));
}

export function withActions<T extends ActionMap>(actions: T) {
  return signalStoreFeature(withMethods(() => produceActions(actions)));
}

Usage:

export const UserState = signalStore(
  withSelectors({
    user: AuthQueries.getUser
  }),
  withActions({
    login: AuthActions.login,
    logout: AuthActions.logout
  })
)
arturovt commented 4 months ago

questions:

markwhitfeld commented 4 months ago

questions:

  • do we keep select name for the utility function?
  • I added docs and placed it under docs/concepts/select/signals.md, I ain't sure if it should be under concepts/select. Any ideas?

I think we can leave this utility function out for the first release of this. WDYT? There needs to be a final discussion on naming. I think that would be the correct place in the docs.

rhutchison commented 4 months ago

questions:

  • do we keep select name for the utility function?
  • I added docs and placed it under docs/concepts/select/signals.md, I ain't sure if it should be under concepts/select. Any ideas?

I think we can leave this utility function out for the first release of this. WDYT? There needs to be a final discussion on naming. I think that would be the correct place in the docs.

I think the utility function is super helpful, is inline with direction of replacing decorators with functions, and times nicely with the latest signal queries:

@ContentChild => contentChild and @ContentChildren => contentChildren @ViewChild => viewChild and @ViewChildren => viewChildren

If we're looking to deprecate @Select it makes perfect sense to introduce select.

markwhitfeld commented 4 months ago

questions:

  • do we keep select name for the utility function?
  • I added docs and placed it under docs/concepts/select/signals.md, I ain't sure if it should be under concepts/select. Any ideas?

I think we can leave this utility function out for the first release of this. WDYT? There needs to be a final discussion on naming. I think that would be the correct place in the docs.

I think the utility function is super helpful, is inline with direction of replacing decorators with functions, and times nicely with the latest signal queries:

@ContentChild => contentChild and @ContentChildren => contentChildren @ViewChild => viewChild and @ViewChildren => viewChildren

If we're looking to deprecate @Select it makes perfect sense to introduce select.

This makes me so happy!!! There has been a lot of discussions about the naming of this utility, and I have always wanted to have it as select, advocating a signals first approach. The hesitation has been that it doesn't match the method names of the Store methods. Taking Angular's lead on this is a great motivation for this naming. @arturovt I'm definitely happy for this utility to be called select. @dmitry-stepanenko What do you think? You had raised the concern about alignment in the discussions.

markwhitfeld commented 4 months ago

Just a subtle naming discussion point... Do you think produceSelectors is the ideal name? It produces something from selectors. It produces "select"s. What do you think of the name produceSelects? I'm not pushing for either, just looking for thoughts on it... cc @arturovt @rhutchison @dmitry-stepanenko @Carniatto @joaqcid @profanis

markwhitfeld commented 4 months ago

And, if we have a select utility to produce a single output as well as produceSelectors to produce a mapped output for selectors, do you think we should also have a dispatch utility for producing a single dispatch function from an action? cc @arturovt @rhutchison @dmitry-stepanenko @Carniatto @joaqcid @profanis

markwhitfeld commented 4 months ago

Given my last 2 questions, it leads me to ask: What do you think about produceDispatchers as an alternative to produceActions? cc @arturovt @rhutchison @dmitry-stepanenko @Carniatto @joaqcid @profanis

rhutchison commented 4 months ago

And, if we have a select utility to produce a single output as well as produceSelectors to produce a mapped output for selectors, do you think we should also have a dispatch utility for producing a single dispatch function from an action? cc @arturovt @rhutchison @dmitry-stepanenko @Carniatto @joaqcid @profanis

I did start to discuss with Artur. asDispatch or dispatch?

export function asDispatch<TArgs extends any[]>(
  actionType: ActionDef<TArgs>
) {
  return (...args: TArgs) => inject(Store).dispatch(new actionType(...args));
}
@Component({...})
export class SomeComponent {
  user = select(AuthQueries.getUser);

  login = asDispatch(AuthActions.login);
}
rhutchison commented 4 months ago

Just a subtle naming discussion point... Do you think produceSelectors is the ideal name? It produces something from selectors. It produces "select"s. What do you think of the name produceSelects? I'm not pushing for either, just looking for thoughts on it... cc @arturovt @rhutchison @dmitry-stepanenko @Carniatto @joaqcid @profanis

Given my last 2 questions, it leads me to ask: What do you think about produceDispatchers as an alternative to produceActions? cc @arturovt @rhutchison @dmitry-stepanenko @Carniatto @joaqcid @profanis

Maybe toSelectMap and toDispatchMap. Would like to hear other's feedback before we merge.

arturovt commented 4 months ago

Given my last 2 questions, it leads me to ask: What do you think about produceDispatchers as an alternative to produceActions? cc @arturovt @rhutchison @dmitry-stepanenko @Carniatto @joaqcid @profanis

Since the function is generating dispatch functions rather than actions themselves a more appropriate name could be produceDispatchers or createDispatchers, or smth else.

markwhitfeld commented 4 months ago

I like select and dispatch. Although, including the dispatch here which returns an Observable seems a bit odd for a @ngxs/signals package. Would it rather belong in the core package?

If so, then what would that mean for the produceDispatchers (or whatever we call it). Does the same logic apply? Would that also shift to the main package?

rhutchison commented 4 months ago

I like select and dispatch. Although, including the dispatch here which returns an Observable seems a bit odd for a @ngxs/signals package. Would it rather belong in the core package?

If so, then what would that mean for the produceDispatchers (or whatever we call it). Does the same logic apply? Would that also shift to the main package?

dispatch should go in core, I think produceSomething should stay here,

I like select and dispatch. Although, including the dispatch here which returns an Observable seems a bit odd for a @ngxs/signals package. Would it rather belong in the core package?

If so, then what would that mean for the produceDispatchers (or whatever we call it). Does the same logic apply? Would that also shift to the main package?

dispatch should definitely go in core, not sure about producer, but makes sense to question it

markwhitfeld commented 4 months ago

In terms of naming, we have a few options:

rhutchison commented 4 months ago

In terms of naming, we have a few options:

  • produceSelects
  • makeSelectMap
  • createSelectMap
  • produceSelectMap
  • asSelectMap

I suggested toSelectMap and toDispatchMap, bc of the rxjs-interop naming convention

markwhitfeld commented 4 months ago

In terms of naming, we have a few options:

  • produceSelects
  • makeSelectMap
  • createSelectMap
  • produceSelectMap
  • asSelectMap

I think I prefer createSelectMap because it has similar naming to our other selector utilities (also starting with create...), which potentially makes it more discoverable too.

arturovt commented 4 months ago

i'm fine with it too. so do we go with createSelectMap and createDispatchMap?

Carniatto commented 4 months ago

Sorry I only saw the discussion now. Definitely the content of this PR is awesome, thanks Arthur for all the work.

I was wondering if we should align with the terminology we already have for the selector utilities. See https://www.ngxs.io/advanced/selector-utils

I'm okay with the proposed naming also, I'm just wondering if we want to create more context for similar functionality

dmitry-stepanenko commented 4 months ago

Awesome PR! I think I like the createSelectMap and createDispatchMap naming option.

@dmitry-stepanenko What do you think? You had raised the concern about alignment in the discussions.

@markwhitfeld back in the RFC we discussed that it will be selectSignal. At the same time I'm looking an it now and I think the select name aligns really well with all the new utilities angular has recently presented.

profanis commented 4 months ago

Great work @arturovt! My delayed input is that I like the select utility. As for the dispatchers/actions, I agree with Artur that the function is generating Dispatchers and as such, I am more towards the CreateDispatchers. I am afraid that the name CreateActions or ProduceActions is not that clear in terms of its underlying implementation.

Having said that, I would vote for the naming convention Create*

rhutchison commented 4 months ago

Great! So it looks like we have consensus on select, createSelectMap and createDispatchMap. We are just missing tests for createDispatchMap in this PR. The one other thing to be settled is the inclusion of the dispatch utility, and if both the dispatch utilities should exist in the @ngxs/signals package, or in the @ngxs/store package. Thoughts? @rhutchison @Carniatto @arturovt @joaqcid @dmitry-stepanenko @profanis

dispatch utility in separate PR https://github.com/ngxs/store/pull/2143

I am not sure where createDispatchMap should land, but I'm leaning to say keep where it is bc of the use-case with ngrx/signals (signalStore)

arturovt commented 4 months ago

We can continue the discussion in a separate pull request related to the dispatch functionality. However, considering the dispatch map, it might seem odd to have different imports since these functions aim to provide the same utility value.

rhutchison commented 3 months ago

I believe it's ready to merge

arturovt commented 3 months ago

Merged. Can address any issues in subsequent PRs.

markwhitfeld commented 3 months ago

I think we should include the dispatch utility in the @ngxs/signals package so that it is grouped together with the other utilities that follow the "signals style". It is also easier to move something into the main package at a later stage, than it out of it.