feat(Client): add conditional ready typings

[memikri]

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

This PR adds conditional typings to the Client class, allowing for TypeScript to infer the ready state of the client and provide access to properties that are initialized at login, such as Client#application and Client#user. This also adds a client parameter to the Client.on("ready") event, defined in typings as a ready client.

This will be a major quality-of-life improvement for users of TypeScript and typings users, as it allows for access to client properties without optional chaining or non-null assertions.

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
• This PR changes the library's interface (methods or parameters added)

[DTrombett]

I think transforming Client in a generic type is a too big change which will make a lot of problems to TS users... Why not just using something like is done with Partials? Like, create an interface called LoggedClient and pass it in events (where we're sure the client is logged).
Also in typings there are a lot of Client... Should we change them all now?
Btw, the ready property should be a getter which checks for this.ws.status === 0

[memikri]

I think transforming Client in a generic type is a too big change which will make a lot of problems to TS users...

ef07148#diff-4f45caa500ef03d94d3c2bfa556caa1642df95d4e2b980d76b876a8fd2e8c522R283

It defaults to boolean which will result in the same typing as before and thus no changes required to existing TypeScript projects (unless they want to).

Why not just using something like Partials? Like, create an interface called LoggedClient and pass it in events (where we're sure the client is logged).

That's another approach that could work; I settled on a generic type as it results in minimal overhead for existing users. i.e. just change Client to Client<true> and it works, no need to import another type.

Also in typings there are a lot of Client... Should we change them all now?

In some cases I think that makes sense, yeah. Like Messages can't really be created by a client that isn't logged in.

Btw, the ready property should be a getter which checks for this.ws.status === 0

On it :)

[memikri]

Are these typing changes suitable? If so, will add them to PR.

memikri@d3addca

[ImRodry]

I would say so although you missed quite a lot of Base extensions

[memikri]

I was leaving the others alone as I thought they could reasonably exist on a client that wasn't logged in, though it looks like I missed a few others nvm lol

[monbrey]

I'm not sure I fully understand the benefit of this. If the TS user has to manually define it as Client<true>, how is that any different to non-null asserting the properties? It's just a shortcut that does them all at once?

TypeScript isn't "inferring" anything if I'm the one casting it to an interface where the properties aren't null, because manual casts can still be done incorrectly.

[ImRodry]

The way I see this is that the classes extend a Base class that is already logged in so it knows the properties in client are not null, the user would not have to do any non-null asserting.

[monbrey]

That sounds even more incorrect. Though uncommon, I don't see anything guaranteeing that an instance of a class extending base has a logged in client.

These are forced types, not inferred ones.

[ImRodry]

You can’t really interact with Discord if you’re not logged in so anything the client receives has to be with a logged in client

[memikri]

If the TS user has to manually define it as Client, how is that any different to non-null asserting the properties? It's just a shortcut that does them all at once?

This PR adds the Client#isReady(): this is Client<true> method, which provides a type-safe way to assert that the client is a Client<true>. Additionally, the ready client is emitted from the ready event, allowing for type-safe initialization logic.

A potential use pattern would be:

• Use Client (with no generic) on a custom Bot class or other custom code
• Use Client<true> on classes that are triggered by events (e.g. commands). This is implied on most event objects with the expansion of Base in this PR.
• For other methods or functions that interact with a client potentially before login, wrap login-dependent code in if (client.isReady()) { ... }

[monbrey]

The problem is that from what I can see, you're not defining it as logged in only on the Client events - you're defining it as always true on the base class itself. This simply isn't true 100% of the time.

As much as I hate myself for writing terrible discord.js usage like this:

const client = new Client({ intents: ["GUILDS"] });
client.token = "<token>";
const guild = await client.guilds.fetch("412130302958239745");

This code is functional, and will return a horribly useless partial Guild object. But it does work.

client.on("message", message => {
  message.client.destroy();
});

This had a logged in client, and now it doesn't anymore. It's still typed as Client<true> though.

To be clear, I very much do like the Client#isReady() typeguard - that's performing the check properly. Extending Base<true> doesn't appear to be.

[memikri]

As much as I hate myself for writing terrible discord.js usage like this:

const client = new Client({ intents: ["GUILDS"] });
client.token = "<token>";
const guild = await client.guilds.fetch("412130302958239745");

This code is functional, and will return a horribly useless partial Guild object. But it does work.

I don't know of any projects using such methods for fetching data without logging in. However, if this is something that some users do, we could remove this by making the client's managers (including Client#guilds, etc.) conditional on being logged in as well. Alternatively, we could drop support for fetching objects without being logged in.

client.on("message", message => {
 message.client.destroy();
});

This had a logged in client, and now it doesn't anymore. It's still typed as Client though.

const { Client } = require("discord.js");

const client = new Client({ intents: ["GUILDS", "GUILD_MESSAGES"] });
client.on("ready", () => {
  console.log("before destroy()", client.user.tag);
  client.destroy();
  console.log("after destroy()", client.user.tag);
});

client.login(process.env.DISCORD_TOKEN);

$ node index.js 
before destroy() Rikona#7979
after destroy() Rikona#7979

Calling client.destroy() and then accessing client data is rather counter-intuitive, but this example (tested with the current master branch) shows that client properties are indeed not null and can be accessed. This is consistent with the types in this PR, stating that the properties are not null.

[DTrombett]

True, the main problem is the advantage of that. In some places we're sure that the client is logged or that, however, some properties are not null, but we still need to use an assertion like isReady(), which is really annoying... Anyway, I think this at least add an easy way to let TS know that some properties are not nullable without using non null assertions or difficult typecast.

[1Computer1]

If there exists cases where it is wrong, then it is wrong. Not a fan of this unless those edge cases can be fixed.
And removing things that make it wrong is a bad approach.

[memikri]

The underlying issue seems to me to be that there is no single well-defined state in which the client is logged in aside from client events (but even those can be triggered without logging in).

With this in mind, universally generalizing the Base class doesn't make sense currently. I can see a few potential solutions for this:

• Keep Base generic and set classes that could be retrieved without a logged-in client to extend Base<boolean> instead of Base<true>. This doesn't really offer a benefit, overall, though, since for example Guilds can be created without logging in as shown earlier.
• Move client.guilds and other managers behind client login. This would align with the intended use of the library and enforce a type-safe control flow. This is obviously a breaking change, but since this is a major semver change, this should be quite acceptable. Alternatively but pursuant to the same end-goal, Client#token could be made read-only. This would prevent arbitrarily assigning it and making API requests without following the standard control flow for logging in.
• Remove the generic from Base entirely, reverting back to commit 4bdf25a. This would remove a lot of the potential benefit of the typing change; however, Client#isReady() and the client generic will remain, which is still beneficial.

@1Computer1 @monbrey Which of these seem the best route to you? Or, do you have other alternatives?

[memikri]

It seems to me actually that Base is outside the scope of the PR. I'll revert back to 4bdf25a, before Base was generified.

[vaporoxx]

I think that creating an interface ConnectedClient extends Client where you override the relevant properties as non-nullable is a more elegant approach, something like Client<true> can be confusing at first sight and the If type also won't work if we decide to have optional properties at some point in the future.

[memikri]

As per an earlier comment on that:

Why not just using something like Partials? Like, create an interface called LoggedClient and pass it in events (where we're sure the client is logged).

That's another approach that could work; I settled on a generic type as it results in minimal overhead for existing users. i.e. just change Client to Client<true> and it works, no need to import another type.

[DTrombett]

Well, in an event (that is not in our index file) we'll need to import Client probably, as we don't need to declare them but use with a function parameter, or with the .client property

[memikri]

Well you would still need to do casting either way since we can't count on event clients being logged in except for in the ready event

[DTrombett]

Yeah, that's why it's the same IMO... Maybe creating another type will be useful for people who are asking why is there a generic, but, at the same time, we still need to use the isReady / a typecast to let TS know that the client is logged.

[memikri]

@SpaceEEC @kyranet