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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug(core): EventEmitter#subscribe() should be properly typed #29016

Open
jorroll opened this issue Feb 27, 2019 · 5 comments
Open

bug(core): EventEmitter#subscribe() should be properly typed #29016

jorroll opened this issue Feb 27, 2019 · 5 comments
Labels
area: core Issues related to the framework runtime breaking changes core: inputs / outputs cross-cutting: types P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Milestone

Comments

@jorroll
Copy link
Contributor

jorroll commented Feb 27, 2019

馃殌 feature request

Relevant Package

This feature request is for @angular/core

Description

Currently, subscribing to an EventEmitter (via EventEmitter#subscribe()) stripes the EventEmitter's type information and converts it to any. This is undesirable, as already pointed out in #6934.

That issue was closed with a nondescript explanation that ~ "you shouldn't use the EventEmitter's subscribe() method". However, EventEmitter#subscribe() is not marked as internal API.

The suggestion that someone should create a duplicate property, using an rxjs subject, solely for the purpose of improved typing, is unsatisfactory.

This is a request to do whatever needs to be done to allow subscribing to EventEmitter directly while preserving type information.

Describe the solution you'd like

The type information, which is already present on EventEmitter, should be retained.

Describe alternatives you've considered

Currently, the suggested workaround seems to be to create a duplicate property and update both the @Output() property and the duplicate Subject property whenever the associated data changes.

e.g.

class MyComponent {
  @Input() name: string;
  
  @Output()
  nameChange = new EventEmitter<string>();
  nameChange2 = new Subject<string>();

  @Input() weather: {prediction: string};
  
  @Output()
  weatherChange = new EventEmitter<{prediction: string}>();
  weatherChange2 = new Subject<{prediction: string}>();

  private updateName(name: string) {
    this.name = name;
    this.nameChange.emit(name);
    this.nameChange2.next(name);
  }

  private weatherName(value: {prediction: string}) {
    this.weather = value;
    this.weatherChange.emit(value)
    this.weatherChange2.next(value)
  }
}
``
@pkozlowski-opensource pkozlowski-opensource added the area: core Issues related to the framework runtime label Feb 28, 2019
@ngbot ngbot bot added this to the needsTriage milestone Feb 28, 2019
@mhevery mhevery added the feature Issue that requests a new feature label Apr 4, 2019
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Apr 4, 2019
@william-lohan
Copy link

william-lohan commented Apr 10, 2019

What about this workaround

this.weatherChange.asObservable().subscribe(weather => {});

mouse over weather is { prediction: string; } not any

https://stackblitz.com/edit/angular-issues-29016?file=src%2Fapp%2Fmy%2Fmy.component.ts

@jorroll
Copy link
Contributor Author

jorroll commented Apr 11, 2019

@william-lohan thanks! That's good enough for me.

@jorroll jorroll closed this as completed Apr 11, 2019
@Airblader
Copy link
Contributor

I still think this should be typed correctly right away, so I would vote to not close this.

@jorroll
Copy link
Contributor Author

jorroll commented Apr 11, 2019

@Airblader ok, I'll leave open for now.

@jorroll jorroll reopened this Apr 11, 2019
@michaeljota
Copy link

I made a simple implementation based on overloads to follow the current implementation. If you want, I can make a PR with this

playground link

In the Playground all three are showed, but with the overload, only the first two will, and also will allow to discriminate between the calls.

@kara kara added cross-cutting: types type: confusing and removed feature Issue that requests a new feature labels May 27, 2020
@ngbot ngbot bot modified the milestones: Backlog, needsTriage May 27, 2020
@kara kara added the triage #1 label May 27, 2020
@jelbourn jelbourn added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent and removed type: confusing labels Oct 12, 2020
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Oct 12, 2020
@jelbourn jelbourn changed the title feat(core): EventEmitter#subscribe() should be properly typed bug(core): EventEmitter#subscribe() should be properly typed Oct 12, 2020
JeanMeche added a commit to JeanMeche/angular that referenced this issue Jan 2, 2023
When passed an observer object, the next callback had any as argument, this commit improve this behavior.

Fixes: angular#29016
JeanMeche added a commit to JeanMeche/angular that referenced this issue Jan 2, 2023
When passed an observer object, the next callback had any as argument, this commit improve this behavior.

Fixes: angular#29016
JeanMeche added a commit to JeanMeche/angular that referenced this issue Jan 2, 2023
When passed an observer object, the next callback had any as argument, this commit improve this behavior.

Fixes: angular#29016
JeanMeche added a commit to JeanMeche/angular that referenced this issue Jan 2, 2023
When passed an observer object, the next callback had any as argument, this commit improve this behavior.

Fixes: angular#29016
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 breaking changes core: inputs / outputs cross-cutting: types P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants