-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Improve Command typings #1758
Improve Command typings #1758
Conversation
* Use multi-argument resolution for .arguments() only * Fix a bug that untyped arguments could be parsed as possibly undefined values even if they had non-undefined default values
These are already being checked in the overloads.
as it is always `undefined`
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.) |
I am not worried about |
Also fixes a bug in Option class that required properties with the value undefined were typed as optional properties
@shadowspawn By the playground, TS 4.1.2 works, TS 4.0.5 not.
|
@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. |
Doing pretty well so far! The TypeScript type tests are in 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! |
@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: |
This is mainly for commands whose arguments and options can't be fully inferred like the global `program`
Infer arguments only if `this['createCommand']` is not altered.
I just realized that Next I'll check if it is possible to Update 2: Investigating TS mixin class. Update 3: This works, but let's see the performance impact another day. |
(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!) |
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 |
I'm trying to use a "class" 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 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. |
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 😄 |
@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 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:
|
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. |
I will argue that anything will be better than the current |
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: |
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 The negated flag handling is more complicated because it should merge types from two flags. I'm going to experiment with that next.
|
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:
Any preferences @abetomo ? |
I would prefer to create a |
I have added an organisation named (There was already a |
Thanks for adding the organization. |
Development of some experimental strong inferred typing has shifted to: https://github.com/commander-js/extra-typings |
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
, andCommand
, 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 withthis
, 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