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

Add observed property to OutputEmitterRef #54837

Open
GuillaumeNury opened this issue Mar 13, 2024 · 10 comments
Open

Add observed property to OutputEmitterRef #54837

GuillaumeNury opened this issue Mar 13, 2024 · 10 comments
Labels
area: core Issues related to the framework runtime core: inputs / outputs feature Issue that requests a new feature
Milestone

Comments

@GuillaumeNury
Copy link

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

core

Description

With the @Output decorator, I used to check the observed of the EventEmitter (because it inherits Subject) to enable/disable some features.

For example, <lu-select (clueChange)="..." /> would display a search bar in LuSelect component but <lu-select /> would not.

This solution seems elegant as it avoid an additional input that would lead to unwanted cases:

  • Both <lu-select enableClue (clueChange)="..." /> and <lu-select /> would work
  • <lu-select (clueChange)="..." /> would be useless because the search bar is not disabled because enableClue = false
  • <lu-select enableClue /> would be useless because the search bar is displayed, but nobody listen to its value

Proposed solution

Add a getterproperty observed in OutputEmitterRef

(in this file)

export class OutputEmitterRef<T> implements OutputRef<T> {
  private listeners: Array<(value: T) => void>|null = null;

  get observed(): boolean {
    return !!this.listeners?.length;
  }

  //...
}

Alternatives considered

Replace existing usages of emitter.observed by inputs.

@alxhub
Copy link
Member

alxhub commented Mar 13, 2024

If we did expose such a property, it would probably be a Signal<boolean>.

@alxhub alxhub added feature Issue that requests a new feature area: core Issues related to the framework runtime core: inputs / outputs labels Mar 13, 2024
@ngbot ngbot bot added this to the Backlog milestone Mar 13, 2024
@GuillaumeNury
Copy link
Author

It would be even better!

@kbrilla
Copy link

kbrilla commented Mar 13, 2024

What about output.required? which would error if no listeners would be registred? - of course I can create separate issue if needed, but it seems like aother solution to the same problem.

@GuillaumeNury
Copy link
Author

The output would need to be required only when an input has a specific value (if I understand what is on your mind)

@kbrilla
Copy link

kbrilla commented Mar 13, 2024

Yeah, Now I see difrence, in your proposition it’s more about conditional required based on value of some input / directive. So yeah I will open new issue as both solutions solve difrent issues. Sory for clogging this issue

@devversion
Copy link
Member

Yeah, this is one of the follow-up's we did consider and would implement using a Signal<boolean> as Alex pointed out.

For the use-cases, we did see a similar pattern, especially with expensive subscriptions only being triggered upon "interest from the parent".

The initial thinking still was that an input likely makes more sense here. Subjectively, it would seem confusing to me if the behavior of a component changes based on an output that I subscribe to. That is because "inputs" can "configure" the directive, but outputs simply emit values to the parent conceptually. It's more explicit and keeps the mental model simpler IMO.

In either case, adding such a signal to OutputRef is trivial.

@Harpush
Copy link

Harpush commented Mar 14, 2024

One use case for this is wrapping third party (non angular) libraries. For example if such library allow binding to mouse move event and I want to allow my component user to add a (for example) chart mouse move output I will need to always listen to the third party event. If I could know if output was declared I could optimize and won't listen to that event unnecessarily.
An input here doesn't make sense as the one using my component shouldn't configure internal optimization based on how the wrapped third party works.

@yjaaidi
Copy link
Contributor

yjaaidi commented Apr 8, 2024

Very interesting issue. Thanks @GuillaumeNury for raising this.

TL;DR: I wouldn't recommend changing OutputRef API for this as there are decent solutions but if we really go that road than we have to think about what we really want to expose (a boolean? the count? the listeners?)

I totally agree with @devversion on this except on this point:

Subjectively, it would seem confusing to me if the behavior of a component changes based on an output that I subscribe to.

where I think that this is not subjective, but really objective 😊

First, I think that it makes outputs align with DOM event listeners which do not affect DOM behavior.
Additionally, this assumption is also an enabler for testing tools or dev tools. For example, some dev/testing tool could track all component outputs to debug them manually or to compare them to a previous run etc...

In the case of performance optimization, I think that outputFromObservable is a the best alternative to subscribe lazily to expensive sources (like @Harpush's example of a 3rd party mouse tracking lib).

That is why I'd generally recommend being explicit and use inputs in Guillaume's case.

This way we can reduce the OutputRef's API surface and keep it simple.

Meanwhile, if you really want to go with magic 😊, or if it is really needed to debug or optimize something, the solution could be a SnitchyEventEmitter 😉: https://stackblitz.com/edit/angular-output-snitching?file=src%2Fmain.ts

If some additional API should be added to OutputRef, then we have to think of whether it's a boolean value or the count of listeners or maybe the listeners?
Indeed, a component might want to throw an error if an output is subscribed to more than once (this can happen through imperative APIs).
For the listeners themselves, I don't have a use case, but for some testing scenarios (that I wouldn't recommend), one might want to trigger a listener from the test.

@GuillaumeNury
Copy link
Author

As observed is not likely to change (from the template you cannot subscribe from an output and then cancel the cancellation), I can use a simple Subject and outputFromObservable to achieve what I need:

@Component({
  standalone: true,
  selector: 'app-child',
  template: `
  <h2>{{ actionEmitter.observed ? '👀' : '🙈' }}</h2>
  <button
    [disabled]="!actionEmitter.observed"
    (click)="actionEmitter.next()">CLICK</button>
  `,
})
export class Child {
  actionEmitter = new Subject<void>();
  action = outputFromObservable(this.actionEmitter);
}

(observed is not a signal anymore but a regular boolean. Subject already track its observers)

Stackblitz : https://stackblitz.com/edit/angular-output-snitching-wd3hf4?file=src%2Fmain.ts

Thanks @yjaaidi for the pointer (and the stackblits)!

As the workaround looks simple, maybe we can close this issue?

@yjaaidi
Copy link
Contributor

yjaaidi commented Apr 8, 2024

Cool! Happy to hear that this works for you.

is not likely to change (from the template you cannot subscribe from an output and then cancel the cancellation
Indeed.

Not likely but still possible in case there is an imperative subscription (e.g. testing or viewChild() usage or dynamic creation). So in that case, the view might not be updated if using ChangeDetectionStrategy.OnPush (or this would cause in error in the future signal-based components).

Just an observation here for anyone reading this in the future. Not all observables have the observed property.
In case the source is not a Subject but an Observable (e.g. FormGroup#valueChanges), then you might need something similar to countObservers operator from the stackblitz above.

arturovt added a commit to arturovt/angular that referenced this issue May 14, 2024
Prior to this commit, it wasn't possible to check whether the output had
any listeners (if it was being observed). Before `output()`, we could check
whether `EventEmitter` had any listeners by checking its `observed` property.

In this commit, we convert `listeners` from a list to a signal so that we can
have a computed `observed` signal.

This commit only updates the `OutputEmitterRef` implementation. We can't update
`OutputFromObservableRef` because it expects an observable signature to be
provided, and the observable doesn't technically have any API to count
subscribers; only `Subject` does.

Please note that there have been different discussions on whether the
`OutputEmitterRef` should be extended or not. This change is not considered
breaking because the signature of the _private_ property has been changed and
another property has been added to the public API.

Related issue: angular#54837
arturovt added a commit to arturovt/angular that referenced this issue May 14, 2024
Prior to this commit, it wasn't possible to check whether the output had
any listeners (if it was being observed). Before `output()`, we could check
whether `EventEmitter` had any listeners by checking its `observed` property.

In this commit, we convert `listeners` from a list to a signal so that we can
have a computed `observed` signal.

This commit only updates the `OutputEmitterRef` implementation. We can't update
`OutputFromObservableRef` because it expects an observable signature to be
provided, and the observable doesn't technically have any API to count
subscribers; only `Subject` does.

Please note that there have been different discussions on whether the
`OutputEmitterRef` should be extended or not. This change is not considered
breaking because the signature of the _private_ property has been changed and
another property has been added to the public API.

Related issue: angular#54837
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 core: inputs / outputs feature Issue that requests a new feature
Projects
None yet
Development

No branches or pull requests

6 participants