Skip to content

types(ClientEventTypes): use Events as keys #11121

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

types(ClientEventTypes): use Events as keys #11121

wants to merge 1 commit into from
+95 −82

Conversation

Pavel-Boyazov
Copy link
Contributor

@Pavel-Boyazov Pavel-Boyazov commented Sep 29, 2025 •
edited
Loading

Please describe the changes this PR makes and why it should be merged:
Allow to use native Events as key for ClientEventTypes (for examle ClientEventTypes[Events] instead of ClientEventTypes[keyof ClientEventTypes])

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

@Pavel-Boyazov Pavel-Boyazov requested a review from a team as a code owner September 29, 2025 10:07
@vercel
Copy link

vercel bot commented Sep 29, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
discord-js Ignored Ignored Preview Sep 29, 2025 10:07am
discord-js-guide Ignored Ignored Preview Sep 29, 2025 10:07am

Jiralite
Jiralite previously approved these changes Sep 29, 2025
View reviewed changes
@Jiralite Jiralite dismissed their stale review September 29, 2025 10:13

Awaiting tests.

Jiralite
Jiralite requested changes Sep 29, 2025
View reviewed changes
Copy link
Member

@Jiralite Jiralite left a comment

Choose a reason for hiding this comment

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

index.test-d.ts is full of errors due to this change.

It seems this stops strings from being used. Unsure if this is desired. @vladfrangu, any ideas?

@github-project-automation github-project-automation bot moved this from Todo to Review in Progress in discord.js Sep 29, 2025
@Jiralite Jiralite added this to the discord.js 15.0.0 milestone Sep 29, 2025
@Pavel-Boyazov
Copy link
Contributor Author

Pavel-Boyazov commented Sep 29, 2025 •
edited
Loading

I agree that removing the ability to use strings directly is a bad idea. But I would also like to add compatibility of the type with the Events enum, since in fact they are completely identical.

@Pavel-Boyazov
Copy link
Contributor Author

As I see it, it comes down to the async_event_emitter's typing. I would like to understand more precisely what can and should be changed there in order to keep support for raw strings

@Pavel-Boyazov
Copy link
Contributor Author

Pavel-Boyazov commented Sep 29, 2025 •
edited
Loading

Another problem. voiceServerUpdate event not exists in ClientEventTypes

@vladfrangu
Copy link
Member

Unsure if this is desired

I wouldn't want this to break normal strings, so if it does... I am -1 on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Jiralite Jiralite Jiralite requested changes

@almeidx almeidx almeidx approved these changes

@vladfrangu vladfrangu Awaiting requested review from vladfrangu vladfrangu is a code owner automatically assigned from discordjs/core

@iCrawl iCrawl Awaiting requested review from iCrawl iCrawl is a code owner automatically assigned from discordjs/core

@kyranet kyranet Awaiting requested review from kyranet kyranet is a code owner automatically assigned from discordjs/core

@SpaceEEC SpaceEEC Awaiting requested review from SpaceEEC SpaceEEC is a code owner automatically assigned from discordjs/core

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

packages:discord.js semver:patch typings

Projects

discord.js
Status: Review in Progress

Milestone

discord.js 15.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@Pavel-Boyazov @vladfrangu @Jiralite @almeidx