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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow symbol event names #43

Merged
merged 6 commits into from Feb 25, 2020
Merged

Conversation

stroncium
Copy link
Contributor

@stroncium stroncium commented Sep 23, 2019

Fixes #16

In typescript, complete typing is blocked by microsoft/TypeScript#1863 at the moment, so we relax typing a bit. This relaxation affects defining EventDataMap with number indexes, so Emittery.Typed<{123: string}, 'someevent'> causes no compile time error. However, using any such event will still cause compile time error.

馃


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


IssueHunt has been backed by the following sponsors. Become a sponsor

@stroncium
Copy link
Contributor Author

@sindresorhus Lemme know if we want this temporal type relaxation or if we will wait for the typescript patch to land so I could update docs accordingly.

@sindresorhus
Copy link
Owner

sindresorhus commented Feb 2, 2020

Lemme know if we want this temporal type relaxation or if we will wait for the typescript patch to land so I could update docs accordingly.

Doesn't look like the TypeScript project will fix this anytime soon, so let's do it this way.


Don't forget to update the docs.

@sindresorhus
Copy link
Owner

@stroncium When this is merged, what do you think of dropping .onAny() and .offAny() later on? Since it can now be represented as:

emitter.on(Emittery.anyEvent, listener);

Would be nice to reduce the API surface.

@stroncium
Copy link
Contributor Author

@sindresorhus I'd vote against it.
With such change we would basically take 2 different code paths and hide them behind single name(introducing additional eq check, so less performance in theory, though I imagine it would be almost unnoticeable), while also introducing additional symbol with highly specific behavior unlike any other symbol. I'm generally against creating such artificial exceptional behavior, and prefer 2 methods(+counterparts) each doing their own thing instead of one(+counterpart) with a special case, as both API docs are clearer and code using them is easier to read.

@stroncium
Copy link
Contributor Author

@sindresorhus Docs didn't mention that what event names are supposed to be anywhere(except type in ts), but I think symbol events are really good and must be used as much as possible, so I took freedom to add a couple lines to example and add notification at the beginning of API docs in readme.md.

index.d.ts Outdated Show resolved Hide resolved
readme.md Outdated
@@ -23,17 +23,29 @@ const Emittery = require('emittery');

const emitter = new Emittery();

const myEvent = Symbol('my symbol event');
Copy link
Owner

Choose a reason for hiding this comment

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

Could use a better example. This doesn't really show how Symbols can be useful as event names.

});

emitter.emit('馃', '馃寛');
emitter.emit(myEvent, '馃')

Copy link
Owner

Choose a reason for hiding this comment

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

Can you add this example to the TS file too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have generic usage example in typescript, only examples to clarify some non-obvious things.

@sindresorhus
Copy link
Owner

@Richienb Can you help review?

index.d.ts Outdated Show resolved Hide resolved
@Richienb
Copy link
Contributor

@stroncium Ping!

@sindresorhus sindresorhus merged commit 8c6bd97 into sindresorhus:master Feb 25, 2020
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.

Support Symbol as eventName
3 participants