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(MessageReaction): add react method #7810

Merged
merged 1 commit into from
Jun 5, 2022

Conversation

ImRodry
Copy link
Contributor

@ImRodry ImRodry commented Apr 17, 2022

Please describe the changes this PR makes and why it should be merged:

This PR adds the MessageReaction#react method to make it easier to add an existing reaction to a message. Its purpose is to make it easier for bots to react with emojis they do not know in their cache. The name is up for debate if anyone has better ideas, this was the best I could come up with

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • 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)

@vladfrangu
Copy link
Member

This PR adds the MessageReaction#react method to make it easier to add an existing reaction to a message. Its purpose is to make it easier for bots to react with emojis they do not know in their cache.

I mean no, you know the emoji since you have the message reaction class :P (+ you don't neeeed the emoji cached to add reactions)
This is just a shortcut 😄

@ImRodry
Copy link
Contributor Author

ImRodry commented Apr 17, 2022

Exactly, I said make it easier not make it possible :)

@monbrey
Copy link
Member

monbrey commented Apr 17, 2022

I don't quite understand this. It's a shortcut for the bot to react with the same emoji a user did?

...I guess it's fine?

@ImRodry
Copy link
Contributor Author

ImRodry commented Apr 17, 2022

Yep that’s pretty much it

@suneettipirneni
Copy link
Member

I don't understand why this method needs to be a part of discord.js. Making a utility method for this would be trivial:

const rereact = async (msg: Message, reaction: MessageReaction) => msg.react(reaction.emoji);

In addition, the use cases to "duplicate" a reaction on a message seem niche. Beyond that, this method makes no structural sense.

A message is a parent to its reactions, the reactions are children of the message. It doesn't make sense for a child to be able to manage its parent. It's the same reason there's no .send() method on Message which sends a "copy" of the message to the channel the original message was sent in. A message shouldn't have a method that implicitly manages its parent/owner.

These kinds of methods also lack readability.

messageRxn.react()

Are we reacting to a message reaction? How is that possible? Where's the arguments? Even with a better name the method will always feel out of place due to the fact that a single message reaction isn't responsible for modifying the collection of message reactions on a given message.

message.react(...)

This is much clearer to see we're reacting to a message here. And the method feels like it's in the right place because messages own reactions, not vice-versa.

@ImRodry
Copy link
Contributor Author

ImRodry commented Apr 17, 2022

It's the same reason there's no .send() method on Message which sends a "copy" of the message to the channel the original message was sent in. A message shouldn't have a method that implicitly manages its parent/owner.

On the Discord client you can't click a message to send one similar to it and you can click a reaction to react with that emoji too, that was the rationale behind this

Are we reacting to a message reaction? How is that possible? Where's the arguments?

You know that's not the case so why are you even making that an argument, that's just stupid. This is simply grabbing an existing reaction by someone else and adding a reaction from the client user similar to the one that was given, nothing more nothing less. If the maintainers don't want to add this they can close the PR but I'm tired of reading a thesis from you every time I open a PR, so please stop, you're not a maintainer

@iCrawl iCrawl added this to the discord.js v14 milestone Apr 22, 2022
@DraftProducts
Copy link
Contributor

I find the addition of this shortcut relevant, especially in the case of reaction vents where <MessageReaction> is directly returned as a parameter.

Reading is much easier in the second demo, and someone who has read the doc knows that reactionMessage represents an emoji linked to a message.
It is therefore understandable that you cannot add another emoji than the one concerned by interacting with this method.

client.on('messageReactionAdd', async (reactionMessage, _user) => {
   await reactionMessage.message.react(reactionMessage.emoji);
})

client.on('messageReactionAdd', async (reactionMessage, _user) => {
   await reactionMessage.react();
})

If the lack of arguments to this method is problematic at this point, could a name change be considered? Something like <MessageReaction>.reReact()?

The project is open-source, it is so that the community can give its opinion and contribute to it, just as much as you allow yourself to propose this PR, it is normal that @suneettipirneni has the right to bring an opinion opposed to yours. 😕

@vladfrangu
Copy link
Member

If the lack of arguments to this method is problematic at this point, could a name change be considered? Something like <MessageReaction>.reReact()?

Personally I'd rather approve the PR with the current naming rather than going with reReact... The naming is something that can be improved (maybe? nobody has offered any decent alternatives for now) but I doubt it'll be a blocker

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.

Past the naming argument which I've mentioned like 3? times now, LGTM

@AverageHelper
Copy link

I personally like this change. It's sometimes fun to see a bot agree with your reaction, even if you know the cases where it might and it's mostly arbitrary. Since I know one and a half bots myself that have this functionality already, making that code easier is a win imo.

@AverageHelper
Copy link

AverageHelper commented Jun 2, 2022

nobody has offered any decent alternatives for now

Has @suneettipirneni's helper function's name (rereact) been considered? I'm not sure what the idiom is when a user does the same thing, that is, reacting with the same reaction another user/bot made, especially by clicking on the existing reaction rather than finding it in the list. Is that "rereacting," "re-reacting," or perhaps just "also reacting"? The effect is the same externally, but since the distinction for users (between clicking the existing reaction and finding the emote in a list) is client-side, perhaps the syntax should reflect that.

The point's been made that some devs unfamiliar with this new react method would be confused, wondering whether it's meant to react to the reaction itself (somehow), and they would need to refer to the method's documentation to understand that it's really just duplicating the reaction that came in. This confusion is, imo, understandable since the method appears (from an external perspective) to modify a MessageReaction instance, not a Message instance. Perhaps a different verb would make the intention clear.

This might be the bot (author?) agreeing with the first reaction. agree or agreeWith might get that across, but I'm no API designer lol. There's also affirm, concur, and duplicate.

TL;DR: The new method does appear to modify a MessageReaction, tho it really operates on a Message, just like the user's client. Maybe the method name should reflect the actual intention.

(EDIT: Some more naming options I thought about just after sending)

@BenPatersonxx

This comment was marked as off-topic.

@ImRodry
Copy link
Contributor Author

ImRodry commented Jun 5, 2022

Just to be clear: if the maintainers agree on any better naming for this method I'll gladly change it but I'd like that opinion to come from them so we don't keep changing things back and forth. Appreciate the feedback so far though

Copy link
Member

@kyranet kyranet left a comment

Choose a reason for hiding this comment

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

I was thinking of reaction.users.addMe(), but I really can't think of a better name.

@iCrawl iCrawl merged commit a328778 into discordjs:main Jun 5, 2022
@ImRodry ImRodry deleted the feat/messagereaction-react branch June 5, 2022 21:33
Jiralite added a commit to suneettipirneni/guide that referenced this pull request Jun 6, 2022
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.

None yet

10 participants