fix(ApplicationCommands): allow managing commands for uncached guilds #5729
Conversation
| /** | ||
| * Fetches the permissions for one or multiple commands. | ||
| * <warn>When calling this on ApplicationCommandManager, guildID is required. | ||
| * To fetch all permissions for an uncached guild use `fetchPermissions(undefined, '123456789012345678')`</warn> |
There was a problem hiding this comment.
I just wanted to make a note here about passing undefined. I don't love this pattern, but as Monbrey said in the discord, I don't love having to pass { command: '123456789012345678' } either.
There was a problem hiding this comment.
If the guild ID is the required one, put it first?
fetchPermissions(guild)
fetchPermissions(guild, command)There was a problem hiding this comment.
when called from a GuildApplicationCommandManager its not required, although I suppose I could just use super in that class.
There was a problem hiding this comment.
Okay, I tried to do this, and it simply doesn't work as wanted.
Putting guildID as the first parameter means implementing fetchPermissions and setPermissions in GuildApplicationCommandManager which we can do in js, but because guildID is not a parameter there, typescript really doesn't like it, suggesting this is a bad pattern in general.
There was a problem hiding this comment.
Any other suggestions for this (or a suggestion for how to make typescript happy with different method signatures 😆 )?
There was a problem hiding this comment.
override still doesn't let you remove / repurpose parameters in the signature though from what I can tell.
There was a problem hiding this comment.
It's not nice, but you could allow either just a command resolvable, or an object with it and the guild.
There was a problem hiding this comment.
There is precedent for this elsewhere in the library actulaly, so it works, will do.
There was a problem hiding this comment.
A CommandResolvable can't be accepted as the only parameter because this.resolve (when providing an id) can't resolve guild application commands, which are required when not passing guildID so that we can get command.guild.id instead. And typescript will complain if I try to make it ApplicationCommand | { guildID: Snowflake | command?: ApplciationCommandResolvable } and then override that to ApplicationCommandResolvable in the subclass
There was a problem hiding this comment.
Excuse a stranger, but if I've understood correctly the goal here is to somehow put the guildId parameter to the fetchPermissions method (possibly to more methods) and at the same time satisfy the TypeScript compiler in the GuildApplicationCommandManager, right?
So I've created a small "demo" of my approach on the TS Playground here and with this solution you can pass all combinations: no params, only command and guild id + previous combinations. I've based it on the @SpaceEEC suggestion to put either a resolvable or an object with guildId and the command.
Note: The
GuildApplicationCommandResolvabletype can be simplified to this (see below) by doing that the usage of theOptional<T>type can be avoided.
type GuildApplicationCommandResolvable = { guildId: Snowflake, command?: ApplicationCommandResolvable }(I know that I wrote it entirely in TypeScript, and it may or may not be 100% applicable but maybe at the very least it will notch you guys in the right direction, hope I've helped)
|
This needs a rebase. |
ckohen
force-pushed
the
uncached-guild-commands
branch
from
bbe9583
to
ca34e6b
Compare
|
This needs a rebase on top of the changes requested. |
Co-authored-by: SpaceEEC <spaceeec@yahoo.com>
ckohen
force-pushed
the
uncached-guild-commands
branch
from
be987fd
to
97d1070
Compare
docs: fixes typedef for GuildApplicationCommandPermissionData
Please describe the changes this PR makes and why it should be merged:
The way the library currently handles global and guild application commands is via a manager that exists on the application or guild respectively. This poses a problem for applications that have guilds authorized only with
applications.commandsas the guild is not cached and therefore does not have a guild application command manager.This PR addresses this issue by allowing the global application command manger to take
guildIDin all of its methods. It also moves the permissions methods fromGuildApplicationCommandManagertoApplicationCommandManagerfor the same reason.It also introduces a new typedef that will be used in an upcoming PR for more consistency in managers. 🚀
ApplicationCommandManagernever haveguildeven when they are guild commands. I haven't been able to come up with a super good way to document this, suggestions welcome.Status and versioning classification: