types(MessageEditOptions): Omit `poll`

discordjs / discord.js

A powerful JavaScript library for interacting with the Discord API

https://discord.js.org

Apache License 2.0

25.29k stars 3.96k forks source link

types(MessageEditOptions): Omit `poll` #10509

Closed TAEMBO closed 1 week ago

TAEMBO commented 2 weeks ago

Please describe the changes this PR makes and why it should be merged: Fixes https://github.com/discordjs/discord.js/issues/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

vercel[bot] commented 2 weeks ago

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

2 Skipped Deployments

Qjuh commented 2 weeks ago

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 commented 2 weeks ago

I'm fine with that.

TAEMBO commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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

almeidx commented 2 weeks ago

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