Few minor changes #94
Conversation
- Updated gitignore to ignore package-lock.json - Removed "engines" key from package.json as code works on v18 and later of Node.js just fine - Updated how "logsFileToChannel" feature so its more readable and visually appealing rather than a blob of text One other thing I do recommend in the future is that you register slash commands before you login to the client as it can and does cause issues if the bot tries to register a new command for the guild, sometimes discord rejects it and the bot throws an error. Double registering can also occur (and has for me)
There was a problem hiding this comment.
Hey @Kyvrixon, thank you for your contribution but I will have to request a few changes before merging this PR.
Firstly, the package-lock.json file is important in my opinion for consistent builds and after reviewing a few articles about it I still think it should not be removed so please revert those changes.
Regarding the engines in package.json, it's not just about compatibility of the code with node versions but rather keeping track of the requirements of the dependencies. Currently some dependencies support only NodeJS 16 or above, soon will probably change to 18 and above etc. So the engines part of the file will enforce a node version that works if not now at least in the future so I will not remove it, should also be reverted back.
The change in error logging to make it easier to read is appreciated, that will be merged once you revert the other changes. Thank you.
On another note, please clarify the issue with registering slash commands. Currently the ready event responsible for that runs before the client logging in so I'm not sure what the issue is.
The file gets overwritten and refreshed everytime you run
I have fully tested every single feature and command and everything works perfectly fine with no issues on every major version from v18. I acknowledge the tracking with enforcing a version but seems to be a redundant restriction.
Of course! The ready event executes when the client is fully online and ready - as you know, so after it logs in properly the event will be fired. This causes problems especially when registering new commands to a guild - usually uncached - (especially for the first time) there is quite a large chance that commands can be double registered and painstakingly hard to remove, and it has for me a couple of times until i roughly relocated that code logic to just above the event handler in bot.js, after that everything worked fine and discord didnt throw any errors and no commands were double registered. I would assume this either running twice without waiting for discords cache to get up to speed (explains the double registering) or its running as the client has logged in (which isnt recommended). I'll look into it futher later on in the week. After reading all this, if you'd still like me to revert your requested changes im more than happy to ralph |
|
I understand your reasoning behind this but I would still appreciate the changes being reverted back. Regarding The main goal of limiting to NodeJS 18 or above is to make sure no one installs the bot on much older versions like NodeJS 14 or 16 and it can always change in the future. I'm fine with keeping it to only NodeJS 18 and above. About the registration of commands, I'm still not really sure about that part since I never encountered any issue with command registration nor had any reports of it having issues with any of the users of the bot. Feel free to make a PR or let me know if you think it can be improved in some way as I'm always open to such improvements of course! Thanks again. 😄 |
|
No worries, I have reverted the changes. Do double check though |
One other thing I do recommend in the future is that you register slash commands before you login to the client as it can and does cause issues if the bot tries to register a new command for the guild, sometimes discord rejects it and the bot throws an error. Double registering can also occur (and has for me)