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 supported events (TypeScript) #159

Merged
merged 4 commits into from
Apr 24, 2018
Merged

Allow user to specify supported events (TypeScript) #159

merged 4 commits into from
Apr 24, 2018

Conversation

ZLima12
Copy link
Contributor

@ZLima12 ZLima12 commented Apr 2, 2018

This would allow users to verify that they only use events that they intend to.

e.g.

type AnyEvent = "ready" | "error" | "message";
class Foo extends EventEmitter<AnyEvent> { ... }
let instance = new Foo();
instance.on("ready", () => doSomething()); // Works fine
instance.on("bar", () => doSomething()); // Error

@lpinca
Copy link
Member

lpinca commented Apr 3, 2018

cc: @delta62

@delta62
Copy link
Contributor

delta62 commented Apr 4, 2018

Hey, @ZLima12. This is a great idea! I'm curious though - why did you decide to change the types in the functions to (string | symbol) & EventTypes instead of just EventTypes?

@ZLima12
Copy link
Contributor Author

ZLima12 commented Apr 4, 2018

@delta62 As far as I know, the library itself only supports strings and symbols for events. If we only use EventTypes, it will allow the user to specify events that aren't strings or symbols. However, the type (string | symbol) & EventTypes will match any type that is both a string or symbol (to maintain compatibility with the library), and one of the types that the user specified.

@delta62
Copy link
Contributor

delta62 commented Apr 5, 2018

Ok, that makes sense. I'd like to play around with something like

declare class EventEmitter<EventTypes extends string | symbol>

and see if that works. I'll try to do that today. If we can't come up with a way to do that, I think that your approach is good.

@ZLima12
Copy link
Contributor Author

ZLima12 commented Apr 9, 2018

@delta62 how does that look?

@delta62
Copy link
Contributor

delta62 commented Apr 13, 2018

Whoops, I missed your reply. Sorry!

I'll use this with my test project and add some examples to make sure we're not breaking anything, but I think this will work out nicely! 👍

@ZLima12
Copy link
Contributor Author

ZLima12 commented Apr 24, 2018

Sorry again about being oblivious to @delta62 's thread on my fork, but I believe this should be ready to merge now.

Copy link
Contributor

@delta62 delta62 left a comment

Choose a reason for hiding this comment

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

:shipit:

@lpinca lpinca merged commit 7eb42a6 into primus:master Apr 24, 2018
@lpinca
Copy link
Member

lpinca commented Apr 24, 2018

Thank you!

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

3 participants