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(Message): throw error on missing channel #6581

Merged
merged 1 commit into from Sep 28, 2021

Conversation

ImRodry
Copy link
Contributor

@ImRodry ImRodry commented Sep 2, 2021

Please describe the changes this PR makes and why it should be merged:
This PR makes it so all Message methods/getters that depend on the message's channel either throw an error when the channel is missing (for methods) or account for the possibility of the channel being null (getters). This is due to the fact that Message#channel became a getter in v13 iirc and this can result in the channel being missing if the client loses access to the channel by either being kicked from the guild or the channel being deleted. Fixes #6578

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

Copy link
Contributor

@NotSugden NotSugden left a comment

Choose a reason for hiding this comment

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

the errors here need to be returned with Promise.reject(...); not throw!

on the non-async functions*

src/structures/Message.js Outdated Show resolved Hide resolved
@ImRodry
Copy link
Contributor Author

ImRodry commented Sep 2, 2021

the errors here need to be returned with Promise.reject(...); not throw!

on the non-async functions*

I totally missed this, sorry, but what difference does it make? Can't the error still be caught with .catch?

@NotSugden
Copy link
Contributor

NotSugden commented Sep 3, 2021

Can't the error still be caught with .catch?

no

Copy link
Contributor

@DTrombett DTrombett left a comment

Choose a reason for hiding this comment

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

Non async functions will throw an error not catchable with the .catch method since there's no promise.

src/structures/Message.js Outdated Show resolved Hide resolved
src/structures/Message.js Outdated Show resolved Hide resolved
src/structures/Message.js Outdated Show resolved Hide resolved
src/structures/Message.js Outdated Show resolved Hide resolved
src/structures/Message.js Outdated Show resolved Hide resolved
Copy link
Member

@vladfrangu vladfrangu left a comment

Choose a reason for hiding this comment

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

Other than all the suggested changes (non-async functions need to either be marked as async or return Promise.reject(new Error) for the errors to be .catch-able,; const { channel } = this; for all the getters (since otherwise it's exactly what @NotSugden said here #6581 (comment))), LGTM

@ImRodry
Copy link
Contributor Author

ImRodry commented Sep 3, 2021

@vladfrangu only crosspostable used references to this.channel more than once so I only added it there, let me know if others need this as well

src/structures/Message.js Outdated Show resolved Hide resolved
Copy link
Member

@Jiralite Jiralite left a comment

Choose a reason for hiding this comment

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

It's possible for this error to appear in direct messages:

const client = new (require("discord.js").Client)({
  partials: [
    "CHANNEL",
    "MESSAGE",
  ],
  intents: [
    "DIRECT_MESSAGES"
  ]
});

client.on("channelDelete", (channel) => {
  channel.messages.cache.first().edit("hii");
});

client.on("ready", async () => {
  const message = await (await client.users.fetch("id")).send("hi");
  message.channel.delete();
});

client.login("token");
Response
/Users/jiralite/Documents/GitHub/Soulobby/node_modules/discord.js/src/structures/Message.js:635
    if (!this.channel) return Promise.reject(new Error('NO_GUILD_CHANNEL'));
                                             ^

Error [NO_GUILD_CHANNEL]: Could not find the channel where this message came from! It may have been deleted.
    at Message.edit (/Users/jiralite/Documents/GitHub/Soulobby/node_modules/discord.js/src/structures/Message.js:635:46)
    at Client.<anonymous> (/Users/jiralite/Documents/GitHub/Soulobby/source/test.js:37:34)
    at Client.emit (node:events:394:28)
    at ChannelDeleteAction.handle (/Users/jiralite/Documents/GitHub/Soulobby/node_modules/discord.js/src/client/actions/ChannelDelete.js:30:14)
    at Object.module.exports [as CHANNEL_DELETE] (/Users/jiralite/Documents/GitHub/Soulobby/node_modules/discord.js/src/client/websocket/handlers/CHANNEL_DELETE.js:4:32)
    at WebSocketManager.handlePacket (/Users/jiralite/Documents/GitHub/Soulobby/node_modules/discord.js/src/client/websocket/WebSocketManager.js:345:31)
    at WebSocketShard.onPacket (/Users/jiralite/Documents/GitHub/Soulobby/node_modules/discord.js/src/client/websocket/WebSocketShard.js:443:22)
    at WebSocketShard.onMessage (/Users/jiralite/Documents/GitHub/Soulobby/node_modules/discord.js/src/client/websocket/WebSocketShard.js:300:10)
    at WebSocket.onMessage (/Users/jiralite/Documents/GitHub/Soulobby/node_modules/ws/lib/event-target.js:132:16)
    at WebSocket.emit (node:events:394:28) {
  [Symbol(code)]: 'NO_GUILD_CHANNEL'
}

In this case, the error message is actually wrong! I suggest removing the "guild" portion of the error as it can happen without guilds, in this case.

Copy link
Contributor Author

@ImRodry ImRodry left a comment

Choose a reason for hiding this comment

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

Reviewing cause I’m not home and this makes it easier to commit but @Jiralite is this good?

src/errors/Messages.js Outdated Show resolved Hide resolved
src/structures/Message.js Outdated Show resolved Hide resolved
src/structures/Message.js Outdated Show resolved Hide resolved
src/structures/Message.js Outdated Show resolved Hide resolved
src/structures/Message.js Outdated Show resolved Hide resolved
src/structures/Message.js Outdated Show resolved Hide resolved
src/structures/Message.js Outdated Show resolved Hide resolved
src/structures/Message.js Outdated Show resolved Hide resolved
src/structures/Message.js Outdated Show resolved Hide resolved
src/structures/Message.js Outdated Show resolved Hide resolved
@Jiralite
Copy link
Member

Jiralite commented Sep 5, 2021

Those changes look good to me!

Co-Authored-By: D Trombett <73136330+DTrombett@users.noreply.github.com>
@iCrawl iCrawl merged commit 60aa9ae into discordjs:main Sep 28, 2021
@ImRodry ImRodry deleted the fix-edit-no-channel branch September 28, 2021 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: editing a message without a cached channel throws a cryptic error
10 participants