Improve Command typings

[PaperStrike]

Pull Request

Problem

This is related to #1245, #1585, similar to #1356 that tries to improve the TypeScript writing experiences. Commander and TypeScript have changed a lot, and improving the typings of commander today, to provide more specific type information of given options, has become more feasible.

Solution

This PR creates a generic class for each Argument, Option, and Command, and solves the camelCase, .action, .addArgument, .addOption, and .opts problems that blocked #1356. You can try this TS Playground link, scroll to the bottom, hover the variables, make some changes and see if there're some syntax I failed to support.

However, there're still some other problems like storeOptionsAsProperties, which is likely able to be solved by intersecting the generics returned with this, but I experienced a huge performance impact when I add that & this to each .argument and .option overload.

Also, the result of chaining off of commander needs to be assigned to a new variable. This can be improved if one day TS supports Non‑void returning assertion functions.

Any comment on this?

ChangeLog

[shadowspawn]

Very interesting. It will take me a while to work through.

First question, do you know what version of TypeScript this requires? (We haven't needed to give guidance on TypeScript compatibility. Yet.)

[shadowspawn]

I am not worried about .storeOptionsAsProperties(), it is listed under "legacy" in the README.

[PaperStrike]

@shadowspawn By the playground, TS 4.1.2 works, TS 4.0.5 not.

Option and Argument classes now work as well! (see this new playground link)

[shadowspawn]

I am not sure if there is any way to minimise the noise, but it can be hard for a casual reader to see the shape amongst the generic and options in the intellisense:

[PaperStrike]

@shadowspawn I don't know either if there is any way to minimise the noise. But if the reader can give one letter of the option name, the intellisense will give very readable results:

And ah... I'm not a type expert, I feel very unconfident about what the consequence these changes will bring, and I don't know how to write proper type test cases. Any help on this will be very appreciated.

[shadowspawn]

Doing pretty well so far!

The TypeScript type tests are in index.test-d.ts and can be run using npm run test-typings. It is possible to test that lines have an error too, rather than "valid" code, like:

const program: commander.Command = new commander.Command();
// @ts-expect-error Check that Command is strongly typed and does not allow arbitrary properties
// eslint-disable-next-line @typescript-eslint/no-unused-expressions
program.silly; // <-- Error, hurrah!

[PaperStrike]

@shadowspawn Thank you! I will try to fix the failing test cases hours later.

A good news, I just figured out a way to merge the options and reduce the noise a bit:

• the action intellisense is shorter:
  
  
• as options are merged to a single object that has no more &:
  

Playground link

[PaperStrike]

I just realized that Command class is expected to be extendable. These changes sadly break the subclasses as this is not used in many places for performance issues.

Next I'll check if it is possible to use some pseudo properties instead of generics for typing and if that doesn't affect the performance much. Update 1: Impossible. (Note to myself: this always refers to the class or subclass. Nothing can be attached to it.)

Update 2: Investigating TS mixin class. Update 3: This works, but let's see the performance impact another day.

[shadowspawn]

(If subclassing proves impossible or impractical, it is fairly rare and it might be possible to offer this functionality in a limited form. You have achieved a lot more than previous experiments!)

[shadowspawn]

I'm doing some experimenting with the option strings to work through the variations and learn more. Pure template work so far, and not sure how much will be useful.

https://github.com/shadowspawn/commander.js/blob/feature/infer-types/typings/infer.test-d.ts

[PaperStrike]

I'm trying to use a "class" ArgsAndOptions to create mixins to support this and store the parsed args and options at the same time without creating a long & this chain: Playground link, ArgsAndOptions is in Line 63.

The performance feels as good as before, and subclass methods logically work, but TS itself seems to have some other problems that it looks like it run into an infinite loop when checking the compatibility of the returned types of .createCommand between classes. The playground reports no error and no unused variable, which is unusual. Putting the code to IDE shows TS2589: Type instantiation is excessively deep and possibly infinite on MyCommand, and removing the base .createCommand method (L100) will remove the error and the playground will show the unused variables as usual.

This is very confusing to me. These days I'm looking for the solution and if there's a better way. Probably eventually we can only offer this in a limited form, but for now I'm still studying and looking.

[danielo515]

Hey, this looks very promising! Very good work so far. Any chance this will be merged? Having to check the argument options because you don't get them properly typed is something that makes me feel uncomfortable 😄

[shadowspawn]

@PaperStrike has found approaches to solve most of the technical challenges to make the typing possible.

Assuming we get it all working, merging it will depend on the tradeoff of some very complicated types appearing in editor and error messages, vs getting some strong typing and editor assistance for .opts() and .action() parameters.

If it isn't a clear win, or if subclassing proves beyond reach, or to get some more feedback first, it could perhaps be published as a separate package or types to try out.

The three high-level approaches so far to build the type information have been:

• pure generics (but breaks subclassing)
• pseudo properties (but breaks subclassing)
• mix-ins, with possible performance issue

[shadowspawn]

I have only done a little experimenting so far in the playground with the latest mix-in work.

I have separately been working through typing the command-argument parameters to the action handler using generics pattern, to better understand what is involved. The arguments are a bit simpler than the options.

https://github.com/shadowspawn/commander.js/blob/feature/infer-types/typings/infer-arg-count.test-d.ts

[danielo515]

I will argue that anything will be better than the current action(fn: (...args: any[]) => void | Promise<void>) 😄 .
Even if we only get the arguments, options as an unknown object that you have to validate and the last CMD with the proper methods it provides I will call it a big win.
Thank you for your efforts!

[shadowspawn]

I copied the code out and reproduced the "excessively deep and possible infinite" error mentioned in #1758 (comment)

I did a little searching and found some coverage in a very in-depth article on mixins, so might be some work-arounds if mixins proves the most usable approach:
https://www.bryntum.com/blog/the-mixin-pattern-in-typescript-all-you-need-to-know/

[shadowspawn]

The coverage by @PaperStrike is very impressive! I have been working through the cases slowly, taking weeks to do what @PaperStrike did in days: 😅

Picked up a few minor omissions with option flag formats using '|' or ' ' as separator.

The negated flag handling is more complicated because it should merge types from two flags. I'm going to experiment with that next.

program
  .option('-c, --color <shade>')
  .option('-C, --no-color`);

[shadowspawn]

I think the changes are too dramatic to drop straight into Commander. Some possible places we could put the code and make it available as a separate package with experimental typings for people to try out and learn more:

1. create a commanderjs organisation (like mochajs and expressjs) and add a repo there
2. we could publish out of a folder in this repo
3. I could create a repo under my account and publish

Any preferences @abetomo ?

[abetomo]

I would prefer to create a commanderjs organisation.
I think it is fine to publish them in your repository, but I think it would be easier to understand if they were grouped together under the organization's control.

[shadowspawn]

I have added an organisation named commander-js on GitHub and npmjs.

(There was already a commanderjs organisation on GitHub so that name not available.)

[abetomo]

Thanks for adding the organization.

[shadowspawn]

Development of some experimental strong inferred typing has shifted to: https://github.com/commander-js/extra-typings