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): add deepComputed function #4539

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

markostanimirovic
Copy link
Member

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

Closes #4202

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Copy link

netlify bot commented Sep 28, 2024

Deploy Preview for ngrx-io canceled.

Name Link
🔨 Latest commit c777c77
🔍 Latest deploy log https://app.netlify.com/sites/ngrx-io/deploys/66f76b2388e73f00089e263d

@markostanimirovic markostanimirovic force-pushed the feat/signals/deep-computed branch from 48c55f0 to c777c77 Compare September 28, 2024 02:34
Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

👍

@brandonroberts
Copy link
Member

Q: Why wouldn't signalState use this?

@markostanimirovic
Copy link
Member Author

Q: Why wouldn't signalState use this?

signalState provides a state source to the toDeepSignal function while deepComputed provides a computed signal:

// signalState
const stateSource = signal(initialState);
toDeepSignal(stateSource.asReadonly());

// deepComputed
toDeepSignal(computed(computation));

Instead of stateSource.asReadonly() we can do the following:

// signalState
deepComputed(computed(() => stateSource()));

But I don't see a benefit because the writable signal provides the asReadonly method.

@brandonroberts brandonroberts merged commit 269bd32 into main Oct 1, 2024
11 checks passed
@brandonroberts brandonroberts deleted the feat/signals/deep-computed branch October 1, 2024 17:52
import { computed } from '@angular/core';
import { DeepSignal, toDeepSignal } from './deep-signal';

export function deepComputed<T extends object>(
Copy link

@matheo matheo Oct 16, 2024

Choose a reason for hiding this comment

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

@markostanimirovic what's the reasoning to restrict T to be an object if toDeepSignal supports "any"?
sometimes we have selectors that returns primitives or objects depending on the context 🤔

I mean, I know we should use this only with objects ideally
but our APIs with custom selectors may not have this restriction like:
https://github.com/niklas-wortmann/xstate-angular/blob/main/lib/xstate-ngx/src/lib/useSelector.ts#L25
we need there a deepSignal instead a simple signal for instance

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's a requirement. deepComputed uses the initial value to create a proxy for deeply nested signals.

Copy link

Choose a reason for hiding this comment

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

@markostanimirovic it's basically an alias of toDeepSignal which doesn't have that requirement

Copy link
Member Author

Choose a reason for hiding this comment

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

If a signal with a primitive value is provided to the toDeepSignal function, it's just returned without further processing. Check the implementation: https://github.com/ngrx/platform/blob/main/modules/signals/src/deep-signal.ts#L26-L29

If a primitive type or union that contains primitive is passed to the DeepSignal type, it will be resolved to Signal:

import { DeepSignal } from '@ngrx/signals';

declare const s1: DeepSignal<string>; // type: Signal<string>
declare const s2: DeepSignal<{ foo: string } | string>; // type: Signal<{ foo: string } | string>

I could remove the extends object restriction from the deepComputed input, but then, users may be confused why the return type is resolved to Signal:

const s1 = deepComputed(() => 1 as number | { foo: string }); // type: Signal<number | { foo: string }>

In other words, there is no difference in this example whether you use computed or deepComputed.

With the extends object restriction it's clear - use deepComputed for objects, otherwise, it has no impact and you can go with computed.

Copy link

Choose a reason for hiding this comment

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

yeah, we can do that with known types
but we cannot do that for generic APIs like the one I provided:
https://github.com/niklas-wortmann/xstate-angular/blob/main/lib/xstate-ngx/src/lib/useSelector.ts#L25

the user can be free to define the selector to get a slice of the state
and the Angular library needs to be able to return a deepSignal (or just a signal if it's a primitive value).

Any user using deepComputed in their components may know the type they are computing
and may realize that if they are passing a primitive value it's equivalent to a signal IMO.

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

Successfully merging this pull request may close these issues.

@ngrx/signals: add deepComputed function
4 participants