refactor(client)!: remove BaseClient
#11274
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
sdanialraza
changed the title
BaseClient
Jiralite
force-pushed
the
refactor/remove-base-client
branch
from
6ced9bd to
514b015
Compare
sdanialraza
requested review from
Copilot
and removed request for
SpaceEEC,
iCrawl,
kyranet and
vladfrangu
There was a problem hiding this comment.
Pull Request Overview
This PR removes the BaseClient class and merges all its functionality directly into the Client class, simplifying the inheritance hierarchy.
- Eliminates the
BaseClientabstraction layer - Client now extends
AsyncEventEmitterdirectly instead of inheriting from BaseClient - All BaseClient methods and properties moved to Client class
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/discord.js/typings/index.d.ts | Removed BaseClient type definition and updated Client to extend AsyncEventEmitter directly, added previously inherited methods and properties |
| packages/discord.js/src/index.js | Removed BaseClient export |
| packages/discord.js/src/client/Client.js | Now extends AsyncEventEmitter directly and includes all functionality previously in BaseClient (REST management, options handling, helper methods) |
| packages/discord.js/src/client/BaseClient.js | Deleted entire file as functionality merged into Client |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| throw new DiscordjsTypeError(ErrorCodes.InvalidType, 'options', 'object', true); | ||
| } | ||
|
|
||
| const defaultOptions = Options.createDefault(); |
There was a problem hiding this comment.
This is now a duplicate of the variable defaults defined later on and should be done only once.
| actions: false, | ||
| presence: false, | ||
| }); | ||
| toJSON(...props) { |
There was a problem hiding this comment.
Considering we don't need nor want to be able to pass more properties to toJSON() this can probably be simplified by removing `...props’ here and on next line
| public login(token?: string): Promise<string>; | ||
| public isReady(): this is Client<true>; | ||
| public toJSON(): unknown; | ||
| public toJSON(...props: Record<string, boolean | string>[]): unknown; |
| */ | ||
| constructor(options) { | ||
| super(options); | ||
| constructor(options = {}) { |
There was a problem hiding this comment.
options are a required parameter for Client constructor (in particular the intents are), so why have a default value defined here?
Please describe the changes this PR makes and why it should be merged:
Resolves #11271
BREAKING CHANGE:
BaseClienthas been removed and its functionality has been merged intoClient.Depends on: