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: revalidate cached members on GUILD_CREATE #9676

Closed
wants to merge 1 commit into from
Closed

fix: revalidate cached members on GUILD_CREATE #9676

wants to merge 1 commit into from

Conversation

WhatCats
Copy link

According to the Discord docs regarding the GUILD_CREATE event:
If your bot does not have the GUILD_PRESENCES Gateway Intent, or if the guild has over 75k members, members and presences returned in this event will only contain your bot and users in voice channels.

Please describe the changes this PR makes and why it should be merged:
This removes the line of code that clears the guild's members cache in _patch() (which is called by the GUILD_CREATE event handler) to save the cached members in the event that you do not have the GUILD_PRESENCES Gateway Intent, or if the guild has over 75k members and Discord sends a GUILD_CREATE event for whatever reason.

Status and versioning classification:

  • typings don't need updating

@WhatCats WhatCats requested a review from a team as a code owner June 30, 2023 09:50
@vercel
Copy link

vercel bot commented Jun 30, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
discord-js ⬜️ Ignored (Inspect) Jul 2, 2023 0:04am
discord-js-guide ⬜️ Ignored (Inspect) Jul 2, 2023 0:04am

@WhatCats
Copy link
Author

WhatCats commented Jun 30, 2023

If a guild goes on an outage, you won't receive updates to their presence or whether they still remain in the guild. This code change introduces potentially invalid guild members, right?

Both invalid GuildMembers and missing GuildMembers is not ideal.
It seems like the one way to avoid this issue completly is by using the GUILD_PRESENCES intent for now.

The other question is when is the GUILD_CREATE event fired?
Is it just

  • on inintial connect after Ready packet (for each Guild)
  • when bot joins Guild
  • when Guild becomes available again

Or will reconnecting for example also trigger it?
(Thus is there a point in not clearing the member cache if the guild was never marked as unavailable at least)

@Qjuh
Copy link
Contributor

Qjuh commented Jul 1, 2023

After full reconnect it will be triggered too, but then you also could have missed members leaving and presences being updated during the reconnect. So the same issue remains. The „fix“ for what you describe as problem here would be to fetch all members in guildCreate event and have a ready event that doesn’t run once but with on on every ready that does that for guilds you need it in. There’s no way for discord.js to validate the cache against what exists on discord other than replacing it completely in those cases.

@WhatCats
Copy link
Author

WhatCats commented Jul 1, 2023

The „fix“ for what you describe as problem here

Yes, a "fix" as there is normally nothing else that will re-fetch the guild members, leaving the cache in an invalid state, without any obvious signs/warnings. This can lead to very weird, random seeming and even frustrating behaviour.

Missed presence updates are also not a worry since this problem only occurs when there is no GUILD_PRESENCES intent, in which case presence updates will not be received anyway.

According to the Discord docs regarding the `GUILD_CREATE` event:
If your bot does not have the `GUILD_PRESENCES` Gateway Intent, or if the guild has over 75k members, **members and presences returned in this event will only contain your bot and users in voice channels**.
@WhatCats WhatCats changed the title fix: keep cached members on GUILD_CREATE fix: revalidate cached members on GUILD_CREATE Jul 2, 2023
@Qjuh
Copy link
Contributor

Qjuh commented Jul 2, 2023

And now you introduced a rather breaking change by fetching all members there. Which discord.js never does, it‘s upon the library user to decide whether they want to fetch all members or not. As already stated, the premise of your PR is wrong. If you need all members fetched you can do so yourself by listening to the respective event and fetching them again on a reconnect. It’s not the job of the library to do that for you.

@WhatCats
Copy link
Author

WhatCats commented Jul 2, 2023

Your right, what I did here does not make much sense.

@WhatCats
Copy link
Author

WhatCats commented Jul 2, 2023

If I am seeing this right though the GUILD_CREATE handler

let guild = client.guilds.cache.get(data.id);
  if (guild) {
    if (!guild.available && !data.unavailable) {
      // A newly available guild
      guild._patch(data);
    }
  } else {
    // A new guild
    data.shardId = shard.id;
    guild = client.guilds._add(data);
    if (client.ws.status === Status.Ready) {
      /**
       * Emitted whenever the client joins a guild.
       * @event Client#guildCreate
       * @param {Guild} guild The created guild
       */
      client.emit(Events.GuildCreate, guild);
    }
  }

will not update the guild if a full reconnect occurs and the GUILD_CREATE's are sent again, since the guild would never have been marked as unavailable?

@WhatCats
Copy link
Author

WhatCats commented Jul 2, 2023

Let's say someone did want to keep the members fetched.
I can't find an event that would tigger when a guild becomes available again (after being marked as unavailable) or for when a shard reconnected (not resumed) again successfully.

@Jiralite Jiralite removed this from the discord.js 14.12 milestone Jul 8, 2023
@WhatCats WhatCats closed this Jul 11, 2023
@WhatCats WhatCats deleted the fix/guild-patch branch July 11, 2023 01:33
@almeidx almeidx removed the blocked label Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants