feat: Prettier way to send a single embed/file/component

Moebits commented 2 months ago

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

vercel[bot] commented 2 months ago

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

2 Skipped Deployments

tipakA commented 2 months ago

[...] 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 commented 2 months ago

[...] 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 commented 2 months ago

[...] 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 commented 2 months ago

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 commented 2 months ago

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.

Moebits commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 / discord.js

feat: Prettier way to send a single embed/file/component #10496