-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
refactor!: remove AttachmentBuilder and support new file body encodables #11278
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
|
Ready for review, but blocked until the other PR is merged. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11278 +/- ##
=======================================
Coverage 44.97% 44.97%
=======================================
Files 317 317
Lines 18222 18222
Branches 1817 1817
=======================================
Hits 8196 8196
Misses 10013 10013
Partials 13 13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5fc03b1 to
2207fa3
Compare
Co-authored-by: Jiralite <[email protected]>
| * @property {MessageMentionOptions} [allowedMentions] Which mentions should be parsed from the message content | ||
| * (see {@link https://discord.com/developers/docs/resources/message#allowed-mentions-object here} for more details) | ||
| * @property {Array<(AttachmentBuilder|Attachment|AttachmentPayload|BufferResolvable)>} [files] | ||
| * @property {Array<(Attachment|AttachmentPayload|BufferResolvable)>} [files] |
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.
I'd prefer if there was still a way to pass an AttachmentBuilder into here, albeit not sure what type would make sense to put for that here.
Another helper type like RawFileEncodable which extends JSONEncodable with the additional getRawFile() method? Seems clunky but 🤷♂️
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 main issue I have with this is that AttachmentBuilder includes data tied to.. well, the attachments array.
What do we do when the user sets that data and they also provide attachments manually?
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 same we did before:
discord.js/packages/discord.js/src/structures/MessagePayload.js
Lines 193 to 204 in 02fc101
| const attachments = this.options.files?.map((file, index) => ({ | |
| id: index.toString(), | |
| description: file.description, | |
| title: file.title, | |
| waveform: file.waveform, | |
| duration_secs: file.duration, | |
| })); | |
| if (Array.isArray(this.options.attachments)) { | |
| this.options.attachments.push(...(attachments ?? [])); | |
| } else { | |
| this.options.attachments = attachments; | |
| } |
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.
That is absolutely awful in terms of library behavior. That snippet effectively means that if you pass both files and attachments at the same time, it's a guaranteed API error, unless it's a message edit and the elements the user passed to the attachments array specifically refer to existing attachments.
In essence, this renders the attachments field as edit-only, we're just completely taking over it with this behavior, largely hiding how the API payload works too (which I am not fond of at all, no matter how rich the the abstraction).
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.
But that's what AttachmentBuilder does too... it hides the same things the same way.
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.
No. This isn't about hiding things, it's allowing the user to easily pass garbage. AttachmentBuilder on its own is fundamentally different from what you want, it removes your granular control of the files/attachments fields, it ties them together completely. There is no way when using it to get non-sense miss-matched arrays. All the more so when you use MessageBuilder
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.
Circling back on this, the only way I'd accept it is making it a union type that forbids files if attachments is AttachmentBuilder[], I'm not compromising otherwise
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.
Thing is that all the other types allowed for files here will also fill the attachments. So if anything it would make sense to completely disallow attachments in create Message options.
And you got it backwards: it's an accepted type for the files property and will disallow the attachments property.
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.
Sure. I'm okay with designing the API that way, but we do also have to acknowledge the complexity it adds
Depends on #11248
Closes #11273