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

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

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

Conversation

Renegade334
Copy link
Contributor

@Renegade334 Renegade334 commented Oct 1, 2024

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:

  1. always enforce overwrite.type as mandatory, even if overwrite.id is a member object or role object;
  2. 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)

Copy link

vercel bot commented Oct 1, 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 1, 2024 10:49pm
discord-js-guide ⬜️ Ignored (Inspect) Visit Preview Oct 1, 2024 10:49pm

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

Successfully merging this pull request may close these issues.

Type error with permissionOverwrites
3 participants