socket.io icon indicating copy to clipboard operation
socket.io copied to clipboard

More strictly typed events

Open ReneWerner87 opened this issue 10 months ago • 6 comments

hi and thx for this @MaximeKjaer https://github.com/socketio/socket.io/pull/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 avatar Aug 25 '23 12:08 ReneWerner87

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 avatar Aug 26 '23 12:08 ReneWerner87

@darrachequesne @MaximeKjaer any opinions on this

ReneWerner87 avatar Aug 29 '23 08:08 ReneWerner87

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

ReneWerner87 avatar Sep 13 '23 12:09 ReneWerner87

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.

darrachequesne avatar Sep 13 '23 12:09 darrachequesne

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 avatar Sep 13 '23 15:09 ReneWerner87

@darrachequesne i tried something like this https://github.com/socketio/socket.io/compare/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 https://github.com/socketio/socket.io/pull/4817 , really interesting

ReneWerner87 avatar Sep 13 '23 21:09 ReneWerner87