-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix(fromEvent): match targets properly; fix result selector type #6208
Conversation
WTF. 🙄 lol |
@@ -70,11 +70,11 @@ export interface AddEventListenerOptions extends EventListenerOptions { | |||
|
|||
export function fromEvent<T>(target: FromEventTarget<T>, eventName: string): Observable<T>; | |||
/** @deprecated resultSelector no longer supported, pipe to map instead */ | |||
export function fromEvent<T>(target: FromEventTarget<T>, eventName: string, resultSelector?: (...args: any[]) => T): Observable<T>; | |||
export function fromEvent<T>(target: FromEventTarget<T>, eventName: string, options?: EventListenerOptions): Observable<T>; | |||
export function fromEvent<T>(target: FromEventTarget<any>, eventName: string, resultSelector: (...args: any[]) => T): Observable<T>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to make sense. We're loosing up the type here, because really it's the T
that matters. However, part of me feels like we should undeprecate this signature (as discussed in #5824), and in that case, we should try to get the types correct that are being passed to the result selector. I suppose we can do that in another pass though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. That's something that can be done after it's split into separate signatures. ATM, FromEventTarget
makes the function look simpler than it really is and the T
is really only applicable to some of the signatures.
Description:
This PR fixes some problems with
fromEvent
:extends
behaves in an unintended manner and the types won't match.resultSelector
returnsT
, the target's type should not beFromEventTarget<T>
.The typing of
fromEvent
could be improved, but I think the first step is to fix these problems. The next would be to replace theFromEventTarget
type with separate signatures.Related issue (if exists): Nope