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

feat(Interactions): member objects for uncached guilds #10195

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

advaith1
Copy link
Contributor

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:

  • I know how to update typings and have done so, or typings don't need updating
  • This PR changes the library's interface (methods or parameters added)

@advaith1 advaith1 requested a review from a team as a code owner March 29, 2024 03:47
Copy link

vercel bot commented Mar 29, 2024

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) Visit Preview Apr 4, 2024 11:00pm
discord-js-guide ⬜️ Ignored (Inspect) Visit Preview Apr 4, 2024 11:00pm

Copy link
Member

@vladfrangu vladfrangu left a 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 as nick, only nickname
  • data.roles isn't exposed at all, only as roleIds
    • 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}
Copy link
Member

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

Copy link
Contributor Author

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) {
Copy link
Member

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? 👀

Copy link
Contributor Author

@advaith1 advaith1 Mar 29, 2024

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
Copy link
Member

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?

packages/discord.js/typings/index.d.ts Show resolved Hide resolved
@@ -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);
Copy link
Contributor

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);
Copy link
Contributor

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.

Copy link
Contributor Author

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',
Copy link
Contributor

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

Suggested change
guild: 'guildId',
guildId: true,

this.avatar === member.avatar &&
this.pending === member.pending &&
this.communicationDisabledUntilTimestamp === member.communicationDisabledUntilTimestamp &&
this.parsedFlags.bitfield === member.parsedFlags.bitfield &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this.parsedFlags.bitfield === member.parsedFlags.bitfield &&
this.flags === member.flags &&

Copy link
Contributor Author

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.
Copy link
Contributor

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.

Comment on lines +73 to +74
* @deprecated Use {@link UncachedGuildMember#premiumSinceTimestamp}
* or {@link UncachedGuildMember#premiumSince} instead
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @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.

Copy link
Contributor Author

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.

packages/discord.js/src/structures/UncachedGuildMember.js Outdated Show resolved Hide resolved
advaith1 and others added 2 commits April 2, 2024 21:34
Co-Authored-By: Vlad Frangu <me@vladfrangu.dev>
Co-Authored-By: Qjuh <76154676+Qjuh@users.noreply.github.com>
@vladfrangu
Copy link
Member

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);
Copy link
Contributor

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.

Copy link
Member

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?

Copy link
Contributor Author

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, and public_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}
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would seem so

Copy link
Contributor

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 to MESSAGE_CREATE and MESSAGE_UPDATE gateway events.

note that is under the Guild Member object table in the DAPI docs?

Copy link
Contributor

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

Copy link
Contributor

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 }));

Copy link
Contributor

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}`))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* .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;
Copy link
Contributor

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

Copy link
Member

@vladfrangu vladfrangu left a 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Review in Progress
Development

Successfully merging this pull request may close these issues.

Return a d.js object instead of a raw member object for interactions from uncached guilds
7 participants