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

More strictly typed events #4808

Closed
ReneWerner87 opened this issue Aug 25, 2023 · 7 comments
Closed

More strictly typed events #4808

ReneWerner87 opened this issue Aug 25, 2023 · 7 comments
Labels
enhancement New feature or request

Comments

@ReneWerner87
Copy link

ReneWerner87 commented Aug 25, 2023

hi and thx for this @MaximeKjaer
#3822

Observation

I wanted to use socketio together with rxjs and found out that the typescript compiler can no longer follow through the type EventEmitter
https://github.com/socketio/socket.io/blame/fd9dd74eeed3fa6a15c63240df30cc3b7357102f/lib/typed-events.ts#L93
https://github.com/ReactiveX/rxjs/blob/master/src/internal/observable/fromEvent.ts#L62-L110

example code which i have reduced to the essential(click me)
import { fromEvent } from 'rxjs';
import { EventEmitter } from 'events';

interface EventsMap {
    [event: string]: any;
}
declare type EventNames<Map extends EventsMap> = keyof Map & (string | symbol);

// here the event emitter causes problems
class MyTarget<UserEvents extends EventsMap> extends EventEmitter {
// since the eventemitter adds the general event listening ("addListener", "removeListener") methods without strict types
    on<Ev extends EventNames<UserEvents>>(ev: Ev, listener: UserEvents[Ev]) {
        return this;
    }
    off<Ev extends EventNames<UserEvents>>(ev: Ev, listener: UserEvents[Ev]) {
        return this;
    }
}

// socket io event declaration approach - @see https://socket.io/docs/v4/typescript/
interface MyEvents {
    foo: (firstParam: boolean) => void;
}

const target = new MyTarget<MyEvents>();

// expect boolean type for the argument
fromEvent(target, 'foo').subscribe((firstParam) => {
    console.log('firstParam', firstParam);
});

Problem

Since the eventemitter adds the general event listening ("addListener", "removeListener") methods without strict types, the typescript compiler cannot follow the listener argument types

with EventEmitter
image

without EventEmitter
image

same also happens with the direct usage of addListener and removeListener
in example https://socket.io/docs/v4/typescript/#types-for-the-server

socket.addListener('basicEmit', (a, b, c) => {})

Idea

The request would be to get rid of the native EventEmitter and replace it with the methods with a stricter one
so that also the typescript of other libraries can follow and more type support for the other native methods exists

@ReneWerner87 ReneWerner87 added the enhancement New feature or request label Aug 25, 2023
@ReneWerner87
Copy link
Author

ReneWerner87 commented Aug 26, 2023

https://github.com/bterlson/strict-event-emitter-types/tree/master
something like this for the native event emitter for the internal methods addListener and removeListener
the types are already present in the interface as generic and would only have to be used for the method declarations

@ReneWerner87
Copy link
Author

@darrachequesne @MaximeKjaer any opinions on this

@ReneWerner87
Copy link
Author

@phiresky do you have an opinion on this? what would you do in this case?

@darrachequesne
Copy link
Member

I think overriding the type for the addListener() and removeListener() methods in the StrictEventEmitter class here should be sufficient. @ReneWerner87 what do you think?

Note: the approach used here is even more powerful, as there is no intermediary class at runtime. Not sure if we could merge/use it.

@ReneWerner87
Copy link
Author

Yes overwrite should be enough for now, try to create a pull-request later (just have some tasks for my project)

If the author of the other type script lib would have time and desire, he could add or merge his types

ReneWerner87 added a commit to ReneWerner87/socket.io that referenced this issue Sep 13, 2023
@ReneWerner87
Copy link
Author

@darrachequesne
i tried something like this
main...ReneWerner87:socket.io:main

unfortunately do not know how I test the whole project together with my changes
think i need support for the change

just found out that the method addListener is also missing in the emitter interface
https://github.com/socketio/emitter/blob/4810bd7592519c522a11da3852d82e6c15eb074d/index.d.ts#L75-L179

but its there in the js code
https://github.com/socketio/emitter/blob/4810bd7592519c522a11da3852d82e6c15eb074d/index.js#L42-L43

think using this other lib and combining it with yours would improve the types quite a bit

i also saw #4817 , really interesting

@darrachequesne
Copy link
Member

For future readers:

Stricter types have been implemented in #4817. Please reopen if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants