types(Client): EventEmitter static method overrides

[Renegade334]

Please describe the changes this PR makes and why it should be merged:

Fixes a build failure in TypeScript 5.5.
Resolves #10358.

See discussion at the associated issue.

Currently, the static on() and once() methods on Node's EventEmitter are overloaded in the discord.js typings to allow for type-checked event handling.

TypeScript 5.5.1-rc fixed the bug that allowed class merging here, and this approach is no longer possible. The EventEmitter class declaration in typings.d.ts causes breaking errors for node:events module imports in TS >=5.5.1, and needs to be removed.

It would still be useful for these methods to have access to this type-safe behaviour for client events, but we can't add static method overloads to EventEmitter itself.

Instead, we can override those methods further down the inheritance chain, and provide Client.on() and Client.once() as the type-safe methods for those who wish to use them. In other words:

// old
EventEmitter.on(client, 'messageCreate'); // AsyncIterableIterator<[message: Message<boolean>]>
Client.on(client, 'messageCreate'); // AsyncIterableIterator<[message: Message<boolean>]>
// new
EventEmitter.on(client, 'messageCreate'); // AsyncIterableIterator<any[]>
Client.on(client, 'messageCreate'); // AsyncIterableIterator<[message: Message<boolean>]>

// old
EventEmitter.once(client, 'interactionCreate'); // Promise<[interaction: Interaction<CacheType>]>
Client.once(client, 'interactionCreate'); // Promise<[interaction: Interaction<CacheType>]>
// new
EventEmitter.once(client, 'interactionCreate'); // Promise<any[]>
Client.once(client, 'interactionCreate'); // Promise<[interaction: Interaction<CacheType>]>

This won't break any existing builds that use EventEmitter.on() or EventEmitter.once(), as any resolved event argument type will now just become any. However, in order to revert back to the type-safe behaviour, they'd need to use the static methods on Client rather than on EventEmitter.

This patch also adds the optional options parameter (Node >=14) to the method overrides.

Status and versioning classification:

• Code changes have been tested against the Discord API, or there are no code changes
• I know how to update typings and have done so, or typings don't need updating

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments

Name	Status	Preview	Comments	Updated (UTC)
discord-js	⬜️ Ignored (Inspect)	Visit Preview		Aug 20, 2024 9:46am
discord-js-guide	⬜️ Ignored (Inspect)	Visit Preview		Aug 20, 2024 9:46am

[vladfrangu]

This is a breaking change, as before the point was to also support EventEmitter.on/once returning strictly typed results. Can't we use namespace merging maybe to add overloads to those functions?

[Renegade334]

This is a breaking change, as before the point was to also support EventEmitter.on/once returning strictly typed results. Can't we use namespace merging maybe to add overloads to those functions?

Namespace function declarations and class static method declarations aren't merge-compatible, so this isn't an option. (example)

It's possible to use namespace declarations to add method-like function properties to the class constructor type, but not to add overloads to static methods that already exist. The question has been asked before (microsoft/TypeScript#39870) but there's just no valid way to do it in TS as things stand. We got away with just redefining the class due to the symbol mergeability bug in the TypeScript compiler, but now that's been patched, the only way to fix the resulting build errors is to remove the EventEmitter overload definitions.

This patch isn't truly "breaking", in the sense that any usage of EventEmitter.on() or EventEmitter.once() will still work and compile just fine, just without the DJS-specific type narrowing. This is a change to existing behaviour, but it's completely unavoidable given the above limitations.

[Renegade334]

More reports of breaking are coming in as people are attempting to upgrade to TS 5.5, so I'm going to mark this as ready for review.

As a minimum changeset to unbreak TS 5.5 builds, the EventEmitter definitions need to be removed – as discussed, there simply is no way of getting around this.

As for how best to provide the type-safe DJS versions of on/once, I think that adding overloads for the inherited static methods Client.on() and Client.once() is the cleanest solution, unless anyone can come up with a better option!

[StarManTheGamer]

I think this is a good solution. Sucks that it has to be removed but I don’t see any other ways around it

[ckohen]

The more appropriate issue seems to be microsoft/TypeScript#2957 which is still open and might see some more attention now that the class merging "bug" has been "fixed".

The other thing worthy of note is DefinitelyTyped/DefinitelyTyped#68313 which added strict typing to EventEmitter natively, but neglected to do the static members. I've asked if this was intentional, and if not I assume we can fasttrack getting that in, which solves this.

I'm suggesting we block this until we get an answer on the static members getting generics. The only concern I have is the potential incompatibility between our ClientEvents interface and what EE takes as a generic, but that is likely fixable via a mapped type if it is an issue (to maintain ClientEvents to be non-breaking)

[Renegade334]

@ckohen: Generics would be ideal, but the question is whether or not it's a good idea to leave the repo in a build-breaking state until such a time as that feature gets added to DT and then makes it into a @types/node package build.

Even if only a temporising measure, having static method definitions on Client isn't future-breaking. If they're removed, they'll just go back to mirroring the inherited EventEmitter method definitions.

The TS syntax proposal is nearly a decade old, so while re-opening the discussion is a good idea, I wouldn't be holding out for it.

[kyranet]

The new code is fine, however, we want to wait for DefinitelyTyped/DefinitelyTyped#69997 first so it's correctly updated.

[jamezrin]

I don't think the DT team is looking to merge DefinitelyTyped/DefinitelyTyped#69997, at least for now. Could this PR get merged since it fixes usage with TS 5.5+?

[ckohen]

DT will not merge it as is but refuses to respond to say how it can be merged, so it's on them. They broke it in the first place and are unwilling to fix it.

Regardless, no matter how we do this it's a breaking change, so we're heavily considering fix-breaking this in a more sustainable way.

[ckohen]

This is the least breaking change (even in the case where EE has full generic support) so we're going to move forward with this for v14 at least.