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

types(MessageEditOptions): Omit poll #10509

Merged
merged 2 commits into from
Sep 17, 2024

Conversation

TAEMBO
Copy link
Contributor

@TAEMBO TAEMBO commented Sep 15, 2024

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

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

@TAEMBO TAEMBO requested a review from a team as a code owner September 15, 2024 04:53
Copy link

vercel bot commented Sep 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
discord-js ⬜️ Ignored (Inspect) Visit Preview Sep 17, 2024 8:14am
discord-js-guide ⬜️ Ignored (Inspect) Visit Preview Sep 17, 2024 8:14am

Copy link
Member

@didinele didinele left a comment

Choose a reason for hiding this comment

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

LGTM but just double checking, there isn't any runtime behavior here that's also incorrect, right?

@Qjuh
Copy link
Contributor

Qjuh commented Sep 15, 2024

LGTM but just double checking, there isn't any runtime behavior here that's also incorrect, right?

We pass any payload data you pass into Message#edit() or Webhook#editMessage() to the API as if it was a full MessagePayload. And the API ignores those or errors if they are wrong. We don’t do runtime checks on it (at least none that differ between creating a message and editing one).

@didinele
Copy link
Member

I'm fine with that.

@Jiralite Jiralite changed the title fix: creating poll from message edit types(MessageEditOptions): Omit poll Sep 15, 2024
Copy link
Member

@Jiralite Jiralite left a comment

Choose a reason for hiding this comment

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

What about the JSDoc? The website for parsing it is still around:
https://old.discordjs.dev/#/docs/discord.js/main/typedef/MessageEditOptions

@TAEMBO
Copy link
Contributor Author

TAEMBO commented Sep 15, 2024

What about the JSDoc? The website for parsing it is still around: https://old.discordjs.dev/#/docs/discord.js/main/typedef/MessageEditOptions

Currently the JSDoc definition for MessageEditOptions inherits from BaseMessageOptions which is where the poll option is defined. As of now I don't see a way to inherit from BaseMessageOptions while omitting the poll option in JSDoc, and the only solution I can think of thus far of manually mirroring all options except for the poll option from BaseMessageOptions into MessageEditOptions doesn't seem very ideal due to duplication of each option which then needs to be taken into account each time a new BaseMessageOptions option is introduced. Let me know if there's a better solution for this or if the one I mentioned will suffice.

@Jiralite
Copy link
Member

Base message options is supposed to be exactly what it is called: the base message options. If poll is not useable in all scenarios, it should not be there.

It's not ideal, but there's no other way around it I see because there is no way to omit in JSDoc—only extend.

@TAEMBO
Copy link
Contributor Author

TAEMBO commented Sep 15, 2024

Gotcha, sounds good. I'll remove it and instead add poll into the individual places it's applicable in. Cheers.

@almeidx
Copy link
Member

almeidx commented Sep 15, 2024

It's not ideal, but there's no other way around it I see because there is no way to omit in JSDoc—only extend.

We could also create another typedef like BaseMessageOptionsWithPoll (which extends BaseMessage) or something along those lines, instead of duplicating the poll option definition everywhere

@Jiralite Jiralite added this to the discord.js 14.16.3 milestone Sep 15, 2024
@kodiakhq kodiakhq bot merged commit 665bf14 into discordjs:main Sep 17, 2024
7 checks passed
@TAEMBO TAEMBO deleted the fix/edit-options-with-poll branch September 17, 2024 22:40
monbrey pushed a commit to monbrey/discord.js that referenced this pull request Sep 18, 2024
fix: creating poll from message edit

Co-authored-by: Jiralite <[email protected]>
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.

MessageEditOptions incorrectly includes poll
6 participants