RFC(builders): The interface, the validation, and the pains you have had with them so far

[vladfrangu]

Preface: this RFC (Request For Comments) is meant to gather feedback and opinions for the 3 issues that will be mentioned down below. I understand builders tend to be quite a sensitive topic with everyone disagreeing on how it should be done, so please keep the feedback useful. Thanks!

This RFC aims to solve (hopefully once and for all) the three pain points of builders as they currently are done. The end goal is to decide what the interface should look like for all current and possibly future builders! It is split into 3 separate parts, denoted by the headings.

Before you answer, please read through the entire RFC, and don’t jump to answering just one question only. You don’t have to answer question 3 if you have nothing to add to it, but the other two would be super appreciated to receive answers to. Thanks for reading this, and now let’s get started~!

---

1. The interface

One of the key issue with the current builders is the lack of a consistent interface between the Application Command builders, the Message Component builders and the Embed builder.

As it stands at the time of writing this RFC:

• Application command builders follow a nested approach, with .add* methods for each option type. The class’s fields represent the structure similar to how the api payload will look like.
• Message component builders follow part of the same structure as application command builders, with some caveats (addComponents takes in any component, whereas add* in application command builders add just that one kind of options), some options that take in arrays allow rest parameters too (something that in my opinion is fine except for set* methods that should take in an array only). The class stores all data in a data property, and accesses it for setting/reading the properties in it
• Embed builder also stores the actual api embed data in the data field. They also allow input in camelCase format, where no other builder allows (or should allow) that.

Several things need to be decided:

1. How should the classes store the data that will be returned from toJSON() calls? In a data field or as the class fields themselves?
2. Should addComponents be removed in favor of addButton, addSelectMenu and so on? Or should application command builders use addOption instead?
3. Should nested objects in any builder be a raw object or should they be miniature builders? Consider EmbedBuilder#author. Currently it’s a raw object. Should it be brought in line with the other two builders and nest it as a builder?

My opinion on those 3 questions are:

1. The class fields should represent as closely what the toJSON will return. Calling { ...builder } should return the same valid data as calling builder.toJSON() Well I changed my mind, we should instead use the data field, however no getters should be added pointing to said field
2. ActionRowComponent should use the same interface as application command builders (one add* per component type)
3. EmbedBuilder should get nested builders for such fields (with fallbacks allowed for objects for users if it’s gonna be such an issue).

---

2. The validation

Outside of application command builders, every other builder has 2 classes inside. An “Unsafe” class (which has 0 input validation) and a “Safe” class (which has validation for inputs (and quite fast validation at that)).

From my perspective, this split needs to be removed. The point of builders is to provide a consistent interface for creating api data. Creating two classes that achieve the same thing, even if one of them just extends the other, feels wrong and redundant. All builders should use validation by default¹. This would clean up a lot of the classes, and also remove the issue that other modules using builders need to export both safe and unsafe versions (the latter of which might scare users away).

¹: Validation should instead be able to be turned off at the validation library level (something that we have planned in shapeshift actually (work feedback that we agree would be a good addition)). That way, you can decide in your code if the validation should be turned on or off (for instance turn off validation automatically if in a production environment)

---

3. The pain points

This point is more less an open page for you to air out what frustrations you’ve encountered with the interface. For this, I’d prefer if you sticked to issues related more to consistency or type issues or performance worries (etc), and not the fact the interface has/will change again. The builders package is still not on version 1, meaning we’re still more-less in development to a degree.

Feel free to add anything that you think would improve the overall experience for everyone.

[Qjuh]

Here are my five cent:

1. 1. data property, keep it private, only get it through toJSON()

   2. I‘m not an expert in TS typings but I guess it would be way easier to have TS tell me that an ActionRow is full if the add-methods were typed? So reduce user error that way? As in Omit the addSelectMenu method once any adder was called and Omit addButton once the row is full

   3. I‘m indifferent here, but would tend to builders all the way down, again to reduce user error (so many ways to mistype iconURL apparently)

2. completely agree, get rid of double type, add a global switch for validation.

3. none yet

[monbrey]

I don't use builders, so I'm going to try to be objective as to my thoughts on design but I can also touch a little on why I never adopted them.

Interface

I'm a big fan of the add* style convention on the application command builders - it provides a clearly readable interface and good type safety.

