Skip to content

Decouple commands object from the bot Client object#747

Merged
Ragviswa merged 3 commits intomasterfrom
husky/decoupling/bot-commands
Feb 24, 2024
Merged

Decouple commands object from the bot Client object#747
Ragviswa merged 3 commits intomasterfrom
husky/decoupling/bot-commands

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Feb 21, 2024

ViBot [8.16.8]

Changelog

Changes

  • Moved commands await from the bot Client object and into lib/commands.js.
  • Refactored all possible references to it, including those using client.commands.
  • Some small cleanup along the way

@ghost ghost self-requested a review as a code owner February 21, 2024 18:45
@ghost ghost linked an issue Feb 21, 2024 that may be closed by this pull request
@Ragviswa Ragviswa added the Change A modification to something that exists label Feb 21, 2024
@Ragviswa Ragviswa assigned ghost Feb 21, 2024
Comment thread lib/commands.js
Comment on lines +8 to +14
load() {
const files = readdirSync('./commands').filter(file => file.endsWith('.js'));
for (const file of files) {
const command = require(`../commands/${file}`);
commands.set(command.name, command);
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rather than calling load, why not move this block into the top level of this file?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I tend to like explicitly saying "I'm initializing this," however I guess to me that only really matters if there's any necessary order to it and at least for now it just needs to be loaded before commands is used, so whenever it's 'require'd should be fine.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It turns out without a lot of refactoring this probably won't work because requiring commands inside of lib/commands.js causes several circular dependencies.

Take just addalt.js for instance. It requires ../utils.js, which requires ./commands/commands.js, which in turn requires ./lib/commands.js. Because of this require chain, lib/commands.js will not be finished & cause the whole require chain to break down.

Putting it in a function to call once lib/commands.js is not being constructed avoids the circular dependency issue.

Copy link
Copy Markdown
Contributor

@Ragviswa Ragviswa left a comment

Choose a reason for hiding this comment

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

Looks good to me, pretty much just file restructuring and a search + replace, so I'm not concerned :)

husky-rotmg and others added 3 commits February 24, 2024 11:49
Moved `bot.commands` into `lib/commands` and refactored all references to it. Made sure to include `client.commands` too for non-direct accesses
@Ragviswa Ragviswa force-pushed the husky/decoupling/bot-commands branch from ef72bd6 to 8aec176 Compare February 24, 2024 11:49
@Ragviswa Ragviswa merged commit 363b31f into master Feb 24, 2024
@Ragviswa Ragviswa deleted the husky/decoupling/bot-commands branch February 24, 2024 11:54
@Thomasc33 Thomasc33 unassigned ghost Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Change A modification to something that exists

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move bot.commands & loadCommands into a commands.js

2 participants