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

Smarter types for fromEvent #5512

Open
waterplea opened this issue Jun 20, 2020 · 9 comments
Open

Smarter types for fromEvent #5512

waterplea opened this issue Jun 20, 2020 · 9 comments
Labels
TS Issues and PRs related purely to TypeScript issues

Comments

@waterplea
Copy link

Feature Request

Is your feature request related to a problem? Please describe.
When you create Observable with fromEvent you have to manually define generic and in received event target type is not inferred from element we provided although to my knowledge DOM event target is always the same element you listen event on. You have to typecast.

Describe the solution you'd like
Here's a proof of concept for a better typed s solution:
https://stackblitz.com/edit/angular-typed-fromevent
It's only for traditional DOM events overload

Describe alternatives you've considered
I've been using wrapper function from example above in my project. Could be great if it was unnecessary and it was built-in in RxJS.

Additional context
I'm willing to work on a PR but I might need assistance with other overloads as I'm not that familiar with them.

@cartant
Copy link
Collaborator

cartant commented Jun 20, 2020

The problem is that GlobalEventHandlersEventMap is declared in TypeScript's dom library - see here - but fromEvent isn't just for DOM events. It works with Node events too - see here.

AFAICT, using GlobalEventHandlersEventMap in the type declarations would necessitate the inclusion the dom library in environments that don't involve the DOM. And that's the principle blocker for this.

@waterplea
Copy link
Author

Is it possible to break TypeScript apart and only use particular parts of it? What would be the benefits of doing so since it installs altogether and only exists in declaration files that take no space and do not make it into compiled sources?

@cartant
Copy link
Collaborator

cartant commented Jun 21, 2020

TypeScript is already broken apart into libraries that contains specific type declarations - the types for said libraries are included by adding the libraries via TypeScript's lib compiler option.

To get the GlobalEventHandlersEventMap, the dom library needs to be specified. And that is the problem. If that type is used in the declaration for fromEvent, Node developers will have to specify dom in their lib compiler option. And that will also have the effect of introducing the DOM types for APIs like setInterval that differ from APIs with the same name in @types/node. Specifying dom will also introduce types for APIs that do not exist in the Node environment.

In short, that has been a problem before and some effort was made to remove the dependency on the dom library from the fromEvent type declarations - see #3566 and the related issue.

An alternative approach could be to provide an additional overload signature to fromEvent - in a separate module or package - using declaration merging. That is, you wouldn't need to implement a wrapping function, you could just provide an alternative signature that used the GlobalEventHandlersEventMap type and required the dom library to be specified. It would look something like this:

declare module "rxjs/internal/observable/fromEvent" {
  export function fromEvent</* whatever */>(/* whatever */): /* whatever */;
}

TypeScript would then attempt to match the merged signature before is matched any of the signatures declared in the RxJS codebase for fromEvent.

@niklas-wortmann
Copy link
Member

If there is really no way to work around this, and you are still interested in releasing it in a node module, this could go to rxjs-web. It's still in an alpha version due to the fact that I am a slacker and need to implement tests :D

@cartant
Copy link
Collaborator

cartant commented Jun 21, 2020

The only way that I can see it working in the RxJS codebase is if there are separate import locations for DOM and non-DOM type declarations. With the former requiring the dom TypeScript library to be specified in the consuming application's tsconfig.json.

@kwonoj kwonoj added the TS Issues and PRs related purely to TypeScript issues label Jun 21, 2020
@devanshj
Copy link

devanshj commented Jul 2, 2020

FWIW: #4891, rxjs-from-emitter (type-safe fromEvent not only for DOM but literally any event emitter)

@chrisguttandin
Copy link
Contributor

Hi everyone, I just stumbled upon this. What do you think about using TypeScript's template strings to infer the event types?

I used that technique to implement the on() method of subscribable-things. The downside is that it requires TypeScript v4.1+. Not sure if RxJS v7 will require that anyway.

This is how it could look when applied to fromEvent:

type EventHandler<T, U extends Event = Event> = ThisType<T> & ((event: U) => void);

type EventTargetWithPropertyHandler<T extends EventTarget, U extends string, V extends Event> = {
    [P in U as `on${P}`]: null | EventHandler<T, V>;
};

type EventType<T extends EventTarget, U extends string> = T extends EventTargetWithPropertyHandler<T, U, infer V> ? V : Event;

declare function fromEvent<T extends EventTarget, U extends string>(target: T, eventName: U): Observable<EventType<T, U>>;

It basically works by looking up the event by it's type. It searches for the property handler with the same name and then uses the type definition of that handler.

When using it like this ...

fromEvent(messagePort, 'message');

... TypeScript will infer the event type from the type definition of messagePort.onmessage.

The type definition would need to be modified a bit to work without the pre-defined EventTarget and Event types but I think it should still work as intended.

It will unfortunately not help to improve the types for events when writing code for Node.js.

@cartant
Copy link
Collaborator

cartant commented Apr 5, 2021

The downside is that it requires TypeScript v4.1+. Not sure if RxJS v7 will require that anyway.

TypeScript 4.2 will be the minimum-supported version.

What do you think about using TypeScript's template strings to infer the event types?

Maybe. There are some other things that need to be fixed before this could be done - see #6208. Specifically, the FromEventTarget union type needs to be replaced with multiple overload signatures.

@thynson
Copy link

thynson commented Apr 30, 2021

#6214 actually cause much more typing error for valid code, due to Typescript's inability to resolve function overload that contains union typed parameters.
I would suggest to separate fromEvent to fromDomEvent, fromNodeEvent(or fromEventEmitter), etc., and use option object instead of function overloading to cover each case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TS Issues and PRs related purely to TypeScript issues
Projects
None yet
Development

No branches or pull requests

7 participants