-
-
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: Don't return builders from API data #7584
refactor: Don't return builders from API data #7584
Conversation
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.
LGTM
611805a
to
bc94d8d
Compare
dba4402
to
2d9d1d5
Compare
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.
Otherwise looking good
This needs a rebase. |
a74597f
to
cbd057a
Compare
In Crawl's words, this needs a rebase |
Co-authored-by: Antonio Román <kyradiscord@gmail.com>
cbd057a
to
48fdac6
Compare
987e2d9
to
a3ebe36
Compare
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.
Otherwise probably fine
Please describe the changes this PR makes and why it should be merged:
Component Builders currently maintain a responsibility it shouldn't. That responsibility is representing received data from the API. The issue with doing this is that it assumes that received data and in-progress build data is always the same type. However, in many places this isn't the case:
customId
is optional when it's always non-null from the APIstyle
is always required while it's optional in builders.options
are optional in builders while always non-null from the APIBecause of this, builders are now more aligned to their original goal: build data, don't represent it
Relieving responsibility for builders:
Most noticeably, builders now have a
Builder
suffix. This makes it extremely clear that the class is purely a builder, nothing more and nothing less.For example
ButtonComponent
->ButtonBuilder
Removal of getters from builders
Keep in mind that builders are meant to be used in places beyond just discord.js. In situations outside of discord.js getters on builders hold little-to-no utility. The main reason as to why is because if you're constructing a structure you already have all the data needed to construct it. If you already have that data in the first place there's no need to retrieve it from a getter.
The main use case of getters on builders was to get data when received from discord.js. However, with this PR removing the responsibility of builders to represent structures, the getters are no longer needed.
Instead, getters have been relocated to their respective finalized structures.
Finalized Structures
ButtonComponent
,SelectMenuComponent
,Embed
etc. are now structures within discord.js fully responsible for representing data received from the API. This allows us to always have API-accurate representations of the data. In addition, it prevents mutation side-effects by making the structures immutable.For example, previously a user could call a
setX
method on a returned API structure. While on the surface it seems harmless, it actually modifies the cache. The cache should never be modified by anything externally, if it is modified it's no longer a valid cache since the data is out of sync.How would I modify a received structure, if I want to make a small change?
Like this:
This preserves immutability while also allowing you to use preexisting component data.
Is this RichEmbed vs MessageEmbed all over again?
tdlr; no
Firstly, if I were to ask someone: "Which class is a builder?
RichEmbed
orMessageEmbed
? ", most people probably wouldn't know. This is because the names don't really indicate the differences between these embeds.Rich
overMessage
doesn't mean anything to me, so figuring out which one to use is difficult.In this PR structures are named clearly as to what they do. "Embed vs EmbedBuilder" is a much clearer distinction. I can clearly tell one of the structures "builds" an embed while the other structure is an embed.
Secondly, both
RichEmbed
andMessageEmbed
were both capable of representing a received embed structure. Whereas in this PR only the finalized structures can represent API responses, not the builders. For example:Status and versioning classification: