yuuko icon indicating copy to clipboard operation
yuuko copied to clipboard

Move options object arguments before function arguments in Command/EventListener

Open eritbh opened this issue 4 years ago • 5 comments

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

eritbh avatar Aug 26 '21 13:08 eritbh

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

NotReallyEight avatar Aug 26 '21 14:08 NotReallyEight

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.

eritbh avatar Aug 30 '21 03:08 eritbh

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

NotReallyEight avatar Aug 30 '21 07:08 NotReallyEight

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.

eritbh avatar Aug 30 '21 18:08 eritbh

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.

eritbh avatar Aug 30 '21 22:08 eritbh