-
Notifications
You must be signed in to change notification settings - Fork 553
Add TypedEventEmitter and integrate IEventProvider with Common Utils #1694
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
Conversation
readonly on: TEvent; | ||
readonly once: TEvent; | ||
readonly prependListener: TEvent; | ||
readonly prependOnceListener: TEvent; |
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.
@DLehenbauer I think i'll keep the verbose set here, as this is already a full event emitter, but our interfaces should expose the minimal set
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, it totally makes sense to include the other inherited APIs when narrowing to the more strongly typed signature. 👍
Hey Tony... I was wondering, are you familiar with the "EventMap" pattern used in 'lib.dom.d.ts' for things like 'addEventListener()'? class TypedEventEmitter<TEventMap> {
on<K extends keyof TEventMap>(event: K, listener: TEventMap[K]) {
super.on(event, listener);
}
...
}
interface IErrorEvents {
"error": (message: any) => void;
}
interface IWidgetEvents {
"foo": (x: number, y: number) => void,
"bar": (s: string) => void,
}
const tee = new TypedEventEmitter<IErrorEvents & IWidgetEvents>(); It does use some advanced features in the TypedEventEmitter declaration, but for consumers of TypedEventEmitter the pattern is straight-forward to follow. The advantage of the EventMap pattern is that creating a new TypedEventEmitter can be very brief: const tee = new TypedEventEmitter<{
"foo": (x: number, y: number) => void,
"bar": (s: string) => void,
}>(); |
No. I wasn't. That's super cool, and maybe a better solution than i've come up with |
@DLehenbauer i played around with the event map a bit, and it doesn't look we can maintain inheritance with event emitter using that technique. keyof TEventMap isn't assignable to string | symbol, also it doesn't seem like intellisence is smart enough to to pick up the events, which is one of the main goals |
Hmmm... that's odd. Let me give it a quick try and see... |
@jack-williams - I wonder if you have any tricks that might help us here. The goal is to create a 'TypedEventEmitter' which narrows the signatures of 'EventEmitter' to ensure the event listener matches the event kind. The "event map" pattern in "lib.dom.d.ts" was looking promising until we subclassed EventEmitter, at which point we started needing additional constraints to convince TypeScript that the narrowed '.on()' and the inherited '.on()' are compatible. The below is close to working... import { EventEmitter } from "events";
interface IEventMap {
[event: string]: (...args: any[]) => void
}
export class TypedEventEmitter<TEventMap extends IEventMap> extends EventEmitter {
public on<K extends keyof TEventMap & string>(event: K, listener: TEventMap[K]): this;
public on(event: string | symbol, listener: (...args: any[]) => void): this {
return super.on(event, listener);
}
}
const x = new TypedEventEmitter<{
"foo": (bar: number) => void
}>();
x.on("foo", (bar: number) => { }); // OK!
x.on("foo", (bar: string) => { }); // Error: type number is not assignable to string But still has an oddity I don't understand where this works: type ErrorEvent = {
"error": (msg: any) => void;
}
new TypedEventEmitter<ErrorEvent>() ...but an equivalent interface does not: interface IErrorEvent {
"error": (msg: any) => void;
}
new TypedEventEmitter<IErrorEvent>() I have a feeling I've gone down the wrong path by piling on more constraints, and was wondering if you might have any thoughts? Thx. |
FWIW I encountered the same problems with inheritance and did not find a clean, non-hacky solution. There is some hacky solution used by one of the libraries out there, but I did not like it. (I think it was this one.) Following this, because I'd love to see a good solution! |
TypeScript treats interfaces as being closed (not having an implicit index signature), while object types do not have this restriction. link. This is the only difference in your example. Here is a small repro of the problem: declare const foo: { x: string };
const dict: Record<string, string> = foo;
8000
// ok
interface Foo {
x: string;
}
declare const foo2: Foo;
const dict2: Record<string, string> = foo2; // error Instead, you could try: import { EventEmitter, Listener } from "events";
export class TypedEventEmitter<TEventMap> extends EventEmitter {
public on<K extends keyof TEventMap & string>(event: K, listener: TEventMap[K] & Listener): this;
public on(event: string, listener: (...args: any[]) => void): this {
return super.on(event, listener);
}
} |
@DLehenbauer, @arinwt and @jack-williams i played around with Jack's ideas a bit and still not luck. I think sticking with my current implementation is best, as it works in the scenarios we care about. We can always continue to explore to see if we can find better syntax later on |
The issue with completions is that Edit: Ah, I'm guessing you maybe mean for the callback? Yes, the intersection breaks there. import { EventEmitter, Listener } from "events";
export class TypedEventEmitter<TEventMap> extends EventEmitter {
public on<K extends keyof TEventMap & string>(event: K, listener: TEventMap[K] extends (...args: any[]) => void ? TEventMap[K] : Listener): this;
public on(event: string, listener: Listener): this {
return super.on(event, listener);
}
} This has better completions I think, but I'm not sure it's better in general compared to the existing approach. |
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.
🍁
No description provided.