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: Use ThreadChannel and AnyThreadChannel consistently #10181

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

RedGuy12
Copy link
Contributor

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

I've been having issues with some methods returning ThreadChannel and some returning AnyThreadChannel. This PR makes how we use each consistent.

  • If a more specific type can be determined (ie, threads in ThreadOnlyChannel will always be PublicThreadChannel<true>), that is used.
  • Property values, return values, and anything else used as a value in userland are the more specific AnyThreadChannel type.
  • If it's something the user passes in to Discord.js, or if it is used as a type guard, it uses the broader ThreadChannel type. Typically, the user shouldn't have to use ThreadChannel in their codebase at all (since all userland-facing values have been changed to AnyThreadChannel), but it doesn't hurt to allow it here.

Status and versioning classification:

  • I know how to update typings and have done so, or typings don't need updating
  • This PR only includes non-code changes, like changes to documentation, README, etc.

@RedGuy12 RedGuy12 requested a review from a team as a code owner March 18, 2024 14:47
Copy link

vercel bot commented Mar 18, 2024

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

Name Status Preview Comments Updated (UTC)
discord-js ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 30, 2024 7:35pm
discord-js-guide ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 30, 2024 7:35pm

@RedGuy12 RedGuy12 changed the title types: Use ThreadChannel and AnyThreadChannel consistently types: Use ThreadChannel and AnyThreadChannel consistently Mar 18, 2024
Copy link
Member

@vladfrangu vladfrangu left a comment

Choose a reason for hiding this comment

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

Also don't forget to lint your codes

packages/discord.js/typings/index.d.ts Show resolved Hide resolved
packages/discord.js/typings/index.d.ts Outdated Show resolved Hide resolved
packages/discord.js/typings/index.d.ts Outdated Show resolved Hide resolved
packages/discord.js/typings/index.d.ts Show resolved Hide resolved
Signed-off-by: RedGuy12 <61329810+RedGuy12@users.noreply.github.com>
packages/discord.js/typings/index.test-d.ts Outdated Show resolved Hide resolved
packages/discord.js/typings/index.d.ts Outdated Show resolved Hide resolved
Signed-off-by: cobalt <61329810+RedGuy12@users.noreply.github.com>
Signed-off-by: cobalt <61329810+RedGuy12@users.noreply.github.com>
Signed-off-by: cobalt <61329810+RedGuy12@users.noreply.github.com>
@RedGuy12 RedGuy12 requested a review from Qjuh March 27, 2024 20:21
@Jiralite Jiralite added this to the discord.js 14.15 milestone Apr 2, 2024
MessageManager: [manager: typeof MessageManager, holds: typeof Message];
// TODO: PermissionOverwriteManager: [manager: typeof PermissionOverwriteManager, holds: typeof PermissionOverwrites];
PresenceManager: [manager: typeof PresenceManager, holds: typeof Presence];
ReactionManager: [manager: typeof ReactionManager, holds: typeof MessageReaction];
ReactionUserManager: [manager: typeof ReactionUserManager, holds: typeof User];
// TODO: RoleManager: [manager: typeof RoleManager, holds: typeof Role];
StageInstanceManager: [manager: typeof StageInstanceManager, holds: typeof StageInstance];
ThreadManager: [manager: typeof ThreadManager, holds: typeof ThreadChannel];
ThreadManager: [manager: typeof ThreadManager, holds: AnyThreadChannel];
Copy link
Member

Choose a reason for hiding this comment

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

My hunch was correct: this creates a discrepancy between what the makeCache method gets (manager, class, manager) and what the types imply when a threadmanager is brought into the mix (which implies the class is an instance). This still needs a typeof in some form

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Review in Progress
Development

Successfully merging this pull request may close these issues.

None yet

5 participants