Update discord.js to latest version
#59
Conversation
| } | ||
|
|
||
| cliReport().catch((e) => { | ||
| const message = getAnalyticsErrorMessage(e); |
There was a problem hiding this comment.
We should let the logger get the error object rather than message.
It has built in formatters to handle errors itself. No need to do it ourselves.
| } | ||
|
|
||
| export async function handleAnalyticsCommand( | ||
| discordClient: Discord.Client, |
There was a problem hiding this comment.
We receive the Discord.Client object inside of the message object.
No need to pass it by ourselves.
Moreover, message.client'sDiscord.Client is sure to be ready (Discord.Client<true>).
This ensures some types don't have the optional flag anymore.
| await handleAnalyticsCommand(discordClient, message); | ||
| } | ||
| async function handleGuildMessage(message: Discord.Message): Promise<void> { | ||
| if (message.author.bot) { |
There was a problem hiding this comment.
Don't handle any bot. That includes this bot itself.
src/discord/bot/analytics/common.ts
Outdated
| const embed = new Discord.EmbedBuilder(); | ||
| embed.setImage(report.imageChartsChart.toURL()); | ||
| options.embed = embed; | ||
| options.embeds = [embed]; |
There was a problem hiding this comment.
discord.js now supports up to 10 embeds, hence plural.
| await reportsChannel.send( | ||
| `Failed to send daily analytics report: ${message}`, | ||
| `Failed to send daily analytics report. Check the logs for more details. | ||
| https://fly-metrics.net/d/fly-logs/fly-logs?orgId=273532&var-app=wasp-bot`, |
There was a problem hiding this comment.
The message was never really to helpful.
I've added a link to our wasp-bot graphana so we can easily check the logs.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Hm still feels a bit weird that we get an error and don't show it :D.
What if it will be valuable at some point? Let's put it this way also: somebody will get this message, and will wonder "but what is the error", and might go here and return code to the old state. While if they do see the error, maybe it is useless, but at least they know they got all the info they could get, and can move on. Plus, what if it is actually useful?
So I would suggest printing error, it can't hurt can it? Why did you even remove it, because we don't have that helper function anymore so it is a bit harder to get the message from the error?
There was a problem hiding this comment.
I just thought that linking logs dashboard directly would be more helpful than something like:
Failed to send daily analytics report: Request failed with status code 500
Failed to send daily analytics report: Request failed with status code 429
# actaully useful one
Failed to send daily analytics report: Not all events have been fetched: PostHog likely rate-limited us.There was a problem hiding this comment.
I can do both, log the message + link to logs.
There was a problem hiding this comment.
Also the weird fact that last few times it failed it displayed nothing. Hmm.
If it failed inside of "total" report it should have been sent.
Maybe I should explore this more.
There was a problem hiding this comment.
Yeah maybe there is some deeper issue (well almost certainly hm).
There was a problem hiding this comment.
I've added the error message logging back. If it's an Error type it should print the error.message by default when appending the error to a string.
There was a problem hiding this comment.
@FranjoMindek nice work! I approved, but left some comments, please take care of those and I am all good.
| await reportsChannel.send( | ||
| `Failed to send daily analytics report: ${message}`, | ||
| `Failed to send daily analytics report. Check the logs for more details. | ||
| https://fly-metrics.net/d/fly-logs/fly-logs?orgId=273532&var-app=wasp-bot`, |
There was a problem hiding this comment.
Hm still feels a bit weird that we get an error and don't show it :D.
What if it will be valuable at some point? Let's put it this way also: somebody will get this message, and will wonder "but what is the error", and might go here and return code to the old state. While if they do see the error, maybe it is useless, but at least they know they got all the info they could get, and can move on. Plus, what if it is actually useful?
So I would suggest printing error, it can't hurt can it? Why did you even remove it, because we don't have that helper function anymore so it is a bit harder to get the message from the error?
src/discord/bot/index.ts
Outdated
| // We also need to enable Privileged Gateway Intents in the Discord Developer Portal. | ||
| // As of now those are: `Server Members Intent` and `Message Content Intent` | ||
| // See: https://discordjs.guide/popular-topics/intents.html#gateway-intents |
There was a problem hiding this comment.
Sounds like a TODO: "We also need to ...".
Maybe instead turn it into:
// NOTE:
// We assume that Privileged Gateway Intents were manually enabled in the Discord Developer Portal.
Because it is documentation of an assumption required for this code to work.
Also make sure to mention this in the README.md .
There was a problem hiding this comment.
But it has NOTE in front of it 😭 .
How about
// NOTE:
// If Privileged Gateway Intents are not enable in the Discord Developer Portal,
// the bot will throw an error on startup.
// As of now those are: `Server Members Intent` and `Message Content Intent`
// See: https://discordjs.guide/popular-topics/intents.html#gateway-intentsThere was a problem hiding this comment.
Ok yeah, taht sounds good also! enableD (typo).
| } | ||
|
|
||
| discordClient.on("messageUpdate", async (_oldMessage, newMessage) => { | ||
| // TODO: Actually handle partial messages. |
There was a problem hiding this comment.
So this is handled now? We enabled them in Discord.Client? Is that manually done or here in the code?
There was a problem hiding this comment.
Discord no longer sends partial messages to message update event. So we don't have to handle them separately.
There was a problem hiding this comment.
Never a big fan of types.ts/hs files. In general we avoid them. But I don't know here waht is a better name, maybe you know.
raw
There was a problem hiding this comment.
Sometimes some types in TS are global helpers. Hard to domain them when their purpose is to work with TS type system and not with worldly domain.
There was a problem hiding this comment.
But these are domain aren't they? Discord?
There was a problem hiding this comment.
It is used in that domain but the type itself is a general TS type helper. That's what I meant.
Updates
discord.jsfrom v12 to v14.This also updates Discord API from v8 to v10.
Discord API v8 is deprecated.
Logic behind changes is explained on the changed files themselves.
Fixes: #21