I feel partly responsible for the design of the components builders. When I did buttons and select menus in discord.js originally (pre-builders) I based them on the design of the MessageEmbed class. It seemed logical to keep consistency with the other common type of "message addition" in the library. I don't think that was an inherently incorrect decision in context, but it clearly had an undesirable impact later.

When these components, and Embeds too for that matter, were moved to builders is where the above precedent introduced a problem. The old builder-class style does not align with the add* functional style of application command builders, but by this point library/organisation development was being approached with a primary driver of "modularise discord.js" rather than rewriting or redesigning these specific features to align with new paradigms, such as the one introduced by command builders. In my opinion, this was the mistake, and continued to be an ongoing mistake in approach from the community as PR after PR attempted to modify these builders to match exactly what was taken out of discord.js. (No, the community is not to blame here, we/maintainers lead development direction and allowed this to happen).

But all of this is largely driven by a desire for a particular style rather than a consideration of the actual requirements of these classes.

Somewhat related, its also worth noting that discord.js still maintains entirely separate ApplicationCommand and ApplicationCommandManager classes, also contradictory to the model of components and embeds where a builder is extended. This was corrected later I believe with immutable class structures for received messages.

So to address the questions, I'm first going to ask a couple of my own:

Does there need to be consistency of language and style (e.g. casing of properties, exposing properties, add* methods) across the entire organisation, or not? Once you can decide something like this, the rest is a little better informed.

Why did Components and Embeds get moved to builders at all? Like it or not, building a component or embed is not at all the same use-case as building an application command. There's a vastly different set of requirements to be considered.

1. How should the classes store the data that will be returned from toJSON() calls? In a data field or as the class fields themselves?

I'm somewhat undecided on this. As much as I like the idea of a seamless flow where you just build and produce JSON, I can't pretend that there aren't use-cases where you'd want to know what has currently been set inside the builder. Having to call toJSON() every time to read one of these properties, because they aren't exposed, isn't a particularly appealing interface.

On the other hand, exposing these properties, be it on the class or the data object, exposes them as snake_case. Personally, I see this as correct, but it's a known contentious point that points back to the consistency of language question.

2. Should addComponents be removed in favor of addButton, addSelectMenu and so on? Or should application command builders use addOption instead?

I would agree that addComponent (singular) should be replaced with these type-specific methods, but what does this mean for the set* and add*s (plural) methods? Removing the ability to add multiple of an item at once would be a step backwards in my opinion, and is the sort of limiting factor that makes me prefer writing my own JSON to having a builder construct it.

3. Should nested objects in any builder be a raw object or should they be miniature builders? Consider EmbedBuilder#author. Currently it’s a raw object. Should it be brought in line with the other two builders and nest it as a builder?

My first thought hated this, but surprisingly I changed very quickly. For a consistent interface I think this would be great, but I think I'll come back to some concerns when I write pain points.

Validation

Not a lot to say here but yes, this would be a huge clean-up. What would the toggle switch look like, how would a use disable validation?

Pain points

As I write these, I feel like I may contradict some of the stuff above, but here we go...

I completely understand the need for the builders interface to have a clear, consistent way of doing things, but this RFC seems to focus mainly on adding things to an empty shell. This makes complete sense when approaching it from the design of application command builders - it's extremely unlikely that anyone is trying to do anything dynamic and mutable with these at runtime.

Application command builders were a truly stand-alone builder, designed for /rest, which meant snake_case support.

