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.

Sign up for GitHub

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

Jump to bottom

refactor(attachment): don't return attachment builders from API #7852

Merged
merged 8 commits into from
Jun 4, 2022
Merged

refactor(attachment): don't return attachment builders from API #7852

merged 8 commits into from
Jun 4, 2022

Conversation

suneettipirneni
Copy link
Member

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

This PR creates a seperate structure for attachments returned by the API. The builder structure has been renamed to AttachmentBuilder

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)

@suneettipirneni suneettipirneni force-pushed the refactor/immutable-attachments branch 3 times, most recently from 6fb2d85 to a3592fb Compare April 26, 2022 21:55
@imranbarbhuiya
Copy link
Contributor

There is already a similar pr #7813

@monbrey
Copy link
Member

monbrey commented Apr 26, 2022

There is already a similar pr #7813

This one is much more consistent with the current approach to builders and internally constructed classes.

@ImRodry
Copy link
Contributor

ImRodry commented Apr 26, 2022

Should probably fix that then instead of completely ignoring the other guy's work and opening something on top of it...

@suneettipirneni
Copy link
Member Author

There is already a similar pr #7813

These PR's have some overlap but are mostly two different approaches to this. This doesn't modify /builders code while the other does, this only makes changes to discord.js code. And in addition, makes the API only return immutable attachment objects.

@ImRodry
Copy link
Contributor

ImRodry commented Apr 26, 2022 •
edited
Loading

So now we’re creating a builder in djs and not in the package that is literally called builders. Great! Inconsistencies!

@iCrawl iCrawl added this to the discord.js v14 milestone Apr 28, 2022
@vladfrangu
Copy link
Member

So now we’re creating a builder in djs and not in the package that is literally called builders. Great! Inconsistencies!

While I can understand why you'd think that, unlike other builders (components, embeds, commands), attachments are heavily implementation based and cannot be easily made agnostic due to their nature ( what would we be supporting? a Buffer/Blob? raw Uint8Arrays/ArrayBuffers? reading from the file system? all of the above?) and I for one don't see how we could create an agnostic builder. (btw, right now builders work on web too if you ever thought of using it, I'm sure someone has a use case for that 😄)

Hope that clarifies it. If not, you can ask me on Discord.

SpaceEEC
SpaceEEC requested changes May 20, 2022
View reviewed changes
packages/discord.js/typings/index.d.ts Outdated Show resolved Hide resolved
packages/discord.js/src/structures/Attachment.js Outdated Show resolved Hide resolved
kyranet
kyranet reviewed May 22, 2022
View reviewed changes
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.

Actually, I noticed this bug. LGTM otherwise.

packages/discord.js/src/structures/AttachmentBuilder.js Outdated Show resolved Hide resolved
@suneettipirneni @kyranet
Update packages/discord.js/src/structures/AttachmentBuilder.js
0822739 
Co-authored-by: A. Román <kyradiscord@gmail.com>
SpaceEEC
SpaceEEC requested changes May 30, 2022
View reviewed changes
packages/discord.js/src/managers/GuildStickerManager.js Outdated Show resolved Hide resolved
packages/discord.js/src/structures/Message.js Outdated Show resolved Hide resolved
packages/discord.js/src/structures/Message.js Outdated Show resolved Hide resolved
packages/discord.js/src/structures/MessagePayload.js Outdated Show resolved Hide resolved
packages/discord.js/src/structures/Webhook.js Outdated Show resolved Hide resolved
@suneettipirneni @SpaceEEC
Apply suggestions from code review
50af16b 
Co-authored-by: SpaceEEC <spaceeec@yahoo.com>
@suneettipirneni suneettipirneni force-pushed the refactor/immutable-attachments branch 2 times, most recently from 87bf558 to d9a58f9 Compare June 4, 2022 12:28
@suneettipirneni @SpaceEEC
Update packages/discord.js/src/managers/GuildStickerManager.js
1b0c6fa 
Co-authored-by: SpaceEEC <spaceeec@yahoo.com>
@iCrawl iCrawl merged commit dfadcbc into discordjs:main Jun 4, 2022
@suneettipirneni suneettipirneni deleted the refactor/immutable-attachments branch June 4, 2022 22:08
suneettipirneni added a commit to suneettipirneni/discord.js that referenced this pull request Jun 4, 2022
@suneettipirneni @almeidx
@kyranet @SpaceEEC
refactor(attachment): don't return attachment builders from API (disc…
4193340 
…ordjs#7852)

Co-authored-by: Almeida <almeidx@pm.me>
Co-authored-by: A. Román <kyradiscord@gmail.com>
Co-authored-by: SpaceEEC <spaceeec@yahoo.com>
@almeidx almeidx mentioned this pull request Jul 19, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@almeidx almeidx almeidx left review comments

@kyranet kyranet kyranet approved these changes

@SpaceEEC SpaceEEC SpaceEEC approved these changes

@iCrawl iCrawl iCrawl approved these changes

@vladfrangu vladfrangu Awaiting requested review from vladfrangu

Assignees
No one assigned
Labels
packages:discord.js refactor semver:major
Projects
discord.js
Archived in project
Milestone
discord.js v14
Development

Successfully merging this pull request may close these issues.
9 participants
@suneettipirneni @imranbarbhuiya @monbrey @ImRodry @vladfrangu @iCrawl @kyranet @SpaceEEC @almeidx