Moebits commented 3 weeks ago

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

The problem:

It has been tedious for me to add SlashCommandOption's with every type being it's own class. This usually results in bloated imports that must be changed between commands

import { SlashCommandStringOption, SlashCommandChannelOption, 
SlashCommandBooleanOption, SlashCommandNumberOption, 
SlashCommandMentionableOption, SlashCommandRoleOption, 
SlashCommandAttachmentOption } from "discord.js" // imports are tedious

const userOption = new SlashCommandMentionableOption()
    .setName("user")
    .setDescription("User")

const subcommand = new SlashCommandSubcommandBuilder()
    .setName("subcommand")
    .setDescription("description")
    .addMentionableOption(userOption) // different method for each type

To improve ease of use, I propose adding two changes:

A SlashCommandOption helper class that can be set to any type in .setType()
Addition of SharedSlashCommandOptions#addOption that supports adding option of any type

Now with my proposed changes, the code will look like this:

import { SlashCommandOption } from "discord.js" // cleaner imports

const userOption = new SlashCommandOption()
    .setType("mentionable")
    .setName("user")
    .setDescription("User")

const subcommand = new SlashCommandSubcommandBuilder()
    .setName("subcommand")
    .setDescription("description")
    .addOption(userOption) // supports any type

SlashCommandOption#setType supports passing a string or ApplicationCommandOptionType.

This also works:

const userOption = new SlashCommandOption()
    .setType(ApplicationCommandOptionType.Mentionable)

No breaking changes

This PR does not rename or remove any old methods so it doesn't have breaking changes. If people prefer the old method they can continue to use it.

Use cases

I believe this makes it easier for me to create SlashCommandOptions and quickly copy code over to a different command without having to deal with changing imports and methods.

Status and versioning classification:

Code changes have been tested against the Discord API, or there are no code changes
This PR changes the library's interface (methods or parameters added)

vercel[bot] commented 3 weeks ago

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

2 Skipped Deployments

vladfrangu commented 3 weeks ago

You should use the callback syntax if importing the classes is too "tedious"

Moebits commented 3 weeks ago

You should use the callback syntax if importing the classes is too "tedious"

I think that the callback syntax is too confusing. I like to create all the options separate and attach them to their subcommand.

codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 25.88235% with 63 lines in your changes missing coverage. Please review.

Project coverage is 35.39%. Comparing base (8a74f14) to head (b48ad30).

Files with missing lines	Patch %	Lines
...ders/src/interactions/slashCommands/options/all.ts	17.54%	47 Missing :warning:
.../slashCommands/mixins/SharedSlashCommandOptions.ts	42.85%	16 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #10491 +/- ## ========================================== - Coverage 35.45% 35.39% -0.06% ========================================== Files 228 229 +1 Lines 14319 14404 +85 Branches 1254 1255 +1 ========================================== + Hits 5077 5099 +22 - Misses 9198 9261 +63 Partials 44 44 ``` | [Flag](https://app.codecov.io/gh/discordjs/discord.js/pull/10491/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=discordjs) | Coverage Δ | | |---|---|---| | [builders](https://app.codecov.io/gh/discordjs/discord.js/pull/10491/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=discordjs) | `88.30% <25.88%> (-3.29%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=discordjs#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tipakA commented 3 weeks ago

I think that the callback syntax is too confusing.

Could you elaborate on why is that? Maybe we could look into addressing this pain point instead.

Here's a slightly cut-down snippet from my current code. There's a single import from discord.js related to this entire builder.

import { SlashCommandBuilder } from 'discord.js';

const deployData = new SlashCommandBuilder()
  .setName(commandName)
  .setDescription('Fix something')
  .addSubcommandGroup(group => { // member
    return group
      .setName('member')
      .setDescription('Fix Member')
      .addSubcommand(sub => { // separator-roles
        return sub
          .setName('separator-roles')
          .setDescription('Fix member\'s separator roles')
          .addUserOption(option => { // member
            return option
              .setName('member')
              .setDescription('Member whose roles to fix')
              .setRequired(true);
          })
          .addBooleanOption(option => { // show
            return option
              .setName('show')
              .setDescription('Should the response be visible?');
          });
      });
  });

Admittedly, I agree that I could switch this to implicit return instead, but using explicit one makes this look better for me. Not like it matters overall though, this code was written once and collapsed both within the blocks (that's why there are name comments on the block starts), as well as with a // #region SlashCommandBuilder wrapping it. So far I didn't have to edit it since I wrote it 2 months ago.

If you're still curious about what else this 537 LOC file imports from discord.js, here:

```ts import { ButtonInteraction, ChannelType, ChatInputCommandInteraction, Collection, EmbedBuilder, GuildBasedChannel, GuildMember, PermissionFlagsBits, PermissionsString, PrivateThreadChannel, PublicThreadChannel, RoleMention, SlashCommandBuilder, Snowflake, } from 'discord.js'; ```

As far as I'm aware, you would not be cutting anything out of this list regardless of how much you'd change up the Builder. By using JSON object directly instead of deployData.toJSON() for the command class, I would save on exactly 1 import. And this isn't even a file with the most imports, other commands of mine also import ButtonBuilders and several types of SelectMenuBuilders.

Moebits commented 3 weeks ago

My issue with that is mainly the same reasons I prefer using async/await over callbacks. It results in a giant "pyramid" that becomes very hard to read the more subcommands/options you add.

The way I do it by separating the options and attaching them later is much easier to read at a glance for me. It's easier to change/remove individual options without going into the pyramid and trying to figure out which block corresponds to which option.

const showOption = new SlashCommandOption()
    .setType("boolean")
    .setName('show')
    .setDescription('Should the response be visible?');

const memberOption = new SlashCommandOption()
    .setType("user")
    .setName('member')
    .setDescription('Member whose roles to fix')
    .setRequired(true)

const sub = new SlashCommandSubcommandBuilder()
    .setName('separator-roles')
    .setDescription('Fix member\'s separator roles')
    .addOption(memberOption)
    .addOption(showOption)

const group = new SlashCommandSubcommandGroupBuilder()
    .setName('member')
    .setDescription('Fix Member')
    .addSubcommand(sub)

const deployData = new SlashCommandBuilder()
    .setName(commandName)
    .setDescription('Fix something')
    .addSubcommandGroup(group)

almostSouji commented 3 weeks ago

This seems like an awfully convoluted PR to support a single use case that's based on style preference. I understand the excitement of wanting to improve handling to what feels more comfortable to use for you, but as a library we have to factor in maintainability, which this seems to negatively affect, since this likely won't be a one-off change if Discord introduces more option types.

Builders exist to ease people into the concept, provide autocomplete and optionally validation. If you are looking to support advanced use cases and imports are that much of an issue, potentially consider supplying objects directly instead of using builders .

imnaiyar commented 3 weeks ago

The way I do it by separating the options and attaching them later is much easier to read at a glance for me.

You can still achieve something similar tho.

Consider the following snippet;


const command = new SlashCommandBuilder()
.setName("name")
.setDescription("description");

// add a boolean option
command.addBooleanOption(option => option.setName("bool").setDescription("desc"))

// add string option
command.addStringOption(...)

// or for a subcommand
const sub = new SlashCommandSubcommnadBuilder()
.setName(...)
.setDescription(...);

// add an option to it
sub.addX(...)
sub.addY(...)

// add subcommand to the command
command.addSubcommand(sub);

// similary for subcommand group

There's nothing "pyramind" going here

vladfrangu commented 3 weeks ago

Souji basically said my feelings about this PR in its current form. I also don't understand why exactly you have an extra function to do something that can be done by just exporting an object with all builders in it and using that (not that it'd make my feelings towards this much better)

Ultimately, if this is something others want I will fold, but imo this is one of those abstractions better fit in your own code. Or just split your declarations up like shown in the previous comment by @imnaiyar.

Moebits commented 3 weeks ago

Ok I found the interface I am happy with which is to export a SlashCommandOption class with a setType method this way it's more consistent with how you create the other parts of a slash command.

This is what usage looks like:

const questionOption = new SlashCommandOption()
    .setType("string")
    .setName("question")
    .setDescription("question")
    .setRequired(true)

I should note that the linter formats the code very weirdly and ruins the spacing on the conditional types. I tried to fix the spacing but it made the test fail so I just left it that way.

If you want me to be honest, I don't really get the point of having so many type classes when the code between them is 90% similar and you could easily combine them into one class. I think there's a lot of unnecessary duplication in the code. However, I got very lukewarm responses to combining them in the discord so this is basically my way of still getting the functionality I wanted without a breaking change.

Yes, I could just implement this into my code. I already did that. But by opening this PR, I hoped that my solution could be helpful to anyone else in the community by being integrated into the library. Am I really the only one that found importing all the option type classes a bit tedious?..

didinele commented 3 weeks ago

If you want me to be honest, I don't really get the point of having so many type classes when the code between them is 90% similar and you could easily combine them into one class. I think there's a lot of unnecessary duplication in the code. However, I got very lukewarm responses to combining them in the discord so this is basically my way of still getting the functionality I wanted without a breaking change.

It was already explained to you multiple times. On top of the maintenance concerns, you legitimately wouldn't even be able to combine them completely because type-guarding away methods based off of a generic is a nightmare-ish task that probably makes you create separate classes internally anyway, even if to the user it seems like one.

Am I really the only one that found importing all the option type classes a bit tedious?..

Yes, use a modern code editor that imports for you automatically, or, for the 10th time, use the callback API.

You're not fixing a legitimate problem with this PR or improving DX so much to the point where it's worth the tech debt you're introducing, which, might I add, is more than builders has seen in its entire lifetime. SlashCommandOption#setType is terribly slow for what it's meant to do, probably has edge-cases that don't work well with that try..catch block, and extremely prone to breaking, while the types are a complete nightmare and look exactly like the sort of thing we've regretted every time in the past.

I, much like @vladfrangu and @almostSouji am against this.

Moebits commented 3 weeks ago

Yes, use a modern code editor that imports for you automatically, or, for the 10th time, use the callback API.

The library provides two separate methods of adding slash command options: 1) importing the type classes and 2) using the.addX() callbacks. My concern is only on improving (1).

My main question is... why do I have to import so many different types? Exporting a simple helper to get any of them is a basic functionality (that should preferably be implemented by the library, not by me).

I don't feel like the issue is so much maintainability, but a clash of style preference. I don't like to use the callback syntax or import so many classes in my code, and this is a suitable solution for me. But it seems like no one else likes my style so they don't want to add this. Claiming that the issue is maintainability when I only added one more file you have to modify just doesn't seem right to me.

Is it really that difficult to add another slot to the conditional types when (once every blue moon) discord adds a new option type? And eventually they will stop adding more option types, there are only so many that they can add. Interface changes like adding a new method to an option type will not break my code.

didinele commented 3 weeks ago

Builders is opinionated. Wherever we provide multiple ways to do something it's because we think it's an improvement to the developer experience that doesn't compromise the set of patterns we've adopted.

Your proposed changes do not fit the patterns in question at all, so it's not even an issue of "style", but rather consistency. Even if this wasn't a consideration, your DX improvement here is "you don't have to import classes anymore", which we already provide a solution to. "I don't like your solution" is not a good enough reason to add another one, especially with how convoluted it is. I'm not even certain why you're using builders, it seems like you would benefit from raw objects a lot more.

As per your comment on maintainability, I've raised multiple points that you haven't addressed & there's more to it than that anyway.

Moebits commented 3 weeks ago

As per your comment on maintainability, I've raised multiple points that you haven't addressed & there's more to it than that anyway.

If complexity is really your concern then I can export a simple object as someone mentioned before. I prefer the call interface less, but on your end it does remove the need for conditional types.

export const SlashCommandOption = {
    attachment: SlashCommandAttachmentOption,
    boolean: SlashCommandBooleanOption,
    channel: SlashCommandChannelOption,
    integer: SlashCommandIntegerOption,
    mentionable: SlashCommandMentionableOption,
    number: SlashCommandNumberOption,
    role: SlashCommandRoleOption,
    string: SlashCommandStringOption,
    user: SlashCommandUserOption
}

But something tells me that it wouldn't make you happy because maintainability wasn't the real issue you had.

You mentioned consistency but I looked at the rest of the builders and I just find the consistency to be... kinda off? I expect each of the builders to each follow the same interface designs but it seems like every single builder is doing things their own way.

This is how you add things to an ActionRowBuilder or ModalBuilder. It is not addButton() or addRow(), but they accept a type generic addComponents() method and you have to set a generic type to define the type of builder.

const buttonRow = new ActionRowBuilder<ButtonBuilder>()
buttonRow.addComponents(button1, button2, button3)

The StringSelectBuilder accepts a addOptions() that can take an arbitrary number of options. It makes sense for these, but why doesn't addX() in SlashCommandOptions accept multiple options?

const option1  = new StringSelectMenuOptionBuilder()
    .setLabel("label")
    .setValue("value")

const select = new StringSelectMenuBuilder()
    .addOptions(option1)

Then there is a weird choice of renaming MessageEmbed to EmbedBuilder and also putting it here, even though a message embed is not an interaction component like the rest of the builders.

The AttachmentBuilder is not even in the builders package, but in the discord.js package! And that's not an interaction either.

To me it looks like adding a SlashCommandOption export would be one of the least consistency issues. But I actually find it more consistent with the other slash command builders because type is a property that should be settable instead of being on a different option class.

didinele commented 3 weeks ago

You're now back-pedaling and looking for straws to pick. I thought I was having a good faith conversation & went out of my way to tell you why your changes are not in-line with our vision for the package.

This is how you add things to an ActionRowBuilder or ModalBuilder. It is not addButton() or addRow(), but they accept a type generic addComponents() method and you have to set a generic type to define the type of builder.

10448

The StringSelectBuilder accepts a addOptions() that can take an arbitrary number of options. It makes sense for these, but why doesn't addX() in SlashCommandOptions accept multiple options?

Because that'd make for a bad API that encourages more complex code updates when, say, changing the type of an option. When I cite "consistency" this doesn't mean everything is going to be the exact same without regard for other factors. For example, if we were to introduce more builders, one which takes a bunch of "options" of the same type (e.g. a bunch of strings), and one which is similar to the slash command options, then the former would be consistent with the string select menu builder, while the latter would be consistent with the slash command options.

Then there is a weird choice of renaming MessageEmbed to EmbedBuilder and also putting it here, even though a message embed is not an interaction component like the rest of the builders.

Quote from the package description:

@discordjs/builders is a utility package for easily building Discord API payloads.

Not sure where else you'd want this to be? Of course all the other builders are related to interactions, because the API doesn't really have any other constructs that mandate a builder... The rename is not at all a "weird choice".

The AttachmentBuilder is not even in the builders package, but in the discord.js package! And that's not an interaction either.

Yes, it's not really a builder, it's poorly named, and quite frankly, there are issues with it, we're already aware of this and addressing it after #10448

Multiple maintainers have come out of their way to provide you with solutions and explanations for why we're not happy with this pull request. We tried having a conversation (as opposed to simply closing the PR with a single-sentence response), but we were met the idea that this is some sort of egoistical behavior on our part. Also worth quoting this part that you edited it out

Maybe library authors get defensive after I implied there was something wrong with their design choices.

This is honestly deeply inappropriate and bordering a violation of our code of conduct.

Your arguments have left me unconvinced about these changes and I believe at this point we'll just be going in circles if we continue, so I'll be resigning from this conversation. If other maintainers wish to go further with this, great.

almostSouji commented 3 weeks ago

Thanks for the suggestion. As various people have iterated on above we do not think this is feasible to introduce on the library level and are not going forward with a merge. If you find it worth the effort, consider wrapping this in userland or consider forking the library to make it fit your specific use case.

Moebits commented 3 weeks ago

I expressed my dissatisfaction using all the import classes and the callback interface, but if you don't want to add my solution there isn't much to be helped. I will implement it into my own code.

discordjs / discord.js

feat: SlashCommandOption#setType and SharedSlashCommandOptions#addOption #10491

Codecov Report

10448