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(core): add observed to OutputEmitterRef #55793

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arturovt
Copy link
Contributor

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: #54837

@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label May 14, 2024
@arturovt arturovt marked this pull request as ready for review May 14, 2024 17:13
@pullapprove pullapprove bot requested a review from thePunderWoman May 14, 2024 17:13
@thePunderWoman thePunderWoman requested review from alxhub and removed request for thePunderWoman May 15, 2024 20:47
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
@devversion
Copy link
Member

I think we still want to discuss this change more— that's why we didn't create a PR for this. We are not aligned whether it makes sense to add this functionality, or if it just increases API surface unnecessarily and potentially even encourages non-ideal practices.

@devversion
Copy link
Member

I think we should discuss more potential use-cases in the issue. So far, there wasn't too much interest IIRC.

@Harpush
Copy link

Harpush commented May 21, 2024

I think we should discuss more potential use-cases in the issue. So far, there wasn't too much interest IIRC.

Think about a component which has the ability to emit an output on every mouse move. If I knew no one listens to the output there is no reason to even add the event listener. Can really help with non angular third party libraries integration too.

@devversion
Copy link
Member

devversion commented May 21, 2024

@Harpush There are ways to address this issue, which may not be super common, looking at the issue interest— E.g.

  1. Using an observable with outputFromObservable
  2. Using an explicit input to control performance-sensitive outputs.

I'm not opposed against this property. It was something we considered during design, but simply wanted to collect more feedback. It's trivial to add

@Harpush
Copy link

Harpush commented May 21, 2024

@Harpush There are ways to address this issue, which may not be super common, looking at the issue interest— E.g.

  1. Using an observable with outputFromObservable
  2. Using an explicit input to control performance-sensitive outputs.

I'm not opposed against this property. It was something we considered during design, but simply wanted to collect more feedback. It's trivial to add

By using output from observable - only when someone subscribes (which is essentially the same as having a consumer to the output) the observable will actually register the event? What about multiple subscribers? Do I need to share it? Although the chance of multiple subscriptions is extremely low...
An input is always an option but a bit annoying as you let the consumer have control on an internal optimization he shouldn't know of...

@devversion
Copy link
Member

By using output from observable - only when someone subscribes (which is essentially the same as having a consumer to the output) the observable will actually register the event? What about multiple subscribers? Do I need to share it? Although the chance of multiple subscriptions is extremely low...

Arguably, if you are thinking of optimizations like this w/r/t to e.g. a mouse event being expensive, you are likely also considering what would happen with multiple subscribers, or investing time to think how this can be solved.

@dylhunn dylhunn added area: core Issues related to the framework runtime core: inputs / outputs labels May 22, 2024
@ngbot ngbot bot added this to the Backlog milestone May 22, 2024
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 detected: feature PR contains a feature commit state: blocked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants