-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(Interactions): member objects for uncached guilds #10195
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Ignored Deployments
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a LOT of inconsistencies between trying to make this a non-semver major change (by also having snake_case properties) and a semver-major change (by not exposing all values under the same name). Let's pick one side (and gut says you want this in a sem-minor 👀) and roll with it!
nick
isn't exposed asnick
, onlynickname
data.roles
isn't exposed at all, only asroleIds
- this one I understand why, but I'd at least try to make uncachedMember.roles be our role manager (it should work)
|
||
/** | ||
* The guild that this member is part of | ||
* @type {Guild} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a string, not a Guild. You can add in a guild getter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops will fix
*/ | ||
this.guildId = guildId; | ||
|
||
if ('user' in data) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can user ever be missing? 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going off the member docs where it's optional, but actually as far as we are concerned here (in interactions) it'll always be present. will change that
/** | ||
* Whether the user is deafened in voice channels | ||
* @type {?boolean} | ||
* @deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this deprecated, and what't the alternative?
@@ -1642,6 +1642,38 @@ export class GuildMember extends Base { | |||
public valueOf(): string; | |||
} | |||
|
|||
export class UncachedGuildMember extends Base { | |||
private constructor(client: Client<true>, data: RawGuildMemberData, guildId: Snowflake); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to your current implementation the type for data
here is wrong, as it only takes APIInteractionGuildMember | APIInteractionDataResolvedGuildMember
and not all the other UnionMembers RawGuildMemberData
contains.
* .catch(console.error); | ||
*/ | ||
|
||
TextBasedChannel.applyToClass(UncachedGuildMember); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won’t work without additional changes in TextBasedChannel#send()
, because that method uses a (granted very hacky) check to find out if it‘s called on a User/GuildMember or a Channel. Without changes it would currently treat UncachedGuildMember
like it was a channel and fail miserably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah thanks, will look into that
|
||
toJSON() { | ||
const json = super.toJSON({ | ||
guild: 'guildId', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no property named guild
in this class, needs to be
guild: 'guildId', | |
guildId: true, |
this.avatar === member.avatar && | ||
this.pending === member.pending && | ||
this.communicationDisabledUntilTimestamp === member.communicationDisabledUntilTimestamp && | ||
this.parsedFlags.bitfield === member.parsedFlags.bitfield && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.parsedFlags.bitfield === member.parsedFlags.bitfield && | |
this.flags === member.flags && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.flags is deprecated so I didn't want to use it; I want to rename parsedFlags to flags in v15
} | ||
|
||
/** | ||
* Deletes any DMs with this member. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It only deletes the DMChannel, not the DMs contained within. I know you just copied this from GuildMember
where it‘s wrong too. But it’s correct in User
.
* @deprecated Use {@link UncachedGuildMember#premiumSinceTimestamp} | ||
* or {@link UncachedGuildMember#premiumSince} instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @deprecated Use {@link UncachedGuildMember#premiumSinceTimestamp} | |
* or {@link UncachedGuildMember#premiumSince} instead | |
* @deprecated Use {@link UncachedGuildMember#premiumSinceTimestamp} or {@link UncachedGuildMember#premiumSince} instead |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is what I had originally, but it fails prettier due to the line length, so I split it up.
Co-Authored-By: Vlad Frangu <me@vladfrangu.dev> Co-Authored-By: Qjuh <76154676+Qjuh@users.noreply.github.com>
Marking the PR as both sem minor and major until I test it out and ensure this is a sem-minor PR 👀 |
* The user that this guild member instance represents | ||
* @type {User} | ||
*/ | ||
this.user = this.client.users._add(data.user, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn’t this a breaking change? APIInteractionGuildMember#user
is APIUser
and thus has snakecase properties. Now it doesn’t anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that is a breaking change indeed... @advaith1 can you solve that too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh, good catch. so I think the options here are
- add (deprecated, maybe undocumented) getters for
global_name
,accent_color
, andpublic_flags
to User - make a new class extending User that adds those, and make UncachedGuildMember construct one of those instead
for simplicity I'd prefer to do the former tbh... what do people think
|
||
/** | ||
* The nickname of this member, or their user display name if they don't have one | ||
* @type {?string} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would they not always have a display name, or is this something to do with partials?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm yeah, why is this nullable in GuildMember? also user is nullable in GuildMember but id (which just returns this.user.id) isn't
are both of those just incorrectly marked as nullable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would seem so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the user being nullable related to the
The field
user
won't be included in the member object attached toMESSAGE_CREATE
andMESSAGE_UPDATE
gateway events.
note that is under the Guild Member object table in the DAPI docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this sounds like something we need to investigate.... possibly seperately
but that would mean the docs on here could be wrong until investigation
complete
re posted because email replies dont work properly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In those two events the message.author
field contains the user
and is patched into the GuildMember
accordingly so won’t be null
either
this.guild.members._add(Object.assign(data.member, { user: this.author })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so in conclusion: the docs are wrong
* @example | ||
* // Send a direct message | ||
* UncachedGuildMember.send('Hello!') | ||
* .then(message => console.log(`Sent message: ${message.content} to ${guildMember.displayName}`)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* .then(message => console.log(`Sent message: ${message.content} to ${guildMember.displayName}`)) | |
* .then(message => console.log(`Sent message: ${message.content} to ${uncachedGuildMember.displayName}`)) |
Feels a bit silly but eh
); | ||
public avatar: string | null; | ||
public get dmChannel(): DMChannel | null; | ||
public get displayName(): string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i note infact from my previous comment that this is marked as always present but in the jsdoc it is not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on an AnyGuildMember
type which is union of both Uncached and GuildMember and we have functions that assert whether a member instance is the uncached one?
Please describe the changes this PR makes and why it should be merged:
fixes #10010
Discord thread
Currently, when an interaction is received from a guild that isn't cached*, raw API member objects are returned instead of a d.js GuildMember object. This PR creates a new UncachedGuildMember object that is returned instead of raw API member objects.
This new object tries to be compatible with GuildMember, but retains backwards compatibility with APIGuildMember as to not make this a breaking change. Therefore it is still somewhat awkward and not great, but it is much better than just receiving an APIGuildMember. Hopefully in v15 we can remove the backwards compatibility and align it better with GuildMember.
This has not been fully tested and likely needs changes - please thoroughly review!
* This was always possible by adding apps with only the applications.commands scope, but user apps makes this a much more common appearance. This PR is based on the discord.js patch AdvaithBot is running to properly handle user app commands.
Status and versioning classification: