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

toSignal causes the signal to not being lazy-evaluated #55337

Open
csisy-bt4w opened this issue Apr 14, 2024 · 6 comments
Open

toSignal causes the signal to not being lazy-evaluated #55337

csisy-bt4w opened this issue Apr 14, 2024 · 6 comments
Labels
area: core Issues related to the framework runtime cross-cutting: signals
Milestone

Comments

@csisy-bt4w
Copy link

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

core

Description

First, we implemented a model (with validation logic and so on) with signals. Howver, since a signal/effect cannot be created anywhere (must be in injection context and outside of reactive context), we opted out and decided to use rxjs instead. This is fine, since the model is actually independent of Angular itself.

Additionally, we have a component that consists of pages. Each page can display validation errors found on that given page. The validation errors come from the model stated above.

Previously, while the model used signals, we were able to take advantage of the lazyness of the signal read operations. Consider the following simplified example:

// model class
readonly errors = computed<string[]>(() => ...);

// page class
readonly model = input.required<Model>();
readonly visited = signal(false);
readonly pageErrors = computed(() => this.visited() ? this.model().errors() : []);

So when we used (read) the pageErrors we only evaluated the model's errors if the page were ever visited.

Using signals in components is comfortable so we used the toSignal from rxjs-interop, something like this:

// model class
readonly errors$: Observable<string[]> = ...

// page class
readonly modelErrors = toSignal(model.errors$, { initialValue: [] });
readonly pageErrors = computed(() => this.visited() ? this.modelErrors() : []);

However, the toSignal call has a non-trivial consequence that it subscribes immediately, losing the lazy-evaluation aspect. The other way around (toObservable) is pretty much the same, maybe even worse (since it uses effect in its implementation that always runs at least once).

Proposed solution

I think the toSignal and toObservable should get a deeper integration, allowing us to avoid use-cases like this. It would be the best if all computed signal properties (lazy + memoized) could be kept when using toSignal - only subscribe when the signal value is read, and unsubscribe when the signal's refcount becomes 0 (all its listeners are unsubscribed).

Alternatives considered

Currently there is no appropriate workaround, at least we could not find any. We could opt-out of using signals, but it has its advantages that we'd like to keep.

@JeanMeche
Copy link
Member

JeanMeche commented Apr 14, 2024

Hi!

If we take the problem the other way around, wouldn't it be more surprising that the signal didn't subscribe eagerly, ie probably missing some values emited by the observable ? This wasn't an issue with the signals because you always get the latest value when reading them.

Also can you give us more insight, what are the consequences for you that model.errors$ is subscribed eargerly ?

Edit: For the sake of completeness, this specific behavior can be implemented in userland. This has already been done by the ngxtension library with toLazySignal.

@JeanMeche JeanMeche added needs reproduction This issue needs a reproduction in order for the team to investigate further area: core Issues related to the framework runtime cross-cutting: signals labels Apr 14, 2024
@ngbot ngbot bot added this to the Backlog milestone Apr 14, 2024
@csisy-bt4w
Copy link
Author

csisy-bt4w commented Apr 14, 2024

Hello!

It depends on the point of view. In the signal world, basically everything is lazy-evaluated, so I wouldn't mind if the subscription doesn't happen immediately. We could catch any "missing" value with a piped shareReplay({ bufferCount: 1, refCount: true }) if I understand it correctly (just a quick thought).

The error$ observable is just an example, but the consequence is processing time. For example, it can be even worse if async validation is used which can be arbitrarily complex. And if the observable emits immediately on subscription (which can be true for Observables created from BehaviourSubjects, ReplaySubjects, etc.), it might start network calls, for example.

Additionally, Observables are cold/lazy as well until there is at least one active subscriber. And for me, creating a signal from an observable should not represent an active subscription until the signal itself is used somewhere.

Thanks for the link, we will take a look at it.

@alxhub
Copy link
Member

alxhub commented Apr 16, 2024

I think toSignal is actually what you want here.

It's intended to be used like the async pipe - converting to signals at the point where the data is used inside of a component. This ensures that the subscription is managed correctly and cleaned up when the data from the Observable is no longer in use (unlike toLazySignal which leaks subscriptions).

That is, the toSignal call itself is the trigger that the cold observable should be pulled, not the usage of the resulting signal in the template.

@e-oz
Copy link

e-oz commented Apr 16, 2024

@alxhub, toLazySignal() doesn't leak subscriptions.
It is a wrapper around toSignal(), and toSignal() manages subscriptions using an injector that is passed to toSignal().
So when a component is destroyed, subscriptions will be terminated by the underlying toSignal() (if manualCleanup is not set - all the options are transferred to toSignal()).
toLazySignal() asserts that it runs in an injection context.

You can see and try it here:
https://stackblitz.com/edit/stackblitz-starters-cysf3d?devToolsHeight=75&file=src%2Fmain.ts

@alxhub alxhub removed the needs reproduction This issue needs a reproduction in order for the team to investigate further label Apr 16, 2024
@ngbot ngbot bot modified the milestones: Backlog, needsTriage Apr 16, 2024
@alxhub
Copy link
Member

alxhub commented Apr 16, 2024

Yeah, I should be more clear. toLazySignal itself doesn't truly "leak" subscriptions, but it's easy to end up in a situation where you don't realize that it won't unsubscribe when you stop pulling the signal in your template. E.g. using toLazySignal in a service, thinking that it'll maintain the same semantics as cold observables.

@csisy-bt4w
Copy link
Author

csisy-bt4w commented Apr 19, 2024

I think toSignal is actually what you want here.

It's intended to be used like the async pipe - converting to signals at the point where the data is used inside of a component. This ensures that the subscription is managed correctly and cleaned up when the data from the Observable is no longer in use (unlike toLazySignal which leaks subscriptions).

That is, the toSignal call itself is the trigger that the cold observable should be pulled, not the usage of the resulting signal in the template.

These are valid points, however the async pipe is kind of lazy as well. In this example the observable is not subscribed until the async pipe is created, and unsubscribed when the pipe is destroyed.

We could create the signal from the observable when we actually need it (similarly how the async pipe is created on the fly), but there are other design choices (e.g. immutability, DI, etc.) that could prevent this. And just by looking at the definition / usage of the signals, one could expect that the signals should be just created at construction time (by toSignal, input, signal, computed or by any other means) and used when needed.

Back to the original example:
Imagine that we have these pages that can produce error messages. These error messages are constructed from validation errors by an observable (through rxjs mapping, for example). The component that displays the errors of the current page is only interested in the error messages produced by the active page. Additionally, there could be use-cases when we don't want to subscribe to the validation error messages at all.

With signals, this can be easily implemented, because (from the docs): "Computed signals are both lazily evaluated and memoized"

readonly validationResults: Signal<...>;
readonly validationErrors = computed(() => createErrorMessages(this.validationResults()));
readonly isErrorsNeeded = signal(false);
readonly errors = computed(() => {
  if (!this.isErrorsNeeded()) {
    return [];
  }

  return this.validationErrors();
});

The errors itself is only read for the active page and the validationErrors is only read if the current value of isErrorsNeeded is true.

However, since the validationErrors comes from a library as an Observable<string[]>, it would be nice if we could just drop-in-replace the computed signal with toSignal(validationErrors$). And since we cannot (and shouldn't) create signals in computed signals, there is no way to easily integrate rxjs-based libraries into lazy-evaluated signal-based components.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: core Issues related to the framework runtime cross-cutting: signals
Projects
None yet
Development

No branches or pull requests

4 participants