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

refactor(typings): partial Types #3493

Merged
merged 13 commits into from Oct 11, 2019

Conversation

@Tenpi
Copy link
Contributor

commented Sep 30, 2019

Please describe the changes this PR makes and why it should be merged:
This is a quick fix for Issue #3486 (Typescript users are forced to account for Partial types, even if they aren't using them). I separated partial properties into their own types such as:

type PartialMessage = {
    id: string;
    partial: true;
    fetch(): Promise<Message>;
} & {
    [P in keyof Omit<Message, "id" | "partial">]: null;
}

Made the following properties non-nullable (if any are wrong you can revert them): message.author.
And now all events will emit as Message | PartialMessage, Channel | PartialChannel, etc.

client.on("message", async (message) => { 
    //message is Message | PartialMessage implicitly
    console.log(message.author.id) //message.author is possibly null
    if (message.partial) message = await message.fetch();
    console.log(message.author.id) //no errors
});

client.on("message", (message: Message) => {
    console.log(message.author.id) //message is Message, no errors
});

In separate functions, you must type message as Message | PartialMessage if you are using partials.

Finally, I deleted the type PartialType since it was an exact duplicate of PartialTypes (?)

Improvements: Emit partial types only if they are set in ClientOptions. I tried doing this, but I gave up since it was too complicated.

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.
@Monbrey

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2019

This PR only includes non-code changes, like changes to documentation, README, etc.

Not sure that's correct...

@Tenpi

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2019

This PR only includes non-code changes, like changes to documentation, README, etc.

Not sure that's correct...

Sorry I fixed it... I wasn't too sure if modifying the typings fell under "code-changes".

@bdistin

This comment has been minimized.

Copy link
Member

commented Sep 30, 2019

typings are documentation, not code changes.

Copy link
Contributor

left a comment

This looks awesome! Just a few suggestions and changes I think would be correct

@@ -935,12 +935,12 @@ declare module 'discord.js' {
public editedTimestamp: number | null;
public readonly edits: Message[];
public embeds: MessageEmbed[];
public readonly guild: Guild | null;
public readonly guild: Guild;

This comment has been minimized.

Copy link
@vladfrangu

vladfrangu Sep 30, 2019

Contributor

This is wrong. A guild can still be null (DM messages)

partial: true;
fetch(): Promise<Message>;
} & {
[P in keyof Omit<Message, 'id' | 'partial' | 'fetch'>]: null;

This comment has been minimized.

Copy link
@vladfrangu

vladfrangu Sep 30, 2019

Contributor

Can't this be

[K in keyof Omit<Message, 'id' | 'partial>]: Message[K] | null;

? Also, why omit the function?

partial: true;
fetch(): Promise<Channel>;
} & {
[P in keyof Omit<DMChannel, 'id' | 'partial' | 'fetch'>]: null;

This comment has been minimized.

Copy link
@vladfrangu

vladfrangu Sep 30, 2019

Contributor

This is wrong.. it doesn't extend just a DMChannel

partial: true;
fetch(): Promise<GuildMember>;
} & {
[P in keyof Omit<GuildMember, 'id' | 'partial' | 'fetch'>]: null;

This comment has been minimized.

Copy link
@vladfrangu

vladfrangu Sep 30, 2019

Contributor

Ditto to my review on Guild

partial: true;
fetch(): Promise<User>;
} & {
[P in keyof Omit<User, 'id' | 'partial' | 'fetch'>]: null;

This comment has been minimized.

Copy link
@vladfrangu

vladfrangu Sep 30, 2019

Contributor

Ditto

Tenpi added 3 commits Sep 30, 2019
@Tenpi

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2019

Thanks for the review, without the Omit the partial property would be true & null, which actually makes it false so I had to add it to fix that bug.

@vladfrangu

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2019

No worries, but I was asking why you Omitted the fetch function 😅

@Tenpi

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2019

No worries, but I was asking why you Omitted the fetch function 😅

Oh yeah you're right, that was unnecessary

Copy link
Contributor

left a comment

A "Partial" type could just be declared where you can pass the structure's type as a generic since the code is just being repeated

type Partial<T> = {
	id: string;
	partial: true;
	fetch(): Promise<T>;
} & {
	[K in keyof Omit<T, 'id' | 'partial'>]: T[K] | null;
};

public on(event: 'guildBanAdd' | 'guildBanRemove', listener: (guild: Guild, user: Partial<User>) => void): this;
// etc
@Tenpi

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2019

Thanks for the review, since Partial<T> is already a built-in typescript utility type I called it Partialize<T> instead. Tell me if you want another name ¯\(ツ)

@SpaceEEC SpaceEEC added the t: typings label Oct 1, 2019
@iCrawl

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

Is Partial some reserved/taken keyword or why did you choose to go with Partialize?
I'd rather stick with Partial in this case.

You know what, I should actually read comments too, not just commits.

@izexi
izexi approved these changes Oct 1, 2019
@Skillz4Killz

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

@Tenpi You are amazing. This is an awesome PR! Thank you so much! This will solve all my headaches with partials.

@kyranet
kyranet approved these changes Oct 4, 2019
@Skillz4Killz

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2019

@Tenpi Would this or could it be adjusted to also solve #3533 ?

@Tenpi Tenpi referenced this pull request Oct 11, 2019
1 of 1 task complete
public readonly joinedAt: Date;
public joinedTimestamp: number;
Comment for lines 839  – 840

This comment has been minimized.

Copy link
@Lewdcario

Lewdcario Oct 11, 2019

Member

Should be marked as null, due to 13bfceb.
Explanation: #3533 (comment)

This comment has been minimized.

Copy link
@Tenpi

Tenpi Oct 11, 2019

Author Contributor

Updated, thanks for the explanation

@iCrawl iCrawl changed the title Added Partial Types at(typings): partial Types Oct 11, 2019
@iCrawl iCrawl merged commit 79133b4 into discordjs:master Oct 11, 2019
3 checks passed
3 checks passed
ESLint ESLint
Details
TSLint
Details
Documentation
Details
@iCrawl iCrawl changed the title at(typings): partial Types refactor(typings): partial Types Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.