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: make public builder props getters #7422

Merged
merged 18 commits into from
Feb 13, 2022

Conversation

suneettipirneni
Copy link
Member

This is a replacement for the previous pr since gh is having trouble loading the diffs.

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

Stores raw api data in a data object and allows access to builder properties via getters.

Rationale

There's quite a few benefits in using getters for builders:

  • Currently builders relies on Reflect#set which is much slower than direct prop assignment, in one benchmark I did it was waaay slower than direct assignment. Beyond performance Reflect#set is less typesafe since property keys are strings and it allows values to be set as any. This PR removes the usage of these methods, and preserves the readonly nature of the fields.

  • The classes are now more flexible. For example, right now if we decided to add a field to any builder class, it would be outputted in toJSON simply because toJSON just spreads this: { ...this }. In addition, since we now have getters we can do any computation before a value is returned unlike a regular field.

  • Constructor assignment is easy, in some cases it's just assigning this.data = data. toJSON is straightforward as well, as it's essentially just returning a copy of this.data.

  • Allows for idiomatic js/ts class field naming. The props are simply retrieved from this.data so they're not constrained to any naming conventions used by the API.

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)

packages/builders/src/components/ActionRow.ts Outdated Show resolved Hide resolved
packages/builders/src/components/ActionRow.ts Outdated Show resolved Hide resolved
packages/builders/src/messages/embed/Embed.ts Outdated Show resolved Hide resolved
packages/builders/src/messages/embed/UnsafeEmbed.ts Outdated Show resolved Hide resolved
packages/builders/src/messages/embed/UnsafeEmbed.ts Outdated Show resolved Hide resolved
packages/builders/src/messages/embed/UnsafeEmbed.ts Outdated Show resolved Hide resolved
packages/builders/src/components/Component.ts Outdated Show resolved Hide resolved
packages/builders/src/components/button/UnsafeButton.ts Outdated Show resolved Hide resolved
packages/builders/src/components/button/UnsafeButton.ts Outdated Show resolved Hide resolved
packages/builders/src/components/button/UnsafeButton.ts Outdated Show resolved Hide resolved
packages/builders/src/messages/embed/UnsafeEmbed.ts Outdated Show resolved Hide resolved
packages/builders/src/messages/embed/UnsafeEmbed.ts Outdated Show resolved Hide resolved
packages/builders/src/messages/embed/UnsafeEmbed.ts Outdated Show resolved Hide resolved
packages/builders/src/messages/embed/UnsafeEmbed.ts Outdated Show resolved Hide resolved
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.

Just one last thing, then LGTM.

packages/builders/src/components/button/UnsafeButton.ts Outdated Show resolved Hide resolved
packages/builders/__tests__/messages/embed.test.ts Outdated Show resolved Hide resolved
packages/builders/__tests__/components/selectMenu.test.ts Outdated Show resolved Hide resolved
packages/builders/src/components/ActionRow.ts Outdated Show resolved Hide resolved
packages/builders/src/components/ActionRow.ts Outdated Show resolved Hide resolved
packages/builders/src/components/Component.ts Outdated Show resolved Hide resolved
packages/builders/src/components/button/UnsafeButton.ts Outdated Show resolved Hide resolved
packages/builders/src/messages/embed/Assertions.ts Outdated Show resolved Hide resolved
@vladfrangu
Copy link
Member

This needs a rebase

packages/builders/src/components/ActionRow.ts 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.

7 participants