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

Allow user to specify argument types for events #181

Merged
merged 1 commit into from
Jun 19, 2019

Conversation

grind086
Copy link
Contributor

This is purely an extension to the current typings, and should be backwards compatible for code, but requires the TypeScript 3.0+ compiler. In addition to allowing you to specify event names (as with the current typings):

const ee = new EventEmitter<'foo' | 'bar'>();

you can alternatively pass an interface that maps event names to a tuple of argument types:

interface MyEvents {
    foo: [string];
    bar: [number, number];
}

const ee = new EventEmitter<MyEvents>();

ee.emit('foo', 'hello');
ee.emit('bar', 1, 2);

This provides strongly typed arguments for on, off, emit, etc.

@lpinca
Copy link
Member

lpinca commented Oct 27, 2018

cc: @delta62

@delta62
Copy link
Contributor

delta62 commented Oct 30, 2018

Wow, this is really neat! I played around with it and it seems like everything is working great.

I think it'd be good to add some documentation about how to use this, because the typings are getting pretty complex. It took me a while to parse through them and see what's going on here. Could we update the README with some simple notes about TS usage?

Also I do think that this should be a major version bump for the package, because we'll break builds for anyone who happens to be using TS < 3, like @grind086 mentioned.

@dave-irvine
Copy link

Ping @delta62 @lpinca is the blocker here lack of a README section? I can probably add something, its basically the content of @grind086 's original comment:

interface MyEvents {
    foo: [string];
    bar: [number, number];
}

const ee = new EventEmitter<MyEvents>();

ee.emit('foo', 'hello');
ee.emit('bar', 1, 2);

@lpinca
Copy link
Member

lpinca commented May 28, 2019

@dave-irvine I'm not very happy with a new major version only for TypeScript type definitions.

@3rd-Eden
Copy link
Member

Is there a way we can see TypeScript < 3 usage numbers? I would assume that it's pretty low as the current version is 3.4

@andsouto
Copy link

I was also missing this feature. I have been testing this a bit and it seems to work as expected. I think it would be a nice addition.

@ndelangen
Copy link

This would be great to have!

@lpinca
Copy link
Member

lpinca commented Jun 15, 2019

As @3rd-Eden wrote above.
How breaking is the change? I mean what are chances of breaking builds due to people using TS < 3?

@delta62
Copy link
Contributor

delta62 commented Jun 17, 2019

Hmm, I'm not aware of any way to query for npm downloads in a given range, e.g. how many people are still downloading typescript@<3.0.0.

It'd be really great to get this feature integrated, but I'm not sure what the best path forward is given the nature of the changes 🤔

I did just verify that if I'm using TS 2.x and try to build against these typings the compiler fails (here is the source for the test).

node_modules/eventemitter3/index.d.ts(1,84): error TS1005: ';' expected.
node_modules/eventemitter3/index.d.ts(1,120): error TS1005: ';' expected.
node_modules/eventemitter3/index.d.ts(2,108): error TS1005: ';' expected.
node_modules/eventemitter3/index.d.ts(2,144): error TS1005: ';' expected.
node_modules/eventemitter3/index.d.ts(2,158): error TS1005: ';' expected.

The latest version of TypeScript 2 appears to be 2.9.2, which was released just over one year ago - June 13, 2018.

@ndelangen
Copy link

@delta62 no, AFAIK there's no way to ask npm for statistics on installs per version. A big shame.

Sounds to me like this would warrant the release of a new major version?

@lpinca
Copy link
Member

lpinca commented Jun 19, 2019

Ok this waited long enough, if there is no other way so it be.

@lpinca lpinca merged commit e79e6a8 into primus:master Jun 19, 2019
@lpinca
Copy link
Member

lpinca commented Jun 19, 2019

Could we update the README with some simple notes about TS usage?

Please open a PR. Thank you.

@oleg-codaio
Copy link

This is great, but I'm having trouble getting these types to work with generic event emitters. Consider this example code:

interface BaseEvents {
  foo: [string, number];
}

class BaseObject extends EventEmitter<BaseEvents> {
  doSomething() {
    this.emit('foo', 'test', 123);
  }
}

This compiles fine. Now, when attempting to create a parameterizable object that can omit a number of additional events:

class GenericBaseObject<T extends {[K in keyof T]: any[]}> extends EventEmitter<BaseEvents & T> {
  doSomething() {
    this.emit('foo', 'test', 123);
    //        ~~~~~
    // Argument of type '"foo"' is not assignable to parameter of type 'EventNames<BaseEvents & T>'.
  }
}

Note that extending GenericBaseObject works, however:

interface BazEvents {
  baz: [number];
}

class BazObject extends GenericBaseObject<BazEvents> {
  doSomethingElse() {
    this.emit('foo', 'test', 123);
    this.emit('baz', 123);
  }
}

The only workaround I found was to do a cast within the base object:

class GenericBaseObject<T extends {[K in keyof T]: any[]}> extends EventEmitter<BaseEvents & T> {
  private get _self() {
    return (this as unknown) as GenericBaseObject<{}>;
  }

  doSomething() {
    this._self.emit('foo', 'test', 123);
  }

So, specifically, the issue is that you can't neatly emit events at a "non-terminal" class without doing a cast. Anyone have a better solution, or is this a TypeScript bug?

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.

None yet

8 participants