refactor: switch to /builders Embed

[kyranet]

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

Removes MessageEmbed and replaces it with @discordjs/builders's Embed, which has a similar interface, and also re-exports it for convenience.

This change has been made to reduce the development cost and keeping embed builders in one place with one design, as opposed to having to support a mix of two.

Also, the somewhat unrelated change in .github/COMMIT_CONVENTION.md is because the MessageEmbed class is gone, so I replaced it with something else. 🪄

Status and versioning classification:

• 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)
• This PR includes breaking changes (methods removed or renamed, parameters moved or removed)

[ImRodry]

Why is this no longer called MessageEmbed? Are there other kinds of Embeds that don't go in messages?

[kyranet]

Because the class from /builders is called Embed.

[ImRodry]

Shouldn't it be called MessageEmbed though? It could always be imported as MessageEmbed here and that would make this a semver minor I believe nvm

[vladfrangu]

The naming between MessageEmbed and Embed is ehh, gonna take a while to get used to, but LGTM

[ImRodry]

I see that the Embed from builders is missing some methods from discord.js's MessageEmbed that can be added but what about #equals? That method is pretty useful but I don't think it would fit within the scope of builders, would it?

[KhafraDev]

-1

This makes creating Embeds much slower, probably due to zod/arg validation. Here's a basic benchmark:

import { createHistogram, performance } from 'perf_hooks';

import { Embed } from '@discordjs/builders';
import { MessageEmbed } from 'discord.js';

const option1 = () => new MessageEmbed()
    .setTitle('The Title')
    .setAuthor({ name: 'Author', url: 'https://www.google.com/' });
const option2 = () => new Embed()
    .setTitle('The Title')
    .setAuthor({ name: 'Author', url: 'https://www.google.com/' });

const h_option1 = createHistogram();
const t_option1 = performance.timerify(option1, { histogram: h_option1 });
  
const h_option2 = createHistogram();
const t_option2 = performance.timerify(option2, { histogram: h_option2 });
  
process.on('exit', () => {
    console.log('option1', h_option1.min, h_option1.max, h_option1.mean, h_option1.stddev)
    console.log('option2', h_option2.min, h_option2.max, h_option2.mean, h_option2.stddev)
});
  
for (let n = 0; n < 1e6; n++) {
    t_option1();
    t_option2();
}

Most - if not all - users do not need argument parsing and can use the package themselves if they want to. I don't know if massively slowing down everyone else is worth it though...

edit: if you want a simpler benchmark it takes 34ms to run the first option 1e6 times and 4.5 seconds to run the second one an equal amount of times (simply using console.[time/timeEnd])

[GoldenAngel2]

This would also remove the ability to use "COLOR_NAME" for setColor using the colors from Constants

Unless I'm overlooking where this is supported in builders.

setFooter has a change that isn't currently deprecated (object only instead of setFooter("text", "image"))
hexColor & createdAt is gone now (both not deprecated)
one that ImRodry mentioned.

[kyranet]

Thank you for bringing all those concerns to us, I have brought this into our internal thread, we were not aware of the performance impact zod had, the previous library, ow, was super-complex but it was also very fast.

As for the missing fields, I might just create a class extending /builders's to implement them (#equals included), or just closing this PR altogether, I noticed footer would have a property named icon_url which is against our naming conventions (but that sadly, cannot be changed because it wouldn't match the API's name).

[KhafraDev]

we were not aware of the performance impact zod had, the previous library, ow, was super-complex but it was also very fast.

When looking at zod, I did notice that there "were" performance issues that were supposedly fixed - but fixed in the sense that it is still immensely slow as the benchmark above showed.

In this issue comment, it takes 76 ms (!!!) to parse one structure. In my own benchmark, it's roughly 220 ms for each Embed (there are 1 - 2 - 3 - 4 assertions for the benchmark I did). Other performance improvements could have lowered that 76ms, or maybe number/string predicates are simply faster.

I think the best course of action would be adjusting the types/methods if needed to accept /builders' Embed but leave MessageEmbed in d.js. Validation, no matter the library, will always degrade performance in some way. After all, most library users do not need argument parsing and there is nothing fundamentally wrong with the current implementation (it's easy to use; fast; and is just that - a wrapper to create an object).

[suneettipirneni]

we were not aware of the performance impact zod had, the previous library, ow, was super-complex but it was also very fast.

When looking at zod, I did notice that there "were" performance issues that were supposedly fixed - but fixed in the sense that it is still immensely slow as the benchmark above showed.

In this issue comment, it takes 76 ms (!!!) to parse one structure. In my own benchmark, it's roughly 220 ms for each Embed (there are 1 - 2 - 3 - 4 assertions for the benchmark I did). Other performance improvements could have lowered that 76ms, or maybe number/string predicates are simply faster.

I think the best course of action would be adjusting the types/methods if needed to accept /builders' Embed but leave MessageEmbed in d.js. Validation, no matter the library, will always degrade performance in some way. After all, most library users do not need argument parsing and there is nothing fundamentally wrong with the current implementation (it's easy to use; fast; and is just that - a wrapper to create an object).

In addition, using json objects to send in is also still a valid option and skips a lot of the validations the builder does, and is well type-checked.

[KhafraDev]

