Skip to content
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

Merged

Conversation

suneettipirneni
Copy link
Member

@suneettipirneni suneettipirneni commented Mar 1, 2022

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:

  • Builders select menu component's customId is optional when it's always non-null from the API
  • Button style is always required while it's optional in builders.
  • Select Menu options are optional in builders while always non-null from the API
  • Modal Submit Interactions don't return the full set of fields that a builder uses, which means you have a huge discrepancy between available fields and non-available fields.

Because 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:

const modified = EmbedBuilder.from(receivedEmbed)
  .setTitle('foo');

interaction.reply({ embeds: [modified] })

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 or MessageEmbed? ", most people probably wouldn't know. This is because the names don't really indicate the differences between these embeds. Rich over Message 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 and MessageEmbed 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:

// RichEmbed, MessageEmbed
const { title } = richEmbed; // OK
const { title } = messageEmbed // OK

// Embed, EmbedBuilder
const { title } = embed; // OK
const { title } = embedBuilder; // Error - property title doesn't exist on `EmbedBuilder`

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • 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)

Copy link
Member

@kyranet kyranet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

packages/discord.js/src/structures/ButtonComponent.js Outdated Show resolved Hide resolved
packages/discord.js/typings/index.d.ts Show resolved Hide resolved
packages/discord.js/typings/index.d.ts Outdated Show resolved Hide resolved
@suneettipirneni suneettipirneni force-pushed the discord.js/refactor/api-return-structures branch from 611805a to bc94d8d Compare March 2, 2022 15:28
@iCrawl iCrawl added this to the discord.js v14 milestone Mar 4, 2022
@suneettipirneni suneettipirneni force-pushed the discord.js/refactor/api-return-structures branch 4 times, most recently from dba4402 to 2d9d1d5 Compare March 6, 2022 02:36
Copy link
Member

@SpaceEEC SpaceEEC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise looking good

@iCrawl
Copy link
Member

iCrawl commented Mar 6, 2022

This needs a rebase.

@suneettipirneni suneettipirneni force-pushed the discord.js/refactor/api-return-structures branch 5 times, most recently from a74597f to cbd057a Compare March 10, 2022 03:13
@vladfrangu
Copy link
Member

In Crawl's words, this needs a rebase

@suneettipirneni suneettipirneni force-pushed the discord.js/refactor/api-return-structures branch from 987e2d9 to a3ebe36 Compare March 12, 2022 01:06
Copy link
Member

@SpaceEEC SpaceEEC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise probably fine

packages/discord.js/src/structures/Message.js Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

10 participants