refactor(PermissionOverwrites)!: cache-independent resolve

Renegade334 commented 1 month ago

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

Resolves #10450.

The linked issue relates to the current cache-dependent behaviour of PermissionOverwrites.resolve(), specifically that passing identical parameters to .resolve() can conditionally pass or fail validation depending on the state of the cache.

This isn't ideal, and the function would be better placed resolving the target via .resolveId() in a cache-independent manner.

This approach doesn't allow for overwrite.type to be optional if overwrite.id is a Snowflake, since it would no longer attempt to seek the respective member/role object from the cache to identify it as one or the other. There are two options for how to handle this:

always enforce overwrite.type as mandatory, even if overwrite.id is a member object or role object;
keep overwrite.type as optional if overwrite.id is a member object or role object (current behaviour), but enforce it as mandatory if overwrite.id is a Snowflake.

This PR implements the latter approach. It validates overwrite.id and overwrite.type as GuildMemberResolvable|RoleResolvable and OverwriteType|undefined respectively, then:

if overwrite.id is a Snowflake, it sets the type of the result to overwrite.type, throwing an error if it's not defined;
if overwrite.id is a member/role object, it sets the type of the result to the OverwriteType corresponding to that object.
- There's then the question of what to do if overwrite.type is provided, but doesn't match the type of overwrite.id. This changeset plumps for throwing a validation error if that occurs, since silently overruling the type provided to the function without warning the user could lead to some hard-to-debug quirks.

The same cache-dependent type behaviour is also present on PermissionOverwriteManager#upsert(), and the behaviour of PermissionOverwriteManager#edit() is also seemingly unnecessarily cache-dependent. Think these should also be updated, but will wait for feedback on the PR so far.

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 includes breaking changes (methods removed or renamed, parameters moved or removed)

vercel[bot] commented 1 month ago

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

2 Skipped Deployments

Renegade334 commented 4 days ago

If this approach is kosher, then just wanted to check the following:

PermissionOverwriteManager#create() and PermissionOverwriteManager#edit() have the same cache-dependent erroring if a snowflake is passed, via the logic here: https://github.com/discordjs/discord.js/blob/e2df0e0dbc224cf8915e30b4906b832e36a6840a/packages/discord.js/src/managers/PermissionOverwriteManager.js#L96-L103 Should this behaviour also be altered to mirror this change?
PermissionOverwriteManager#edit() uses the cache to generate the existing permission bitfields, which are used as a base onto which the changes are added: https://github.com/discordjs/discord.js/blob/e2df0e0dbc224cf8915e30b4906b832e36a6840a/packages/discord.js/src/managers/PermissionOverwriteManager.js#L146-L149 If a snowflake is passed here and there is a cache miss, then the "base" bitfields will end up being empty, and any permissions that aren't explicitly modified will be silently truncated on the target. Does this also need some cache failure logic? (May or may not be out of scope.)

discordjs / discord.js

refactor(PermissionOverwrites)!: cache-independent resolve #10528