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

feat(Client): add typed wrapper methods for EventEmitter.{on,once} #10581

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Renegade334
Copy link
Contributor

@Renegade334 Renegade334 commented Oct 24, 2024

So...

Historically, the discord.js typings have overloaded EventEmitter.on() and EventEmitter.once() with signatures that return typed tuples from ClientEvents, if the target is a client object and the event is in ClientEvents. This allowed users to, for example, do things like const [ role ] = await EventEmitter.once(client, 'roleCreate') and have the yielded values be automatically typed without needing any manual coercion.

These overloads had to be removed, due to TypeScript patching out the bug that was being exploited to add them to EventEmitter itself. They were moved downstream to Client.{on,once} in #10360 as a temporising measure, but the overload signatures themselves remained the same.

Unfortunately, EventEmitter.on() returns an async iterator.

Iterators are in a state of heavy flux within the TS library at the moment, and hence also within @types/node, which recently changed the types of its returned iterators across the entire library. These were formerly [Async]IterableIterators, but are now internal interfaces which extend TypeScript's [Async]IteratorObject in TS 5.6+.

Since those interfaces change shape depending on which TypeScript libs are loaded, this has the potential to cause subtle assignability errors with the Client overloads (eg. #10577) that are conditional on both TS and @types/node versions and also on which libs are loaded, and are therefore not picked up within the library's build tests. Even if the Client overloads are fixed now, they're going to be vulnerable to further breakage in the future if those interfaces continue to evolve.

However, there is a different approach entirely: declare new client instance methods, which wrap the EventEmitter static methods.

Instead of playing with EventEmitter definitions, wrapper methods are declared on the BaseClient prototype (naming up for discussion):

  • .eventIterator(eventName[, options]) as a wrapper for EventEmitter.on(client, eventName[, options])
  • .awaitEvent(eventName[, options]) as a wrapper for EventEmitter.once(client, eventName[, options])

This has a number of advantages:

  • Offers users a stable API, any changes to which can respect discord.js semantic versioning.
  • Offers a simpler call signature: client.awaitEvent(event) rather than EventEmitter.once(client, event).
  • Makes the type definitions independent of users' TypeScript and @types/node versions (since they only depend on Promise and AsyncIterableIterator).
  • Obviates the need to mess with the @types/node library definitions altogether, which removes the potential for further breakage in the future.

Removing the client-specific Client.{on,once} overload definitions from the .d.ts won't break any existing builds, since the only potential change from reverting to the native @types/node signatures is to widen the types of the yielded/resolved arrays (from typed tuples to any[]).

Resolves: #10577

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
  • This PR changes the library's interface (methods or parameters added)

@Renegade334 Renegade334 requested a review from a team as a code owner October 24, 2024 23:51
Copy link

vercel bot commented Oct 24, 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 Oct 25, 2024 0:06am
discord-js-guide ⬜️ Ignored (Inspect) Visit Preview Oct 25, 2024 0:06am

Remove broken Node.js EventEmitter overloads
@Renegade334 Renegade334 force-pushed the baseclient-eventemitter-wrappers branch from bc11ef8 to 383495b Compare October 25, 2024 00:06
@Renegade334 Renegade334 marked this pull request as draft November 5, 2024 01:31
@almeidx
Copy link
Member

almeidx commented Dec 5, 2024

Instead of adding yet another workaround, why can't users just use this instead?

const [role] = await Client.once(client, 'roleCreate');

ts playground

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
3 participants