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

typings: make TypeScript interaction with channels better #3469

Open
wants to merge 5 commits into
base: master
from

Conversation

@BadCoder1337
Copy link

commented Sep 10, 2019

Please describe the changes this PR makes and why it should be merged:
This PR just edits a few types for more clear interaction with channels in TypeScript.

  • Creating channels now returns correct type (instead of union) based on specified kind of channel.
  • Editing channels also returns correct type that allows to chain promises & assign awaited values
voiceChannel = await voiceChannel.setName('voice 1337')
    .then(v => v.setBitrate(1337))
    .then(v => v.delete())

P.S. I know these requests could be combined into single .edit. That's just an example.
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.
typings/index.d.ts Outdated Show resolved Hide resolved
@iCrawl
Copy link
Member

left a comment

I know Ch makes sense, but can we stick to the established <T> convention?

@iCrawl iCrawl dismissed their stale review Sep 10, 2019

resolved

@@ -783,20 +783,20 @@ declare module 'discord.js' {
public readonly position: number;
public rawPosition: number;
public readonly viewable: boolean;
public clone(options?: GuildChannelCloneOptions): Promise<GuildChannel>;
public clone<T extends this>(options?: GuildChannelCloneOptions): Promise<T>;

This comment has been minimized.

Copy link
@kyranet

kyranet Sep 11, 2019

Contributor

Is there any reason for public clone<T extends this>(options?: GuildChannelCloneOptions): Promise<T>; instead of public clone(options?: GuildChannelCloneOptions): Promise<this>;? I use the latter in many projects of mine, and so far it works very well.

@@ -1708,7 +1708,9 @@ declare module 'discord.js' {

export class GuildChannelStore extends DataStore<Snowflake, GuildChannel, typeof GuildChannel, GuildChannelResolvable> {
constructor(guild: Guild, iterable?: Iterable<any>);
public create(name: string, options?: GuildCreateChannelOptions): Promise<TextChannel | VoiceChannel | CategoryChannel>;
public create(name: string, options?: GuildCreateChannelOptions & { type: 'voice' }): Promise<VoiceChannel>;

This comment has been minimized.

Copy link
@SpaceEEC

SpaceEEC Sep 11, 2019

Member

options may not be optional here.
Currently this will make guild.channels.create('test'); return Promise<VoiceChannel>, but it should return Promise<TextChannel>.

@@ -1708,7 +1708,9 @@ declare module 'discord.js' {

export class GuildChannelStore extends DataStore<Snowflake, GuildChannel, typeof GuildChannel, GuildChannelResolvable> {
constructor(guild: Guild, iterable?: Iterable<any>);
public create(name: string, options?: GuildCreateChannelOptions): Promise<TextChannel | VoiceChannel | CategoryChannel>;
public create(name: string, options?: GuildCreateChannelOptions & { type: 'voice' }): Promise<VoiceChannel>;
public create(name: string, options?: GuildCreateChannelOptions & { type: 'category' }): Promise<CategoryChannel>;

This comment has been minimized.

Copy link
@SpaceEEC

SpaceEEC Sep 11, 2019

Member

Same applies here, options may not be optional.

public create(name: string, options?: GuildCreateChannelOptions): Promise<TextChannel | VoiceChannel | CategoryChannel>;
public create(name: string, options?: GuildCreateChannelOptions & { type: 'voice' }): Promise<VoiceChannel>;
public create(name: string, options?: GuildCreateChannelOptions & { type: 'category' }): Promise<CategoryChannel>;
public create(name: string, options?: GuildCreateChannelOptions): Promise<TextChannel>;

This comment has been minimized.

Copy link
@SpaceEEC

SpaceEEC Sep 11, 2019

Member

As example this snippet:

const opts: GuildCreateChannelOptions = { type: 'category' };
const channel = await guild.channels.create('test', opts); // channel being TextChannel (not a CategoryChannel or an union of possible types)

It's not possible to infer what channel is being created here.
This should probably still return an union of all available types.

To solve this:

  • Should this overload return Promise<TextChannel | VoiceChannel | CategoryChannel>

    guild.channels.create('test', opts);

  • another one being added similar to the above ones returning a Promise<TextChannel>,
    However the options actually need to be optional in that case. See the first example below

    guild.channels.create('test'); or guild.channels.create('test', { type: 'text' });

@SpaceEEC SpaceEEC added the t: typings label Sep 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.