Allow event names to be numbers

[lukehorvat]

Hi 👋

I'd like to use emittery to model the packets sent between a client and server. All packets are identified by a "type", which is a number. Therefore I was hoping to use emittery in the following way:

enum ServerPacketType {
  LOGIN_FAILED = 3,
  CHAT = 17,
  SYNC_CLOCK = 30
}

interface ServerPacketEvents {
  [ServerPacketType.LOGIN_FAILED]: [reason: string];
  [ServerPacketType.CHAT]: [from: number, message: string];
  [ServerPacketType.SYNC_CLOCK]: [time: number];
}

const emitter = new Emittery<ServerPacketEvents>();
emitter.on(ServerPacketType.LOGIN_FAILED, ([reason]) => {
  console.log('Login failed!', reason);
});

This actually compiles fine, there's no type error. But at runtime an error is thrown here:

emittery/index.js

Lines 14 to 18 in 3cf4a0a

	function assertEventName(eventName) {
	if (typeof eventName !== 'string' && typeof eventName !== 'symbol') {
	throw new TypeError('eventName must be a string or a symbol');
	}
	}

Maybe I'm missing something crucial here, but I don't see any harm in supporting numbers for event names? So I prepared this PR which does just that. Let me know what you think.

Thanks.

[lukehorvat]

Also, FYI the issue linked in this comment here appears to have been shipped, so I can update this line to Record<string | symbol | number, any> if you like.

emittery/index.d.ts

Line 167 in 3cf4a0a

EventData = Record<string, any>, // When https://github.com/microsoft/TypeScript/issues/1863 ships, we can switch this to have an index signature including Symbols. If you want to use symbol keys right now, you need to pass an interface with those symbol keys explicitly listed.

[sindresorhus]

Also, FYI the issue linked in this comment here appears to have been shipped, so I can update this line to Record<string | symbol | number, any> if you like.

👍 I think TS actually has built-in object key type you could use.

[sindresorhus]

Yeah, I'm ok with adding this. But can you add a short note that it's generally preferred to use symbols than numbers?

[lukehorvat]

But can you add a short note that it's generally preferred to use symbols than numbers?

There was already a note in the readme about the benefit of using symbols. I added the word "preferred" to it but wasn't sure what else I could add. Is it what you had in mind?

👍 I think TS actually has built-in object key type you could use.

Nice! I pushed a commit replacing string | symbol | number with PropertyKey.

However now that I redefined EventData's key type as a PropertyKey, these tests fail:

emittery/index.test-d.ts

Lines 105 to 110 in 3cf4a0a

	// Userland can't emit the meta events
	{
	const ee = new Emittery();
	expectError(ee.emit(Emittery.listenerRemoved));
	expectError(ee.emit(Emittery.listenerAdded));
	}

I guess now that EventData accepts symbol keys, listenerRemoved and listenerAdded can be emitted without error. I'm not really sure how to solve this at a type level. I tried doing something like EventData = Record<Exclude<PropertyKey, typeof listenerAdded>, any>, but that doesn't work. Maybe we have to make it a runtime check?

[sindresorhus]

Maybe we have to make it a runtime check?

👍

[sindresorhus]

There was already a note in the readme about the benefit of using symbols. I added the word "preferred" to it but wasn't sure what else I could add. Is it what you had in mind?

👍

[lukehorvat]

I pushed a commit to make it a runtime check. 👍 I added a second (boolean) param to assertEventName that controls whether or not the event name can be listenerRemoved and listenerAdded. All internal calls to emit/emitSerial specify the param as true to avoid any error being thrown.

Not really sure if this is the best way though. It means emit/emitSerial now has an undocumented third param that userland could exploit if they are curious enough...

One idea I had to prevent any exploit was to change the param from a boolean to an internal (not publicly exported) symbol:

const metaEventsAllowed = Symbol('metaEventsAllowed');

function assertEventName(eventName, allowMetaEvents) {
	if (typeof eventName !== 'string' && typeof eventName !== 'symbol' && typeof eventName !== 'number') {
		throw new TypeError('`eventName` must be a string, symbol, or number');
	}

	if (isListenerSymbol(eventName) && allowMetaEvents !== metaEventsAllowed) {
		throw new TypeError('`eventName` cannot be meta event `listenerAdded` or `listenerRemoved`');
	}
}

This way userland would never be able to bypass the error. I haven't worked with symbols much so I'm not sure whether this is a good pattern or not. wdyt?

[sindresorhus]

This way userland would never be able to bypass the error. I haven't worked with symbols much so I'm not sure whether this is a good pattern or not. wdyt?

👍 That's a good use for symbols.

[lukehorvat]

Modified the runtime check to use a private symbol. 👌

I also added another commit renaming isListenerSymbol to isMetaEvent because I think it makes it a bit clearer when reading the code to understand what a "meta" event is. But I can revert if you disagree.

[sindresorhus]

Thanks for the high-quality pull request 👍

[lukehorvat]

Thanks @sindresorhus! 🎉