That doesn't ring true for embeds and components though - consider the use-case of disabling a button on click, or removing/splicing some fields from your embed. I don't see these use-cases being considered in an addField(field => style interface. Failing to provide this functionality would force me to continue to prefer JSON over builders.

Component and embed builders were a hybrid model, designed to be a module of discord.js rather than stand-alone, but tried to meet some builders styles anyway. They were then patched, then extended and re-exported, and continue to be stuck in limbo (hence the need for this very RFC) because this separation was either based on incorrect designs, or simply should never have happened at all.

You don't build components/embeds at the same time, for the same reason, or in the same way as you build application commands. Nobody (or at least, very few people) want to use components or embeds with the raw API, unlike the approach we recommend for deploy-once commands.

The actual use-case needs to come back to the surface to inform these decisions based on what users actually need, not how we want it to look.

[vladfrangu]

Why did Components and Embeds get moved to builders at all? Like it or not, building a component or embed is not at all the same use-case as building an application command. There's a vastly different set of requirements to be considered.

builders isn't necessarily meant for do-it-once-and-never-touch-it-again payloads. It's meant to also help reduce API errors you could encounter by passing in invalid/too long data (think an embed description that's too long) by validating the input and helping build out some more complicated payloads (looking at you action rows)

---

I would agree that addComponent (singular) should be replaced with these type-specific methods, but what does this mean for the set* and add*s (plural) methods? Removing the ability to add multiple of an item at once would be a step backwards in my opinion, and is the sort of limiting factor that makes me prefer writing my own JSON to having a builder construct it.

What's the use case for calling addComponents([new Button(), new Button()]) vs just calling .addButton().addButton()? Is the former much better than the latter, that it would be a groundbreaker for using builders? (things like addFields still don't vibe with me personally, but I doubt it'll go away with this RFC)

As for setComponents or anything similar... Is there an actual use case for this method? When would you add components only to then set the entire action row again?

---

What would the toggle switch look like, how would a use disable validation?

Once sapphiredev/shapeshift#125 is merged and released (🤞 this weekend), we can export methods from builders to disable just the validators we use (probably enableValidators()/disableValidators() or similar interface).

---

That doesn't ring true for embeds and components though - consider the use-case of disabling a button on click, or removing/splicing some fields from your embed. I don't see these use-cases being considered in an addField(field => style interface. Failing to provide this functionality would force me to continue to prefer JSON over builders.

Such use cases can be replicated with code like ButtonComponent.from(theData).setDisabled() and similar, and then using that in your library. But at the core, builders doesn't have to necessarily approach implementation from discord.js's perspective but from a more standalone one, just like discord-api-types does.

[suneettipirneni]

Addressing the recent comment:

What's the use case for calling addComponents([new Button(), new Button()]) vs just calling .addButton().addButton()? Is the former much better than the latter, that it would be a groundbreaker for using builders?

In my opinion, addComponents should stay. Why? Because it's just representing an array of ComponentBuilderss. You can begin to see the issue just by looking at the source code for application builders:

	public addBooleanOption(
		input: SlashCommandBooleanOption | ((builder: SlashCommandBooleanOption) => SlashCommandBooleanOption),
	) {
		return this._sharedAddOptionMethod(input, SlashCommandBooleanOption);
	}

	public addUserOption(input: SlashCommandUserOption | ((builder: SlashCommandUserOption) => SlashCommandUserOption)) {
		return this._sharedAddOptionMethod(input, SlashCommandUserOption);
	}

	public addChannelOption(
		input: SlashCommandChannelOption | ((builder: SlashCommandChannelOption) => SlashCommandChannelOption),
	) {
		return this._sharedAddOptionMethod(input, SlashCommandChannelOption);
	}

All of these methods have the exact same implementation and same amount of arguments, they only differ in name and type signatures. This begs the question, why not just make one generic unified method instead of duplicating code? Is this current approach more type safe? As far as I can tell no, is it easier to use? Well actually it seems harder to use because it works in less use cases:

type Shape = Triangle | Square | Circle

class Box {
  private shapes: Shape[];

  public addTriangle(...) { ... }
  public addSquare(...) { ... }
  public addCircle(...) { ... }
}

As I mentioned above, all of these methods would have the same exact implementations. The real issue for users comes in when they have shapes stored under the polymorphic Shape type:

function generateShapes(): Shape[] { ... }

const box = new Box();
const shapes = generateShapes();

shapes.map(shape => {
	if (shape.type === 'circle') {
		box.addCircle(shape);
	} else if (shape.type === 'square') {
		box.addSquare(shape);
	} else {
		box.addTriangle(shape);
	}
});

Yikes, that's a lot of code for something that should relatively simple. To make matters worse I'll have to continue to update my own code when a new shape gets added because I have to check exhaustively. The core issue here is that the add* approach doesn't take advantage of polymorphic types and makes the user. Let's look at how the same result above would be achieve with a unified addShapes method.

function generateShapes(): Shape[] { ... }

const box = new Box();
const shapes = generateShapes();

box.addShapes(shapes);

As for concrete use cases, anywhere where ComponentBuilder[] (or just Component) is returned or cached makes the proposed system much harder to use than the current system. The current system also offers the maintainers the benefit of not having to duplicate the same implementation code for any new component discord adds (the same idea would apply to command options).

So I end with my main question: "Why is this considered better?"

[kyranet]

Why did Components and Embeds get moved to builders at all? Like it or not, building a component or embed is not at all the same use-case as building an application command. There's a vastly different set of requirements to be considered.

builders isn't necessarily meant for do-it-once-and-never-touch-it-again payloads. It's meant to also help reduce API errors you could encounter by passing in invalid/too long data (think an embed description that's too long) by validating the input and helping build out some more complicated payloads (looking at you action rows)

I would like to also add that it being in a separate package allows developers to re-use the builders in other projects, that don't necessarily need discord.js (think of HTTP bot frameworks, or even other libraries).

The validation is also pretty much a vital part of a HTTP bot's lifecycle, specially when they're used in the HTTP response and not from a subsequent edit, as unlike HTTP calls, we don't get the error back if we send an invalid payload as a reply. An error from builders is a lot more helpful and easier to catch than a silent one from a HTTP call.

Furthermore, it also plays a vital role in developer experience in other places, such as testing if all commands can be registered just fine without doing a HTTP request or even connecting to Discord, which is something very needed for bots that accept translations made by community users. Is a script allowed? Is a specific character in a language supported by Discord's RegExp? Not needing to do a deploy to figure out is very helpful in that regard, and at least we can also know that the data is correctly formed with very simple logic, depending on the framework.

---

I also agree with @suneettipirneni's points, that pattern is very similar to what I do with message components.

[vladfrangu]

While I understand your concerns @suneettipirneni, you need to consider the fact that, at least in the action rows cases, there is no benefit in having an .addComponents method when the row can only have 5 buttons, 1 select menu or 1 text input at this time.

Not to mention that from a users perspective, doing

import { ActionRowBuilder } from 'module-name';

const row = new ActionRowBuilder().addButton(button => button.setCustomId().soOn());

versus

import { ActionRowBuilder, ButtonBuilder } from 'module-name';

const row = new ActionRowBuilder().addComponents([
	new ButtonBuilder().setCustomId().soOn(),
]);

would make it more consistent, especially if they're also working with application commands (which they most likely are).

Another thing to consider is that an action row will never be polymorphic like in your example. The components inside it will always (at this time) be of one type only, which means we can also make validation of attempts to add different types of components in row much simpler and cleaner (and possibly faster too).

Also, maybe a personal hot take, but it's much better to be explicit about implementation than implicit / expected to work with new type.

[monbrey]

builders isn't necessarily meant for do-it-once-and-never-touch-it-again payloads

As for setComponents or anything similar... Is there an actual use case for this method? When would you add components only to then set the entire action row again?

I feel like these points contradict each other and ignore a lot of what I said in the first comment. The attitude towards component builders continues to be focused on building them for the first time, with no consideration for editing.

ButtonComponent.from(theData).setDisabled() is not sufficient. How do you then replace that one button in what could be up to 5x5 rows?

It should not be necessary to clone 5 action rows / 25 buttons using Component.from just to edit one nested item.

Maybe you wouldn't set all 5 - this is a better argument for spliceComponents, but still. Consider the editing usecase.

[vladfrangu]

The attitude towards component builders continues to be focused on building them for the first time, with no consideration for editing.

And that's correct? I mean I don't see the issue in that, it shouldn't be up to builders to aid in editing a components, that's up to you when using them. Of course, the alternative is calling ActionRowBuilder.from(row), then accessing the components of the row and editing that one button you're looking for, and then passing that down, but again, that falls outside the scope of the module. At least that's how I've envisioned builders but I'll wait for the others maintainers' thoughts.

[imranbarbhuiya]

the 2nd point is now resolved. ref: #8074

[monbrey]

It shouldn't be up to builders to aid in editing a components, that's up to you when using them

If that's a design decision to be made for component builders, then so be it, they can only build.
However it should be acknowledged that this greatly undercuts their usefulness and ignores user stories that I personally would consider must-haves were this a product being designed based on the requirements of actual developers,