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

fix(ApplicationCommand): remove false negatives in equals #10596

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

Conversation

ImRodry
Copy link
Contributor

@ImRodry ImRodry commented Nov 16, 2024

Please describe the changes this PR makes and why it should be merged:
This fixes a bug where having a local command that doesn't declare integrationTypes explicitly would cause it to not be equal to the command returned by Discord as they send a default value of [ 0 ]. In order to fix this, it defaults the value to this.client.application.integrationTypesConfig?.keys() or [ApplicationIntegrationType.GuildInstall] if that's null

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 fixes a bug where having a local command that doesn't declare integrationTypes explicitly would cause it to not be equal to the command returned by Discord as they send a default value of [ 0 ]
@ImRodry ImRodry requested a review from a team as a code owner November 16, 2024 01:22
Copy link

vercel bot commented Nov 16, 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 Dec 7, 2024 11:04pm
discord-js-guide ⬜️ Ignored (Inspect) Visit Preview Dec 7, 2024 11:04pm

@ImRodry ImRodry changed the title Fix/applicationcommand equals false positive fix(ApplicationCommand): remove false positives in equals Nov 16, 2024
@ImRodry ImRodry changed the title fix(ApplicationCommand): remove false positives in equals fix(ApplicationCommand): remove false negatives in equals Nov 16, 2024
@ImRodry
Copy link
Contributor Author

ImRodry commented Nov 16, 2024

Since I've seen semver:major PRs starting to be merged I'm not sure if this could fit into a v14.16.4 or a v14.17.0 but it would be nice if it could!

@Jiralite
Copy link
Member

We are currently backporting pull requests if possible to v14.

Copy link
Contributor

@imnaiyar imnaiyar left a comment

Choose a reason for hiding this comment

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

According to documentation, 0 is not default.

Defaults to your app's configured contexts

According to this discord/discord-api-docs#7108 (comment) it's 0 for only older commands

Upon testing, it depends on installation contexts set by users on dev portal when using bulk overwrite route, so it's not just one thing that's returned by the api.

In any case, this is not the correct approach. I'm not sure what would be an appropriate solution, but imo, it should be left as is

@ImRodry
Copy link
Contributor Author

ImRodry commented Nov 16, 2024

It should absolutely not be left as it is, as a local command should always be equal to the version sent by Discord, and this method is often used to check for changes locally and update them with Discord accordingly. I believe it is safe to assume that apps made with discord.js will default to guild installs (I'm not sure if a user install can even be made with discord.js) but I'd be happy to change the approach if the maintainers think otherwise

@ImRodry
Copy link
Contributor Author

ImRodry commented Nov 16, 2024

Also upon further investigation, my bot has no integrationTypesConfig (it's set to null), so I think it would be safe to assume that the default is [0] in that scenario, though we could also make the default be client.application.integrationTypesConfig?.keys() ?? [0] if I'm reading the documentation correctly

@imnaiyar
Copy link
Contributor

D.js doesn't decides default value, it's discord. And it varies depending on situation

  • for older commands, it default to [0]
  • for commands created via POST method, it defaults to [0]
  • for commands created via Bulk Overwrite route (PUT), it depends on the installation contexts.

So if a person has both for installation contexts, the default value will be [0, 1] if they created the command via PUT route. And discord.js can't simply know that. So there's a chance of false negatives in any solution, so even if we use integrationTypesConfig to determine the default, it'll will be false for first two points and vice versa.

@imnaiyar
Copy link
Contributor

Also upon further investigation, my bot has no integrationTypesConfig (it's set to null), so I think it would be safe to assume that the default is [0] in that scenario

Also you are thinking only for yourself, not everyone applies to your situation

@vladfrangu
Copy link
Member

Hmm, this definitely highlights an issue (and an oddly inconsistent behavior in Discord's own API, re: POST/PUT) that can't be fixed just by slapping in a 0 in the if-not-nullish array. I guess since we have the application we can also try defaulting to the app config? 🤷

@ImRodry
Copy link
Contributor Author

ImRodry commented Nov 16, 2024

Yeah that was my suggestion, but what should we do when that config is also null like the case I described above?

@ImRodry
Copy link
Contributor Author

ImRodry commented Nov 17, 2024

Also upon further investigation, my bot has no integrationTypesConfig (it's set to null), so I think it would be safe to assume that the default is [0] in that scenario, though we could also make the default be client.application.integrationTypesConfig?.keys() ?? [0] if I'm reading the documentation correctly

Applied this solution, let me know if this is better

@ImRodry ImRodry requested a review from vladfrangu November 17, 2024 02:12
@ImRodry ImRodry requested a review from almeidx December 7, 2024 23:04
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.

5 participants