-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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(Resolvables): valid resolvables throw error when uncached #5495
Conversation
src/structures/GuildMember.js
Outdated
@@ -283,11 +283,11 @@ class GuildMember extends Base { | |||
*/ | |||
async edit(data, reason) { | |||
if (data.channel) { | |||
data.channel = this.guild.channels.resolve(data.channel); | |||
if (!data.channel || data.channel.type !== 'voice') { | |||
let voiceChannelID = this.guild.channels.resolveID(data.channel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let voiceChannelID = this.guild.channels.resolveID(data.channel); | |
const voiceChannelID = this.guild.channels.resolveID(data.channel); |
should there also be some check if the channel is voice-based if its cached ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could but these are some points that I had in my mind related to this:
- The check will need another
resolve
, that might slow down the method a little bit. - Right now if the channel passed isn't a VC (cached or uncached), it will throw the same error that is returned by the API. While after adding the check, the errors will be slightly different depending on the channel passed was cached (lib error) or uncached (API error), which seems a little inconsistent from a user's perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldnt nessercerily require another resolve
something like
const voiceChannel = this.guild.channels.cache.get(voiceChannelID);
if (voiceChannel && voiceChannel.type !== 'voice') { ... }
would be fine imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second point still holds true tho. I feel like letting the API do the work here seems more consistent and also saves us from making that get
call (which I know isn't that big of a deal but still 😅).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aight, It's 2-1 now, so I added your suggestion. Check it out and see if there is anything that I forgot :)👍
Co-authored-by: Sugden <28943913+NotSugden@users.noreply.github.com>
per review and discordjs#5495
Since #5314 is changing how most of these error drastically I changed those there since vaporox suggested it there as well. |
src/structures/GuildMember.js
Outdated
const voiceChannelID = this.guild.channels.resolveID(data.channel); | ||
if (!voiceChannelID) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would probably be nice to keep the type check for cached channels.
Not sure if there is some elegant solution for that though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added it back in b3ed332. Not sure how elegant it is tho😅
per review and discordjs#5495
Please describe the changes this PR makes and why it should be merged:
answers/fixes: #5242
This PR is an extension of #5489 (unlike that PR. this one doesn't add any new parameter) and aims to fix errors that arise due IDs that belong to structures which aren't cached. This happens because the library tries to check for the validity of the resolvable by using
<Manager>#resolve
, this doesn't work in case of the resolvable being uncached. All of this can be fixed by using<Manager>#resolveID
instead. This way we let the API do the validity test. I've tested most of the changes and the errors returned by API do a good job in letting the user know what's wrong.Guild#addMember
due to it being related to OAuth2. Even though it should work, It would be nice if someone can test it :)Status and versioning classification: