fix(GuildMember): cache based on partials options #10785
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
| @@ -31,7 +31,7 @@ class GuildMemberManager extends CachedManager { | |||
|
|
|||
| /** | |||
| * The cache of this Manager | |||
| * @type {Collection<Snowflake, GuildMember>} | |||
| * @type {Collection<Snowflake, GuildMember | PartialGuildMember>} | |||
There was a problem hiding this comment.
The whole idea of fixing this was that there won‘t be PartialGuildMember in the cache. And there is no PartialGuildMember in JS land anyway, so this would only make sense in the typings (but is wrong there for the above reason).
Same applies for all other JSDoc comments you added PartialGuildMember to
| deaf: false, | ||
| mute: false, | ||
| }, | ||
| client.options.partials.includes(Partials.GuildMember), |
There was a problem hiding this comment.
This needs more/better logic. The member should always be patched with that data, if cached before, but should never be newly added to cache. The guildMemberAvaliable event here should only emit if the partial is set. So add the partial check to the if conditions surrounding this block instead.
There was a problem hiding this comment.
The
guildMemberAvaliableevent here should only emit if the partial is set.
I think it also needs to be emit if member object cached before. Didn't?
There was a problem hiding this comment.
No, if it's cache before then the !member condition would be false anyway.
| @@ -63,7 +63,7 @@ class Presence extends Base { | |||
|
|
|||
| /** | |||
| * The member of this presence | |||
| * @type {?GuildMember} | |||
| * @type {?(GuildMember | PartialGuildMember)} | |||
There was a problem hiding this comment.
Here this.member should be turned into a property created from data passed in constructor instead of a getter. That way it could contain a partial guild member without putting that partial data into cache.
| private constructor(guild: Guild, iterable?: Iterable<RawGuildMemberData>); | ||
| public guild: Guild; | ||
| public get me(): GuildMember | null; | ||
| public get me(): GuildMember | PartialGuildMember | null; |
There was a problem hiding this comment.
We always get the full GuildMember data for the ClientUser on GUILD_CREATE, so this can never be a PartialGuildMember to begin with.
There was a problem hiding this comment.
GUILD_CREATE not emit if Guilds intent not set
There was a problem hiding this comment.
Without Guilds intent you wouldn't have any GuildMembers cached and the whole GuildMemberManager would be inaccessible because you don't have a Guild having it as property to begin with.
There was a problem hiding this comment.
What prevents us from using fetch to get and cache guilds with specific members exclude client member?
There was a problem hiding this comment.
discord.js/packages/discord.js/src/managers/GuildMemberManager.js
Lines 138 to 145 in a30a749
And if it like you say, why exist this functionality?
There was a problem hiding this comment.
Okay, that's a good catch indeed. Guess GuildMemberManager#me can be a PartialGuildMember after all, if the GuildMemberManager cache is limited to 0 without exception for the ClientUser.
But about no Guilds intent: most of the typings rely on that being present. Because several helper methods need it to work anyway.
| public get hoist(): Role | null; | ||
| public get icon(): Role | null; | ||
| public get color(): Role | null; | ||
| public get highest(): Role; | ||
| public get premiumSubscriberRole(): Role | null; | ||
| public get botRole(): Role | null; | ||
| public member: GuildMember; | ||
| public member: GuildMember | PartialGuildMember; |
There was a problem hiding this comment.
While technically correct this degrades UX. Probably better to add a Partialized version of this manager overriding this property and overriding the roles getter on PartialGuildMember with that partial manager.
| | Guild | ||
| | NonThreadGuildBasedChannel | ||
| | GuildMember | ||
| | PartialGuildMember |
There was a problem hiding this comment.
I don't think it makes much sense to resolve a Guild from a partial member, but that one's up to debate I'd say
Please describe the changes this PR makes and why it should be merged:
This PR based on previous PR's conversation.
Discord API's Guild Member object has isn't nullable
joinedAtproperty.This also confirms the partial property set to false, which depends on the nullability of
joinedAtand the use ofPartializefor this property to create thePartialGuildMemberinterface.It's also incorrect that caching of a partial member is performed despite the provided partials options.
Status and versioning classification: