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: Prettier way to send a single embed/file/component #10496

Closed
wants to merge 2 commits into from

Conversation

Moebits
Copy link
Contributor

@Moebits Moebits commented Sep 9, 2024

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

The problem:
I know this comes down to stylistic preference, but I don't like the way you have to send a single embed/file/component, making me having to use [x] everywhere in my code:

await interaction.reply({embeds: [embed], files: [file], components: [actionRow]})

This is such a simple check to make with Array.isArray. I also added the types MessageEmbed, FileResolvable, and MessageActionRow to aid in the typings.

New method:

await interaction.reply({embeds: embed, files: file, components: actionRow})

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

@Moebits Moebits requested a review from a team as a code owner September 9, 2024 01:10
Copy link

vercel bot commented Sep 9, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
discord-js ⬜️ Ignored (Inspect) Visit Preview Sep 9, 2024 1:10am
discord-js-guide ⬜️ Ignored (Inspect) Visit Preview Sep 9, 2024 1:10am

@tipakA
Copy link
Contributor

tipakA commented Sep 9, 2024

[...] making me having to use [x] everywhere in my code

export function myInteractionReply(interaction, { embed, file, actionRow, ...extra } = {}) {
  return interaction.reply({ embed: [embed], file: [file], components: [actionRow], ...extra });
}

And now you no longer do that anywhere 🎉

@Moebits
Copy link
Contributor Author

Moebits commented Sep 9, 2024

[...] making me having to use [x] everywhere in my code

export function myInteractionReply(interaction, { embed, file, actionRow, ...extra } = {}) {
  return interaction.reply({ embed: [embed], file: [file], components: [actionRow], ...extra });
}

And now you no longer do that anywhere 🎉

The entire point of this (as well as closed #10491) is to not patch up library methods in my own code.

@tipakA
Copy link
Contributor

tipakA commented Sep 9, 2024

[...] to not patch up library methods in my own code.

But my solution isn't touching anything with the library? It is an example of a method you can implement in your code, and use in your code. You can create whatever helper classes and functions you desire this way, this is not an exotic concept.

@Qjuh
Copy link
Contributor

Qjuh commented Sep 9, 2024

Let me be blunt here: this (and your previous PR) looks like a PR trying to partially revert discord.js to the way it was in older versions. And not because there’s actual valid library design decisions considered but merely because you are used to it and are now (after coming back to it after years) still used to the old way.

There’s been enough discussions about it when that change was made 2 years ago. We won’t change it just because you want it for your personal code without any considerations of maintainability or the broad userbase of this package.

I‘ll try again: we are open to discussions showing valid improvements if they consider those two aspects. But keeping to open PRs without those will not get them merged.

@didinele
Copy link
Member

didinele commented Sep 9, 2024

I think you should broaden your horizons with some other langs where you'll notice no one really adds those "helper" overloads to their methods, even though it's a lot easier in a language with actual type overloads.

Either way, as explained above, there has already been extensive discussion on this topic.

@didinele didinele closed this Sep 9, 2024
@Moebits
Copy link
Contributor Author

Moebits commented Sep 9, 2024

Let me be blunt here: this (and your previous PR) looks like a PR trying to partially revert discord.js to the way it was in older versions. And not because there’s actual valid library design decisions considered but merely because you are used to it and are now (after coming back to it after years) still used to the old way.

I don't understand why this change was made. It feels like a lot of the things from v12 were reworked in a more convoluted manner. I don't think it's me "being used" to the old way, but that the old way was objectively more user friendly.

Can you at least explain the reason/link to a discussion why you can't do an isArray check?

@vladfrangu
Copy link
Member

This is gonna end up in an endless circle of debate as to why we did certain library changes that become opinionated on "but I liked it before".

The changes mentioned here were done because Discord made it so. Bots can send multiple embeds, bots can attach multiple files, bots can add in more component rows, we just adapted to that. Adding in overloads for one item is gonna cause a lot of tech debt, and possible unknown side effects / missed places causing subtle bugs until reported. Your best bet is to make utility functions for your common use cases and using those instead.

@Moebits
Copy link
Contributor Author

Moebits commented Sep 9, 2024

If you don't want to overload the parameters, then what about adding separate methods like replyWithSingle and sendWithSingle that accepts single embed, file, or component?

And rather than changing the interface of reply and send to be less friendly to use, you should have added replyWithMultiple or sendWithMultiple to send arrays if you didn't want to overload it. 90% of the time a bot only wants to send one, so accepting arrays should have been treated like an extra case not the default.

@almostSouji
Copy link
Member

almostSouji commented Sep 9, 2024

Thanks for sharing your viewpoint, option keys will generally follow upstream conventions and paradigms, if applicable.
Wrapping a single instance into an array is negligible overhead both in performance and typing effort, but results in a much cleaner interface.

If you feel passionate about this, consider wrapping the functionality into methods as you deem fit in user land or fork the library.
Writing functions to make for API wrapper interfaces more comfortable to use for your specific use case is a common practice in software development. We cannot bend the interface to fit every use case.

I think between us we have sufficiently covered the reasoning for not going forward with these suggestions.

@discordjs discordjs locked as resolved and limited conversation to collaborators Sep 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants