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

feat(*): document and support embeds field in message create endpoint #5792

Merged
merged 14 commits into from
Jun 11, 2021
Merged

feat(*): document and support embeds field in message create endpoint #5792

merged 14 commits into from
Jun 11, 2021

Conversation

castdrian
Copy link

@castdrian castdrian commented Jun 9, 2021 •
edited
Loading

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

The message create endpoint now supports up to 10 embeds just like the webhook endpoint, these changes reflect that addition within the typings and APIMessage#create function.

Status and versioning classification:

  • This PR changes the library's interface (methods or parameters added)
  • 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 includes breaking changes (methods removed or renamed, parameters moved or removed)

@castdrian castdrian marked this pull request as draft June 10, 2021 00:54
@ckohen
Copy link
Member

ckohen commented Jun 10, 2021

Upstream has been merged 🚀 changes are live.

@kyranet kyranet self-requested a review June 10, 2021 05:50
kyranet
kyranet reviewed Jun 10, 2021
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.

Since Discord seems to plan the removal of the embed field (in favour of embeds), should we refactor the code and account for less overloads? I see this as a great opportunity to remove some conditional checks made for webhooks (and interactions).

As for the embed option, should we keep it as an alias for embeds: [embed]? Since { embed } is much nicer and shorter to write (and we already have an alias for content, so why not?).

GoldenAngel2
GoldenAngel2 suggested changes Jun 10, 2021
View reviewed changes
Copy link
Contributor

@GoldenAngel2 GoldenAngel2 left a comment

Choose a reason for hiding this comment

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

src/structures/APIMessage.js#L147 needs to be updated aswell, so this works properly.

@castdrian
Copy link
Author

src/structures/APIMessage.js#L147 needs to be updated aswell, so this works properly.

I am aware, please refer to the initial comment in italics, I somewhat mistimed opening the PR yesterday

@castdrian castdrian changed the title feat(MessageOptions): document embeds field in typings feat(*): document and support embeds field message create endpoint Jun 10, 2021
@castdrian castdrian changed the title feat(*): document and support embeds field message create endpoint feat(*): document and support embeds field in message create endpoint Jun 10, 2021
@castdrian castdrian marked this pull request as ready for review June 10, 2021 11:52
@castdrian
Copy link
Author

Since Discord seems to plan the removal of the embed field (in favour of embeds), should we refactor the code and account for less overloads? I see this as a great opportunity to remove some conditional checks made for webhooks (and interactions).

As for the embed option, should we keep it as an alias for embeds: [embed]? Since { embed } is much nicer and shorter to write (and we already have an alias for content, so why not?).

Changes adressed, tested & rebased :)

@csuvajit
Copy link

csuvajit commented Jun 10, 2021 •
edited
Loading

src/structures/APIMessage.js#L206 needs some changes here.

ckohen
ckohen requested changes Jun 10, 2021
View reviewed changes
Copy link
Member

@ckohen ckohen left a comment

Choose a reason for hiding this comment

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

discord.js/src/structures/interfaces/TextBasedChannel.js

Line 73 in 9e46178

* @property {MessageEmbed|Object} [embed] An embed for the message
needs updating.

typings/index.d.ts Outdated Show resolved Hide resolved
@castdrian
Copy link
Author

TextBasedChannel.js#L73 needs updating.

Done, thank you

adrifcastr added 2 commits June 12, 2021 00:02
Revert "commit suggested changes"
f819f38 
fix(TextChannel): JSDoc doesn't seem to like this, revert
fix(ApiMessage): fix eslint errs
46383da
@iCrawl iCrawl merged commit 99ff715 into discordjs:master Jun 11, 2021
@vaporoxx vaporoxx mentioned this pull request Jun 12, 2021
Merged
@iCrawl iCrawl added this to the Version 13 milestone Jun 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vaporoxx vaporoxx vaporoxx left review comments

@ToasterSticks ToasterSticks ToasterSticks left review comments

@kyranet kyranet kyranet approved these changes

@ckohen ckohen ckohen requested changes

@monbrey monbrey monbrey left review comments

@GoldenAngel2 GoldenAngel2 GoldenAngel2 requested changes

@SpaceEEC SpaceEEC SpaceEEC approved these changes

@iCrawl iCrawl iCrawl approved these changes

@vladfrangu vladfrangu vladfrangu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Version 13
Development

Successfully merging this pull request may close these issues.