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(signals): provide @ngxs/signals package with signals utilities #2141

Merged
merged 8 commits into from
Mar 27, 2024

Conversation

arturovt
Copy link
Member

No description provided.

Copy link

nx-cloud bot commented Mar 23, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 0c0c064. 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

Sent with 💌 from NxCloud.

Copy link

bundlemon bot commented Mar 23, 2024

BundleMon (Integration Projects)

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

No change in files bundle size

Final result: ✅

View report in BundleMon website ➡️


Current branch size history | Target branch size history

@arturovt arturovt force-pushed the feat/select-for-signal branch from 962a2ef to 49c8baf Compare March 23, 2024 22:47
@arturovt arturovt changed the title feat(store): expose function to select signal feat(signals): provide @ngxs/signals package with signals utilities Mar 23, 2024
@rhutchison
Copy link
Contributor

rhutchison commented Mar 23, 2024

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 arturovt force-pushed the feat/select-for-signal branch from 49c8baf to 2308a9e Compare March 24, 2024 00:00
// There's no reason to call the following function with an empty object as
// `produceActions({})` or `produceSelectors({})`. We should prevent invalid
// or incorrect use cases.
export type RequireAtLeastOneProperty<T> = keyof T extends never ? never : T;
Copy link
Contributor

Choose a reason for hiding this comment

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

if we're using type-fest, you can use NonEmptyObject

import { ActionDef, Store } from '@ngxs/store';
import { Observable } from 'rxjs';

import { RequireAtLeastOneProperty } from './types';
Copy link
Contributor

Choose a reason for hiding this comment

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

import type { NonEmptyObject } from 'type-fest';

import { Signal, inject } from '@angular/core';
import { Store, TypedSelector, ɵSelectorReturnType } from '@ngxs/store';

import { RequireAtLeastOneProperty } from './types';
Copy link
Contributor

Choose a reason for hiding this comment

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

import type { NonEmptyObject } from 'type-fest';

Copy link
Member

@markwhitfeld markwhitfeld left a comment

Choose a reason for hiding this comment

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

I think that this is a really great, simple utility. Nicely done!
Just needs docs ❤️

packages/signals/tests/produce-selectors.spec.ts Outdated Show resolved Hide resolved
packages/signals/types/tests/produce-actions.lint.ts Outdated Show resolved Hide resolved
@arturovt arturovt force-pushed the feat/select-for-signal branch from 2308a9e to d111778 Compare March 24, 2024 11:10
@arturovt
Copy link
Member Author

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?

@markwhitfeld
Copy link
Member

markwhitfeld commented Mar 24, 2024

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
Copy link
Contributor

rhutchison commented Mar 24, 2024

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.

@arturovt arturovt force-pushed the feat/select-for-signal branch from 995e18f to 5c7f03e Compare March 25, 2024 05:33
@arturovt arturovt marked this pull request as ready for review March 25, 2024 05:33
@markwhitfeld
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member Author

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.

@ngxs ngxs deleted a comment from codeclimate bot Mar 25, 2024
@markwhitfeld
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

markwhitfeld commented Mar 25, 2024

In terms of naming, we have a few options:

  • produceSelects
  • makeSelectMap
  • createSelectMap
  • produceSelectMap
  • toSelectMap

@rhutchison
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member Author

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

@ngxs ngxs deleted a comment from codeclimate bot Mar 25, 2024
@Carniatto
Copy link
Contributor

Carniatto commented Mar 26, 2024

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
Copy link
Contributor

dmitry-stepanenko commented Mar 26, 2024

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
Copy link
Contributor

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*

Copy link
Member

@markwhitfeld markwhitfeld left a comment

Choose a reason for hiding this comment

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

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

@ngxs ngxs deleted a comment from codeclimate bot Mar 26, 2024
@rhutchison
Copy link
Contributor

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 #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
Copy link
Member Author

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
Copy link
Contributor

I believe it's ready to merge

@arturovt arturovt merged commit b4d0487 into master Mar 27, 2024
12 of 13 checks passed
@arturovt arturovt deleted the feat/select-for-signal branch March 27, 2024 14:55
@arturovt
Copy link
Member Author

Merged. Can address any issues in subsequent PRs.

@markwhitfeld
Copy link
Member

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.

@markwhitfeld markwhitfeld added this to the v.18.0.0 milestone Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants