e-enes / invitron

The Best Way to Track Your Discord Server's Invites!
MIT License
24 stars 9 forks source link

Cannot read properties of undefnined (reading 'uses') #14

Closed VoidSector closed 7 months ago

VoidSector commented 7 months ago

I encountered an error in which the bot stops logging invites and stops posting the logs. I'm not entirely sure why, because the bug just starts eventually when the bot is running for a while. The bot is hosted on PebbleHost, that also explains why there are minecraft dirs in the error log below. But it's odd because the bot still keeps running and still responds to commands. Just the logging and tracking stops and im not sure why it would suddenly stop being able to read uses.

The bot version is 2.0 The NodeJS Version is 16.17

Now I can see that a lot of packages actually ask for NodeJS 18 and above. Especially discord collection@2.0.0 So I would assume that maybe this error is due to the NodeJS being on 16.

So is this caused by the host running NodeJS 16 by default and I need to change to 18 or is the files provided setting it specifcally to 16.17?

Error Log


Uncaught Exception
Message: Cannot read properties of undefined (reading 'uses')

Name: TypeError
Cause: undefined
Source: undefined

Stack:
TypeError: Cannot read properties of undefined (reading 'uses')
    at file:///home/minecraft/multicraft/servers/server703773/dist/events/guild/member/GuildMemberAdd.js:1:822
    at Map.find (/home/minecraft/multicraft/servers/server703773/node_modules/@discordjs/collection/dist/index.js:172:11)
    at w.execute (file:///home/minecraft/multicraft/servers/server703773/dist/events/guild/member/GuildMemberAdd.js:1:797)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
e-enes commented 7 months ago

I am unable to reproduce this error; the code has been tested with NodeJs v18, which is the recommended minimum. Could you please try upgrading to the correct version and see if the problem persists?

VoidSector commented 7 months ago

I have upgraded to NodeJS 20 since the host only allows an earlier version of 18, which does not fulfill the requirements of some packages.

So far, the error has not occurred again, although I don't quite understand why it was working with NodeJS 16 for a certain number of hours to begin with.

It appears that ensuring the NodeJS version is at least >=18.18 resolves this issue. This leads me to believe the entire error was, in fact, related to the discordjs collection 2.0.0 requiring NodeJS >18

I also want to point out there are two different entries for discord collection in the pnpm-lock.yaml

/@discordjs/collection@1.5.3: asking for NodeJS >= 16.11.0

/@discordjs/collection@2.0.0: asking for NodeJS >= 18

Considering one would download 2.0.0 and receive that .yaml file. Wouldn't that make the 1.5.3 entry redundant?

Issue fixed by NodeJS >= 18 Optimal NodeJS version for all packages is >= 18.18

e-enes commented 7 months ago

You're right about the Collection being redundant. I overlooked it because the -lock.yml file is generated automatically. I'll make the necessary changes soon, thanks.

VoidSector commented 7 months ago

Okay so the type error occurred again in regards of not being able to read the properties of "uses"

Message: Cannot read properties of undefined (reading 'uses')

Name: TypeError
Cause: undefined
Source: undefined

Stack:
TypeError: Cannot read properties of undefined (reading 'uses')
    at file:///home/minecraft/multicraft/servers/server703773/dist/events/guild/member/GuildMemberAdd.js:1:822
    at _Collection.find (/home/minecraft/multicraft/servers/server703773/node_modules/@discordjs/collection/dist/index.js:172:11)
    at w.execute (file:///home/minecraft/multicraft/servers/server703773/dist/events/guild/member/GuildMemberAdd.js:1:797)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Since this means it was unrelated to the NodeJS version.

I took a closer look at GuildMemberAdd following the StackTrace.

The 1:822 in GuildMemberAdd.js if you lead it back to before compilation points to the .get(key)!.uses! part from return currentGuildInvites.get(key)!.uses! > previousInvite.uses!;

The 1:797 in GuildMemberAdd.js points to the .find function used within the let usedInvite

I analyzed the code segment and it raised some questions.

# 1. Why are we even declaring code = key?

We are assigning key to code for what? The Invites already contain the property code to call from.

# 2. Further down we also assign member.guild.vanityURLCode to code, but why?

The invite code either is vanity or isn't. We just need to compare the vanityURLUses if the condition is met that the current code is equal to vanityCode. I don't quite understand the re-assignment of code

In case we ever would need to fetch vanityData we can do so with member.guild.fetchVanityData(); which resolves to an object of the vanity URL and the use count.

# 3. Couldn't using the find() on cachedInvites lead to issues?

Let's assume the following. A user joins the guild, but there is no data on the invite used in the cache We would be using find() on an undefined value.

currentInvites / newInvites however you wanna call it always is the most current data. It also never can be undefined, since we literally fetch it.

.getjust retrieves cached data from the Collection; hence, we use it for cached Invites. This data could potentially be outdated or stale. .fetchwould fetch data through the API by making an HTTP request to Discord, ensuring the most up-to-date data.

# Code Suggestion So wouldn't it be wiser to do .find on currentInvites and then compare to cached? Even if previousInvites somehow ends up undefined, we can just provide a fallback value of 0.

let usedInvite = currentGuildInvites.find((newInvite) => (previousInvites?.get(newInvite.code)?.uses ?? 0) < newInvite.uses!);

If previousInvites ever returns undefined or null, for that matter, we fallback to 0 because if it doesn't exist, we might also count it as 0 uses.

#

Now, I'm not exactly sure how previousInvites end up being undefined. I can only assume this is due to the bot not actually initializing its cache? My best guess on why, although confused.

Invite Check of code that isn't in the Invite Cache

This could be the case if one clears the invite cache and then a user joins. We have currentGuildInvites, which is from the fetch of the current invite cache on each user's join, but if the cache gets cleared, there is no previous one. #

4. Invite Cache never is initialized? I'm not sure where the bot actually retrieves its invite cache once a member joins. Usually, bots create a separate Collection to hold invite Data. This one doesn't?

I know that fetching invites by default has caching on. But this bot never fetches first. It only fetches on member join.

Meaning previousInvites will be the value it gets from

const previousInvites = this.client.invites.get(member.guild.id)!;

But it needs to fetch to get anything, no? This is why most bots create a Collection beforehand and then compare the invites from that Collection to the newly fetched list of invites with currentGuildInvites.

This is definitely a long one, but this error is so odd to me, especially since it never occurred in my initial testing. Especially it does not seem to occur in my small test server, but then again I'm only testing with inviting/leaving the same 2nd account. Adjusting the code of holding a separate Collection to initialize the cache (a .fetch before a member joins) and compare it to the data we get on a member join event would also imply that we update our cache accordingly when creating a new Invite.

Maybe I'm missing something here, but I would greatly appreciate any clarification you can give me!

e-enes commented 7 months ago

1. Why are we even declaring code = key?

Because the CachedInvite does not have a code property. It is impossible to find the code via the cache without using the key.

2. Further down we also assign member.guild.vanityURLCode to code, but why?

Because the vanity code is not part of the GuildInvitesManager. It must therefore be treated separately.

3. Couldn't using the find() on cachedInvites lead to issues?

It is impossible that there is no data in the clients.invites. Searching with the new or old one has no importance, because the old one is updated at each event involving the modification of the invitations. New invitations are only fetched to compare use cases, not to have new code to cache.

And if it doesn't even find the invitation, it will simply tell you in the logging. The problem doesn't come from there. Image 20-04-24 à 00 41

4. Invite Cache never is initialized?

The invitation cache is initialized every time the bot is started, or if a guild is available/unavailable, or if the bot joins/leaves a guild. The invitation cache is updated each time an invitation is created (manually or via /invites create)/deleted or if a member joins.

The 1:822 in GuildMemberAdd.js if you lead it back to before compilation points to the .get(key)!.uses! part from return currentGuildInvites.get(key)!.uses! > previousInvite.uses!;

If the problem points to .get(key)!.uses!, then it is not a problem with the cache but with the fetched ("new") invitations. This means that the retrieved invitations lack a "uses" property. You can attempt the following method, but be aware that it is a provisional solution and has not been tested:

guildMemberAdd.ts, lines 43:

const currentGuildInvites = await member.guild.invites.fetch({ cache: false });

Also make sure the bot has access to all channels and has the necessary permissions.

VoidSector commented 7 months ago

It is impossible to find the code via the cache without using the key.

I assume this is why I see other invite trackers do a temporary mapping of invites in the form of a Collection? To compare newly fetched to what is in the stored Collection.

The bot itself has all permissions and is set to administrator cause initially, I also assumed maybe it had perm issues.

I don't quite understand why it would be undefined, especially if that would be the case of newly fetched currentGuildInvites Like that can never be undefined but according to the error it somehow is?

I really do wonder if this is more of a host thing, but even then I don't get why. The inconsistency of the error just confuses me.

Even if we would do

const currentGuildInvites = await member.guild.invites.fetch({ cache: false });

We would just fetch without caching. But that should be unrelated to a possible undefined error, no?

e-enes commented 7 months ago

I assume this is why I see other invite trackers do a temporary mapping of invites in the form of a Collection? To compare newly fetched to what is in the stored Collection.

This bot operates differently; it only caches the essential components of the invitation (from the discord.js Invite class) to minimize ram usage. Since the "code" itself is used as a key, we can directly retrieve it without having to create a separate "code" property. You can see the type of CachedInvite in ./src/types/index.d.ts

Even if we would do const currentGuildInvites = await member.guild.invites.fetch({ cache: false }); We would just fetch without caching. But that should be > unrelated to a possible undefined error, no?

The "uses" of the Invite class is only retrieved at each fetch. I assumed that the "uses" of invitations in the discord.js cache has expired and we have to force it instead of looking in the cache.

e-enes commented 7 months ago

Did this change resolve the issue?

VoidSector commented 7 months ago

Okay so I observed the occurrence of the error over a longer period now. I do feel a bit dumb, kinda. A true classic user error. #

So. I had the bot in two guilds. In my testing guild and in the actual guild it's intended for. Both guilds were using the same bot account. But since we need to define a webhook url for the error handler and that is specific to a guild+channel.

Well. Inviting a test user on the testing discord ends up throwing an error through the webhook on the live guild, because it has no god damn idea who the hell supposed to have joined not to mention what kind of uses value it should be looking at. Because a user was invited on the test guild not the live guild.

And since that throws an uncaught exception and we are not preventing the crash on the case of undefined so the code can continue through try catch or promise rejection handling the bot crashes.

Which begs the question. Should the bot implement catching that error of undefined or just adjust the error log and make it part of the setup function. So when using /setup we simply define per-guild channels

Restarting the bot fixes this....obviously because well it crashed. This also explains why initially it worked again upon node update and then stopped working again because I was slightly modifying certain outputs and adjusting slash commands so moderation commands are split and not shown to regular users. Since Discord still does not allow permissions for subcommands display.

While this does make a lot of sense I think adjusting the bot to be truly a per-guild configuration is required.

I just have a few last open questions, but you can close this issue with your response to them.

#

This bot operates differently; it only caches the essential components of the invitation (from the discord.js Invite class) to minimize ram usage

What is considered essential here to minimize RAM usage? Because a Collection is created either way through client.ts

We might as well just move the Collection for Invite Cache to a dedicated Invites Handler and have all the logic clean in that file. Define an async function for caching Guild Invites, declaring variables for get and reset cache. Just an example from a different bot (just pointing it out because it's clearer)


const inviteCache = new Collection();

const getInviteCache = (guild) => inviteCache.get(guild.id);
const resetInviteCache = (guild) => inviteCache.delete(guild.id);

const getEffectiveInvites = (inviteData = {}) =>
  inviteData.tracked + inviteData.added - inviteData.fake - inviteData.left || 0;

const cacheInvite = (invite, isVanity) => ({
  code: invite.code,
  uses: invite.uses,
  maxUses: invite.maxUses,
  inviterId: isVanity ? "VANITY" : invite.inviter?.id,
});

/**
 * This function caches all invites for the provided guild
 * @param {import("discord.js").Guild} guild
 */
async function cacheGuildInvites(guild) {
  if (!guild.members.me.permissions.has("ManageGuild")) return new Collection();
  const invites = await guild.invites.fetch();

  const tempMap = new Collection();
  invites.forEach((inv) => tempMap.set(inv.code, cacheInvite(inv)));
  if (guild.vanityURLCode) {
    tempMap.set(guild.vanityURLCode, cacheInvite(await guild.fetchVanityData(), true));
  }

  inviteCache.set(guild.id, tempMap);
  return tempMap;
}

Here the full repo if you wanna check the logic solution Source: GitHub Repo #

Because the CachedInvite does not have a code property. It is impossible to find the code via the cache without using the key.

So why not add a code property to it then? Simply adding a code property to CachedInvite would make that redundant, in fact, it's essentially doing the same. We just don't store it in the CachedInvite Collection, right?

The code is inserted either way into the database, would skipping the code property in the Collection really increase RAM usage that much? It's impossible because it lacks it, yet we assign code = key on each iteration of find anyway cause we need it.

In fact CachedInvite lacks the code property but carries the optional source property for custom invites. Couldn't we just have both?

Right now, when providing a source, logs just display the source and not the code, but essentially, that created code is always linked to that provided source. If there is no source provided then the log displays code used:

If that was the intention anyway, meaning a provided source prevents code display. Looking up a code by source isn't possible, right? Having both in CachedInvite atleast would link them there too. I don't quite understand how skipping the storage of a code property actually benefits RAM usage.

#

Searching with the new or old one has no importance because the old one is updated at each event involving the modification of the invitations. New invitations are only fetched to compare use cases, not to have new code to cache.

So the cache is updated whenever an invite-related event is emitted. We fetch whenever the bot starts, guild create etc.

So using find() on previousInvites should have no issues because the cache is updated upon inviteCreate. Maybe I just got confused in the way of thinking about the check logic. We want to look through the invites and find which invite increased its uses =>

 const invite = newInvites.find(i => i.uses > oldInvites.get(i.code));

Question about possible edge case I do wonder however what would happen if one would delete an invitation after a member arrived through it. We wouldn't be able to track the invite of that member anymore, right? #

e-enes commented 7 months ago

So. I had the bot in two guilds. In my testing guild and in the actual guild it's intended for. Both guilds were using the same bot account. But since we need to define a webhook url for the error handler and that is specific to a guild+channel. Well. Inviting a test user on the testing discord ends up throwing an error through the webhook on the live guild, because it has no god damn idea who the hell supposed to have joined not to mention what kind of uses value it should be looking at. Because a user was invited on the test guild not the live guild. And since that throws an uncaught exception and we are not preventing the crash on the case of undefined so the code can continue through try catch or promise rejection handling the bot crashes. Which begs the question. Should the bot implement catching that error of undefined or just adjust the error log and make it part of the setup function. So when using /setup we simply define per-guild channels Restarting the bot fixes this....obviously because well it crashed. This also explains why initially it worked again upon node update and then stopped working again because I was slightly modifying certain outputs and adjusting slash commands so moderation commands are split and not shown to regular users. Since Discord still does not allow permissions for subcommands display. While this does make a lot of sense I think adjusting the bot to be truly a per-guild configuration is required.

The bot operates independently in each guild. The only potential issue is with the error handler; the bot needs to be present in the server where the webhook is located. If you prefer not to use the error handler, you can choose not to include the webhook URL. As long as your database (mysql/mariadb) is accessible and the Discord API is working properly, the error handler is not necessary.

Additionally, please be aware that this bot (v2.0.1) is operational in five different servers each containing between 1k and 5k members.

What is considered essential here to minimize RAM usage? Because a Collection is created either way through client.ts

A Collection must be created to track past use cases for invitations. This is necessary due to the limitations of the Discord API, which does not support tracking invitations directly. The size of the Collection can vary depending on the data you include in it.

Here the full repo if you wanna check the logic solution Source: GitHub Repo

For this bot, I adopted a "chain-style" approach. Instead of using helper functions that are only employed once, I integrate the search functionality directly where it's needed. This is similar to how I handle database data; rather than relying on preset queries which might generate unnecessary data, I tailor each query to be as personalized and relevant as possible for the specific use case. However, there are situations where helper functions are crucial, such as initializing the cache. This occurs when launching the bot, whenever the bot joins a new guild, or if a guild becomes unavailable. In these scenarios, using helpers is vital to avoid code redundancy.

So why not add a code property to it then? Simply adding a code property to CachedInvite would make that redundant, in fact, it's essentially doing the same. We just don't store it in the CachedInvite Collection, right? The code is inserted either way into the database, would skipping the code property in the Collection really increase RAM usage that much? It's impossible because it lacks it, yet we assign code = key on each iteration of find anyway cause we need it.

This approach is largely a matter of personal preference. I'm constrained by the amount of RAM available on my VPS, so I aim to reduce its usage as much as possible. By adopting this method, I've introduced an additional CPU task (value assignment), but I'm not concerned about CPU capacity, only RAM. Ultimately, what works best will vary from person to person and conducting performance tests is essential to understand the advantages and disadvantages of each approach.

However, if you notice any part of the code that could be improved or optimized to enhance the bot's performance, please feel free to fork the repository and submit a pull request.

Right now, when providing a source, logs just display the source and not the code, but essentially, that created code is always linked to that provided source.

The "source" feature serves this exact purpose. However, I plan to soon implement an option that allows you to choose whether to include the code along with the source in the configuration.

Having both in CachedInvite atleast would link them there too.

it's the same thing, they are "linked" anyway after the search operation. As said above, this adds an operation to the CPU as compensation.

So using find() on previousInvites should have no issues because the cache is updated upon inviteCreate. Maybe I just got confused in the way of thinking about the check logic. We want to look through the invites and find which invite increased its uses =>

It amounts to the same thing, whether you check it one way or another.

I do wonder however what would happen if one would delete an invitation after a member arrived through it. We wouldn't be able to track the invite of that member anymore, right?

If the bot records the invitation, it will be able to identify who invited a member even after they leave the server, regardless of whether the invitation has been deleted on discord. When a member leaves, the bot consults its own database for this information because discord does not update invitation details when someone leaves.