8000 Add TypedEventEmitter and integrate IEventProvider with Common Utils by anthony-murphy · Pull Request #1694 · microsoft/FluidFramework · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 5 commits into from
Apr 6, 2020

Conversation

anthony-murphy
Copy link
Contributor

No description provided.

readonly on: TEvent;
readonly once: TEvent;
readonly prependListener: TEvent;
readonly prependOnceListener: TEvent;
Copy link
Contributor Author
@anthony-murphy anthony-murphy Apr 3, 2020

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

Copy link
Contributor

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. 👍

@DLehenbauer
Copy link
Contributor

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,
}>();

@anthony-murphy
Copy link
Contributor Author

No. I wasn't. That's super cool, and maybe a better solution than i've come up with

@anthony-murphy
Copy link
Contributor Author
anthony-murphy commented Apr 3, 2020

@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

@DLehenbauer
Copy link
Contributor

Hmmm... that's odd. Let me give it a quick try and see...

@DLehenbauer
Copy li 8000 nk
Contributor
DLehenbauer commented Apr 3, 2020

@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.

@arinwt
Copy link
Contributor
arinwt commented Apr 3, 2020

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!

@jack-williams
Copy link
Contributor
jack-williams commented Apr 3, 2020

@DLehenbauer

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);
    }
}

@anthony-murphy
Copy link
Contributor Author
anthony-murphy commented Apr 3, 2020

@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

@jack-williams
Copy link
Contributor
jack-williams commented Apr 3, 2020

The issue with completions is that emitter.on(| wont prompt the event names, or is it that the other non-overwritten methods don't get completions? The former works fine for me in VSCode.

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.

Copy link
Contributor
@skylerjokiel skylerjokiel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍁

@anthony-murphy anthony-murphy merged commit cd5041e into microsoft:master Apr 6, 2020
@anthony-murphy anthony-murphy deleted the typed-emitter branch April 6, 2020 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0