-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
refactor: switch to /builders Embed
#7067
Conversation
Why is this no longer called MessageEmbed? Are there other kinds of Embeds that don't go in messages? |
Because the class from /builders is called Embed. |
Shouldn't it be called MessageEmbed though? It could always be imported as MessageEmbed here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming between MessageEmbed and Embed is ehh, gonna take a while to get used to, but LGTM
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? |
-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]) |
This would also remove the ability to use
|
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 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 |
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. |
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. 😆 |
I'm leaning towards not going forward with this change, because |
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 |
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 |
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 |
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. |
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. |
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). |
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. |
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.
Must have been a while ago, I started using the lib when RichEmbed was still a thing 😄 |
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. 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. |
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. |
2 small things 👀
|
40bd6ab
to
96a7477
Compare
8a31af5
to
fa4d85c
Compare
This needs a rebase. |
1 similar comment
This needs a rebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
fa4d85c
to
25d2a43
Compare
Please describe the changes this PR makes and why it should be merged:
Removes
MessageEmbed
and replaces it with@discordjs/builders
'sEmbed
, 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: