-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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: Add ApplicationEmoji to EmojiResolvable and MessageReaction#emoji #10477
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 Skipped Deployments
|
This may also need updating discord.js/packages/discord.js/src/managers/BaseGuildEmojiManager.js Lines 23 to 28 in a11ff75
|
Thanks I updated BaseGuildEmojiManager.js and it seemed to fix the equality comparison with |
|
||
public burstColors: string[] | null; | ||
public readonly client: Client<true>; | ||
public count: number; | ||
public countDetails: ReactionCountDetailsData; | ||
public get emoji(): GuildEmoji | ReactionEmoji; | ||
public get emoji(): GuildEmoji | ReactionEmoji | ApplicationEmoji; |
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 its current state, it is not possible for MessageReaction#emoji
along with its associated private property to return an ApplicationEmoji, which would make these typings inaccurate.
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 think it makes sense for a MessageReaction done by the bot with an ApplicationEmoji to return ApplicationEmoji for its emoji property. Do you know where I can make these changes?
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.
Since application emojis need to be fetched to be had, I wouldn't actually try to squeeze that in. it's fine to leave it as before
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 not add a cache check for ApplicationEmoji here?
discord.js/packages/discord.js/src/structures/MessageReaction.js
Lines 120 to 127 in 4594896
if (this._emoji.id) { | |
const emojis = this.message.client.emojis.cache; | |
if (emojis.has(this._emoji.id)) { | |
const emoji = emojis.get(this._emoji.id); | |
this._emoji = emoji; | |
return emoji; | |
} | |
} |
If it‘s not in cache the behavior would stay as is, if cached you‘d get the ApplicationEmoji.
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.
Oof, that getter. I'd maybe add that logic into the getter to store app emojis if we really wanna go there
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 have added ApplicationEmoji
lookup to the 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.
That now changes the scope of the PR to not be types only. Please edit the PR title and description accordingly and make sure you actually tested that change.
// Check to see if the emoji has become known to the client | ||
if (this._emoji.id) { | ||
const emojis = this.message.client.emojis.cache; | ||
if (emojis.has(this._emoji.id)) { | ||
const emoji = emojis.get(this._emoji.id); | ||
this._emoji = emoji; | ||
return emoji; | ||
} else { | ||
const applicationEmojis = this.message.client.application.emojis.cache; |
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.
Should be moved under line 124
Please describe the changes this PR makes and why it should be merged:
When using typescript you can't react to a message with an
ApplicationEmoji
becauseEmojiResolvable
lacks the type.Edit: I also updated BaseGuildEmojiManager.js and added
ApplicationEmoji
to theMessageReaction#emoji
getter.Status and versioning classification: