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): option to auto-fetch replies #5831

Merged
merged 4 commits into from
Jun 29, 2021

Conversation

monbrey
Copy link
Member

@monbrey monbrey commented Jun 12, 2021

Stemming from internal discussion, this is one of two opposing PRs which provide a way to automatically fetch the reply/deferral/update to an interaction, assuming it's not ephemeral.

This PR adds an option to the methods, allowing the user to enable auto-fetching. The default maintains the existing behaviour. I've written it such that attempting to auto-fetch an ephemeral reply, or auto-fetch an update to an ephemeral message throws an error - this can be changed of course.

(Testing cross-PR closing) - Closes #5830

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)

src/structures/interfaces/InteractionResponses.js Outdated Show resolved Hide resolved
src/structures/interfaces/InteractionResponses.js Outdated Show resolved Hide resolved
@iCrawl
Copy link
Member

iCrawl commented Jun 15, 2021

This needs a rebase.

@iCrawl
Copy link
Member

iCrawl commented Jun 23, 2021

This needs a rebase.

@monbrey monbrey force-pushed the interaction-autofetch-option branch from 8b365de to 3797966 Compare June 23, 2021 23:22
@monbrey
Copy link
Member Author

monbrey commented Jun 23, 2021

Rebased

@iCrawl
Copy link
Member

iCrawl commented Jun 24, 2021

This needs a rebase.

@monbrey monbrey force-pushed the interaction-autofetch-option branch from 3797966 to 2149f8b Compare June 24, 2021 21:20
@SpaceEEC
Copy link
Member

I'm not sure what the benefit of this PR really is. (Probably because I missed the internal discussion about this)
Are we trying to "fix" Discord not responding with message data?


We merged and will probably still merge a bunch of PRs making overloads fewer and simpler.
This PR however is adding another and breaks passing object with an unknown fetch reply status, e.g.

const options: InteractionReplyOptions = this.getOptions(); // might have fetchReply, but might not
const maybeMessage = await interaction.reply(options); // option is not assignable to either overload

To fix this yet another overload is needed that returns Message | RawMessage | void.


Alternatively to this PR developers could do something like this: (Or anything equivalent to this)

const message = await interaction.reply("hello there").then(() => interaction.fetchReply())

@monbrey
Copy link
Member Author

monbrey commented Jun 26, 2021

Yeah @SpaceEEC, fixing Discord not responding with any data is exactly the intention :D

I don't really have any strong preference for this feature existing or not - a lot of users are already doing the manual fetching. If it's generally agreed to still not be in line with how the lib should be developed (internal fetches, less overloads etc) lets just close it.

@kyranet
Copy link
Member

kyranet commented Jun 26, 2021

If we want to keep code and overloads simple, we can simply not merge this PR and tell people to call that method themselves. There are multiple solutions for the issue at hand after all:

  • We fetch for the reply with an HTTP call, has the problem that it makes the methods take longer and thus the bot becomes irresponsive for a brief period of time (the major use-case for this seems to be interaction collectors, after all).
  • We collect a message from MESSAGE_CREATE that matches all the filters, then return it. It has the problem that it's bound to the bot scope (which is optional with applications.commands) and high CPU usage as it needs to filter many messages, specially in large internal sharding.
  • We rewrite collectors to not require the response's message ID. I do not know what other use-cases do users have for fetching the reply.
  • We educate our users about stateless and how they can use custom_id to avoid collectors, this is the solution I'll 100% go for (100 bytes is more than plenty of space if you use binary data), but I also understand that it's a very advanced and complex topic many users won't understand.

If there are more use-cases for this feature, please let me know and maybe we can think of an alternative solution.

@iCrawl iCrawl added this to the Version 13 milestone Jun 27, 2021
@iCrawl
Copy link
Member

iCrawl commented Jun 28, 2021

This needs a rebase.

@monbrey monbrey force-pushed the interaction-autofetch-option branch from e65853d to 6f1e48c Compare June 28, 2021 11:37
@iCrawl iCrawl merged commit 5e28ff8 into discordjs:master Jun 29, 2021
@monbrey monbrey deleted the interaction-autofetch-option branch June 30, 2021 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants