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.
| 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.
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.
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.
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.
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.
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.
Added it back in b3ed332. Not sure how elegant it is tho😅
iCrawl
changed the title
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>#resolveIDinstead. 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#addMemberdue to it being related to OAuth2. Even though it should work, It would be nice if someone can test it :)Status and versioning classification: