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

types(Client): EventEmitter static method overrides #10360

Merged
merged 2 commits into from
Aug 20, 2024

Conversation

Renegade334
Copy link
Contributor

@Renegade334 Renegade334 commented Jun 21, 2024

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

Copy link

vercel bot commented Jun 21, 2024

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
Copy link
Member

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
Copy link
Contributor Author

Renegade334 commented Jun 21, 2024

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.

cm-ayf added a commit to discordjs-japan/om that referenced this pull request Jun 22, 2024
by with declare module "node:events" in discord.js

see: discordjs/discord.js#10360
@Renegade334
Copy link
Contributor Author

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!

@Renegade334 Renegade334 marked this pull request as ready for review June 23, 2024 14:32
@Renegade334 Renegade334 requested a review from a team as a code owner June 23, 2024 14:32
@Jiralite Jiralite added this to the discord.js 14.16 milestone Jun 23, 2024
Copy link

@StarManTheGamer StarManTheGamer left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

ckohen commented Jun 24, 2024

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
Copy link
Contributor Author

Renegade334 commented Jun 24, 2024

@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.

Copy link
Member

@kyranet kyranet left a comment

Choose a reason for hiding this comment

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

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

@jamezrin
Copy link

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
Copy link
Member

ckohen commented Aug 18, 2024

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.

nyapat pushed a commit to nyapat/discord.js that referenced this pull request Oct 4, 2024
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Conflict between discord.js + @nodes/types after upgrading
9 participants