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

@ngrx/signals: add deepComputed function #4202

Open
1 of 2 tasks
JulienLecoq opened this issue Jan 5, 2024 · 7 comments
Open
1 of 2 tasks

@ngrx/signals: add deepComputed function #4202

JulienLecoq opened this issue Jan 5, 2024 · 7 comments

Comments

@JulienLecoq
Copy link

JulienLecoq commented Jan 5, 2024

Which @ngrx/* package(s) are relevant/related to the feature request?

signals

Information

I would like to have a computed() function similar to the one provided by Angular for Signals but in this case, instead of returning a Signal, it would return a DeepSignal.

This would allow to optimise template rendering for computed states based on objects.

Exemples:

  • Current way of using computed variables:
<p>
    {{ computedState().firstName }}
</p>
  • Proposed way of using computed variables:
<p>
    {{ computedState.firstName() }}
</p>

Describe any alternatives/workarounds you're currently using

I'm using the computed() function provided by Angular which returns a Signal.

I would be willing to submit a PR to fix this issue

  • Yes
  • No
@brandonroberts
Copy link
Member

brandonroberts commented Jan 5, 2024

Unless I'm mistaken on your request, then is what the signalState function does.

https://ngrx.io/guide/signals/signal-state

We wouldn't created a computed function that does what signalState does, as it might be confusing.

@JulienLecoq
Copy link
Author

JulienLecoq commented Jan 6, 2024

No, signalState creates a "deep" signal typed as SignalState.
What I would like to have is the ability to get a computed deep signal.

For signals we have:

count = signal(0)

doubleCount = computed(() => {
    return this.count() * 2
})

I would like the same but for deep signals, so something like:

count = signal(0)

doubleCount = computedState(() => {
    return this.count() * 2
})

Hence, doubleCount would be a SignalState instead of a Signal.

For primitives, this isn't very useful. But this would be useful for non-primitive values.

Examples with non-primitive values:

  • Signal based approach
profile = signalState({
   firstName: "Georges",
   lastName: "Bob",
})

computedSignal = computed(() => {
    return {
        firstName: this.profile.firstName(),
        lastName: "Alice"
    }
})
  • SignalState approach
profile = signalState({
   firstName: "Georges",
   lastName: "Bob",
})

computedSignalState = computedState(() => {
    return {
        firstName: this.profile.firstName(),
        lastName: "Alice"
    }
})

Do not really pay attention to the body of the computed() and computedState() functions, this is just to give you a quick example.

@JulienLecoq
Copy link
Author

JulienLecoq commented Jan 6, 2024

Right know, you can only have signals when using computed(), which give you this when using it:

<p>
    {{ computedSignal().firstName }}
</p>

Whereas with a deep signal that you could get with computedState(), you could do:

<p>
    {{ computedSignalState.firstName() }}
</p>

Which is better for template rendering/change detection. With the computedSignalState solution, change detection would only be triggered if firstName has changed. Whereas, with the computedSignal solution, change detection is triggered every time computedSignal is updated, even if firstName hasn't changed.

@markostanimirovic
Copy link
Member

We already considered this feature some time ago - deepComputed:

const counts = signalState({ count1: 1, count2: 2 });

const doubleCounts = deepComputed(() => ({
  count1: counts.count1() * 2,
  count2: counts.count2() * 2,
});


console.log(doubleCounts()); // { count1: 2, count2: 4 }
console.log(doubleCounts.count1()); // 2
console.log(doubleCounts.count2()); // 4

@markostanimirovic markostanimirovic changed the title Add computed(...): SignalState<T> function to @ngrx/signals @ngrx/signals: add deepComputed function Jan 6, 2024
@JulienLecoq
Copy link
Author

Cool, what was the result of this consideration?

Btw, if it is introduced later on in the library with the name deepComputed, wouldn't it make sense to rename signalState to deepSignal? Maybe it's just me, but I find the name deepSignal more descriptive than signalState. I myself created a wrapper around signalState so that it's called deepSignal when I use it haha.

@ducin
Copy link
Contributor

ducin commented Feb 12, 2024

@JulienLecoq I get the idea that the API is different and you like it more, but I'm not sure what problem are you trying to solve here. Since computeds are cached anyway, don't think perf could make much difference here.

Especially in this section:

Describe any alternatives/workarounds you're currently using

I'm using the computed() function provided by Angular which returns a Signal.

In what way would the proposed deepComputed be better?

IMO the fact that e.g. the store itself is NOT a signal, or that we've got DeepSignals, etc. - all these are implementation details. The wonderful thing in the signal store design is its simplicity. By exposing additional withDeepComputed (or whatever) the complexity gets higher. It appears as an abstraction leak to me.

Currently the fact that signalStore creates deep signals is hidden behind a proxy. And thanks to precise typing, you can easily use those deep signals without even knowing how all those things work. You can't decide directly whether a piece in signal store is a DeepSignal vs Signal (correct me @markostanimirovic if I'm mistaken). IMO encapsulating computed vs DeepComputed makes a better approach - as it doesn't leak abstraction.

@Maransatto
Copy link

Maransatto commented Mar 21, 2024

@ducin
Here is a scenario where withDeepComputed would be useful:

I have a RecipeStore which is a SignalStore as the root store.
Then I also have a withControlPoints() function to get a signalStoreFeature() and that adds to the root state an attribute controlPoints. But if you look at the RecipeStore attributes from their initial state, they are all DeepSignals, while the attributes derived from the computed signalStoreFeatured are regular signals.

In the end, when we access the store from a component, we see all the attributes, but some of them are deepSignals (from the initial state) while others are regular Signals (from computed). This is confusing.

The ideal would be to return DeepSignals when using withComputed() by default.

Edit:
When accessing a store from a component or service, I would expect to have all the attributes as DeepSignals, not only the ones created on the initialState.

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

No branches or pull requests

5 participants