In addition, using json objects to send in is also still a valid option and skips a lot of the validations the builder does, and is well type-checked.

Using plain objects is definitely the best way to go if you absolutely want the best performing option, but I've been using MessageEmbed (and RichEmbed before that) for so many years now it's ingrained into my memory at this point. 😆

[kyranet]

I'm leaning towards not going forward with this change, because Embed stores timestamp as an ISO 8601 string instead a number, which will increase memory usage drastically in the long run. See #6567 (comment).

[vladfrangu]

I disagree with you on that, I think we should go forward with the change, but only after we fix the performance issues in /builders

[ImRodry]

The embed from builders will always be more limited than the discord.js builder since you can't have things like ColorResolvable and, with the amount of checks that builders has, I think it will always be inevitably less performant than whatever implementation we have on the library itself

[vladfrangu]

And? You can always extend the /builders one, implement ColorResolvable support and carry on. And again, builders are meant to validate, it has been said already that if you care for performance, you use the object directly

[KhafraDev]

if you care for performance, you use the object directly

If you're talking about what I said in a previous comment, this isn't really true: I said "absolutely want the best performing option". Of course plain objects are the fastest, but being able to create hundreds of thousands of MessageEmbed objects a second is also performant enough for everyone using discord.js.

Personally I think the best option would be to allow a builders Embed, but keep MessageEmbed. It's already low maintenance, is performant, and provides the amount of validation a majority of users need - next to none. If a dev wants or needs validation, they should be able to use builders' Embed, but if a developer does not want to use validation, it should remain as an option.

edit: fixing performance issues also just isn't possible. Any validation will be substantially slower than the current MessageEmbed class.

[iCrawl]

Take it as you will but I personally think it's incredibly silly to want something like a "builder" of any kind in a language like JavaScript where objects rule the world.

Thats why the approach to have the builders be heavily validated instead of just some fancy way to avoid object literals.

[KhafraDev]

Take it as you will but I personally think it's incredibly silly to want something like a "builder" of any kind in a language like JavaScript where objects rule the world.

That's a strange take because a large portion of this library is a wrapper around object literals received from the api with a few utility methods/getters (which MessageEmbed has as well).

[iCrawl]

I think that's a pretty hyperbolic take since this library does a lot more than just wrap some http calls and json payloads.

Additionally none of those classes technically need to be constructed by the user itself or modified in a significant/free-form way such as the Message Embeds discord allows you to create.

The reason a lot of other languages have builders is because it is incredibly or rather annoyingly hard to create such pure object literals. We didn't even have a builder back in the day, and even then it was met with heavy criticism because of the language we operate in makes constructing objects in such a free-form way incredibly easy.

[KhafraDev]

I think that's a pretty hyperbolic take since this library does a lot more than just wrap some http calls and json payloads.

That was the point I was trying to make: the embed builder does more than just make an object (has utility functions like resolving colors, allows chaining functions, etc). Obviously someone could make their own embed builder, just as someone could use @djs/rest for every api call. The reason people don't do so is because of their ease of use.

I genuinely don't understand what is wrong with the current implementation - as seen in the help channels, tons of people use them and have no issue. Replacing them with an implementation that isn't 1:1 will cause more burden for you guys (the maintainers) than simply leaving it, and adding support for builders' Embed.

We didn't even have a builder back in the day

Must have been a while ago, I started using the lib when RichEmbed was still a thing 😄

[iCrawl]

The biggest problem would stem from supporting and constantly updating it in two places.

I would say we reached a good consensus internally about this by exposing two classes in the builder, one with validation (the default) and one opt-in that does no validation at all.

The reason the validating one is the default is because 99% of the userbase does not need any of those numbers in the benchmarks. Additionally, benchmarks are just that, benchmarks, while good to see where problems lie, a discord bot itself has so many more problematic/moving parts (http, ws, parsing, transforming), that none of this (especially the one million per second) will never really apply here.
For specific users (naming noticeably big bots here that also have caching concerns) they can seamlessly switch to changing the import and get the benefit of the builder but no validation.

When it comes to backporting features, that is still on the table and nothing is set in stone when it comes to that, so eventually we will reach feature parity or even move past that.

[KhafraDev]

I would say we reached a good consensus internally about this by exposing two classes in the builder, one with validation (the default) and one opt-in that does no validation at all.

This is great! My biggest concern was that validation would be done even on embed objects received from the api (which are more or less guaranteed to be validated), which would have slowed everyone's bots down tremendously. Imagine a message with an embed taking 50-75 ms times the number of fields to parse (times number of embeds I guess) even though the data was already validated!

In fact, looking back at it, validating any data received from discord's end doesn't seem to be a great idea. Glad that this won't be a problem.

[vladfrangu]

2 small things 👀

1. This needs a rebase
2. Would like to clarify that the validation only happens on the set*/add* methods, not on the constructor itself, nor on toJSON method (although the latter is tbd as for slash builders we do "validate" the master toJSON too)

[iCrawl]

This needs a rebase.

[iCrawl]

This needs a rebase.

[ImRodry]

Can't we keep the MessageEmbed class just like we had Collection extend the @discordjs/collection for a long time with methods like #equals which probably don't make much sense on /builders? Where would we put those methods otherwise?