yuuko
yuuko copied to clipboard
Move options object arguments before function arguments in Command/EventListener
Instead of
export default new Command('ping', msg => {
// this could be long...
}, {
guildOnly: true,
});
we should support putting options at the beginning before the potentially long function body:
export default new Command('ping', {
guildOnly: true,
}, msg => {
// this could be long...
});
Same goes for EventListener - especially important for this one since the once option changes the meaning of the listener body significantly.
Will require deprecation for the old form - both should be supported until v3. Runtime checks will need to be present past v3 to support the options being optional but coming before another required parameter.
old idea
shower thought: instead of
new EventListener('ready', /* ... */, {once: true})
it would be trivial to allow
new EventListener('ready:once', /* ... */);
Is this a good idea? I don't know of any other options we'd want to put on an event listener, so is the options object even necessary? I feel like allowing it via options is a cleaner way to do it, but requiring a full options object for a single property that will always be true or not present seems kinda eh. But I also don't think I would want the :ready method to be the only way to do it, because it's pretty different from how anything else handles one-time listeners.
As far as typings go it should be pretty easy I think - can make a new interface for the events that uses template literal string types for the additional keys I think
Hmm, wouldn't it be less clean as an idea? I can't really think of a better way to do so, but the current system seems "good" in terms of how clean it can be
I think my main concern with this is that if you're registering a complex listener, its easy to miss the once: true at the very bottom of the definition when reading the code back later. Commands have the same issue. I think a better way to handle both cases is to allow the options object to come before the function. Performing extra checks at runtime to allow for a different order of the arguments shouldn't be a big deal for performance since commands and event listeners are only registered once for the lifetime of the bot process.
I guess the other question is, do we want to enforce ordering the other way for v3, or does it make sense to allow both forms forever? I am slightly concerned that this would lead to confusion if the order of things isn't consistent.
I think you can like allow both at the release and deprecate the older way (or somehow idk), and in another release remove it completely and stay with the new order could be better
Typescript doesn't like putting optional arguments before required ones, but it's easy enough to do it via overrides and some runtime shuffling of arguments. I definitely want the options argument to be optional - I think the readability benefits will generally be worth the extra overhead. My only concern is how this will look in the generated docs, because I don't want to confuse people with weird looking signatures in docs if possible. Will need to look into that.
Committing to this. I got it working locally, and I can mark the messy implementation signature internal so it doesn't get included in docs. This is good enough for me.