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

fix(typings): return types for 'Webhook(Client)#send()' #4876

Merged
merged 6 commits into from Jan 22, 2021

Conversation

izexi
Copy link
Contributor

@izexi izexi commented Oct 2, 2020

As shown below, Webhook(Client)#send() would either return a Message (if the client was initiated and the channel was cached) or it would return the raw message response from the API (which has been added as a APIRawMessage interface in this PR).

const channel = this.client.channels ? this.client.channels.cache.get(d.channel_id) : undefined;
if (!channel) return d;
return channel.messages.add(d, false);

Cuurently the typings do not reflect this behaviour, this PR fixes that.

I did consider making use of the APIMessage over on discord-api-types but that would mean introducing another dependency which be a breaking change for all Typescript developers.

⚠️ This change is breaking for Typescript developers that make use of properties or methods exclusive to a Message and not within the APIRawMessage from the resolved returned value of Webhook(Client)#send().
Here's a snippet of how I tested these changes
import { expectType } from 'tsd';
import { WebhookRawMessageResponse, Message, WebhookClient } from 'discord.js';

const webhookClient = new WebhookClient('', '');

expectType<Message | WebhookRawMessageResponse>(
  await webhookClient.send('foo')
);

expectType<(Message | WebhookRawMessageResponse)[]>(
  await webhookClient.send('foo', { split: true })
);

expectType<
  Message | WebhookRawMessageResponse | (Message | WebhookRawMessageResponse)[]
>(await webhookClient.send({ embeds: [] }, { split: true }));

expectType<Message | WebhookRawMessageResponse>(
  await webhookClient.send('foo', { embeds: [] })
);

expectType<
  Message | WebhookRawMessageResponse | (Message | WebhookRawMessageResponse)[]
>(await webhookClient.send('foo', { embeds: [], split: true }));

Status

  • 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

Semantic versioning classification:

  • This PR changes the library's interface (methods or parameters added)
    • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
  • This PR only includes non-code changes, like changes to documentation, README, etc.

@vladfrangu
Copy link
Member

I don't see how adding a dependency on discord-api-types is breaking for TS developers (note: dependency, not dev/peer dep).. 👀

@vladfrangu vladfrangu requested review from iCrawl and kyranet and removed request for iCrawl October 2, 2020 09:53
Copy link
Contributor

@NotSugden NotSugden left a comment

Choose a reason for hiding this comment

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

these overloads should be applied to WebhookClient directly, as the Webhook class is instantiated by internal methods and always has a client, thus always returns a full message

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

izexi commented Oct 2, 2020

I don't see how adding a dependency on discord-api-types is breaking for TS developers (note: dependency, not dev/peer dep).. 👀

@vladfrangu I was assuming that they had to go under devDependicies since that's where the other type packages were listed. Although, as mentioned by Sudgen some of the OAuth2 properties wont be present so I could Omit them I guess but I'm not sure about adding it as a dependency just for the APIMessage.


these overloads should be applied to WebhookClient directly, as the Webhook class is instantiated by internal methods and always has a client, thus always returns a full message

@NotSugden If the channel isn't within the ChannelManager#cache Collection then it would return the raw message, although this isn't possible unless the developer removes the channel manually I believe it should still be accounted for.

@kyranet
Copy link
Member

kyranet commented Dec 8, 2020

I don't see how adding a dependency on discord-api-types is breaking for TS developers (note: dependency, not dev/peer dep).. 👀

@vladfrangu I was assuming that they had to go under devDependicies since that's where the other type packages were listed. Although, as mentioned by Sudgen some of the OAuth2 properties wont be present so I could Omit them I guess but I'm not sure about adding it as a dependency just for the APIMessage.

Since we're releasing this for discord.js v13, I personally don't see a reason to not include discord-api-types, we can afford semver-major this time.

@iCrawl
Copy link
Member

iCrawl commented Dec 14, 2020

For what its worth: I would like to keep this as is and move forward like this until we have a clear path for modularization.

@iCrawl iCrawl merged commit eb28ee7 into discordjs:master Jan 22, 2021
@izexi izexi deleted the fix-Webhook(Client)-send branch January 22, 2021 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants