-
Notifications
You must be signed in to change notification settings - Fork 0
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
Decouple commands
object from the bot Client
object
#747
Conversation
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); | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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 :)
Moved `bot.commands` into `lib/commands` and refactored all references to it. Made sure to include `client.commands` too for non-direct accesses
ef72bd6
to
8aec176
Compare
ViBot [8.16.8]
Changelog
Changes
commands
await from the bot Client object and intolib/commands.js
.client.commands
.