narrow value types based on choices

[edwardfoyle]

Updates the .choices() type signature of both Option and Argument to narrow the type to a specific string when const string arrays are used as the input.

The tests should make it pretty clear what's going on, but basically in a situation where choices is a const array, the type will be inferred to be a union of strings rather than just string:

program
  .addArgument(createArgument('<value>').choices(['thing1', 'thing2'] as const)
  .action(arg => {
    // arg is inferred to be 'thing1' | 'thing2' instead of just string
  }

[shadowspawn]

Cool! Adding support for choices was the only thing I had on my wish-list for investigating. Might be a few days before I can take a detailed look.

In your two code examples, is the first one meant to be missing the as const?

[shadowspawn]

What does the T[number] achieve? (I didn't immediately recognise that signature.)

[Edit] Oh, deconstructing the array so get the union of the values. Got it. 💡

[shadowspawn]

The main Argument tests are in a section titled Check command-arguments from .addArgument() in arguments.test-d.ts. If you look carefully, you will see that I am checking <required> and [optional] and a variadic ([o...] or <r...>) for the cases that are affected by the difference.

If you add a test like this, where I think I got the expected type right:

program
  .addArgument(new Argument('[value]').choices(['A', 'B', 'C'] as const))
  .action(arg => {
    expectType<'A' | 'B' | 'C' | undefined>(arg)
  })

TypeScript does not complain because the type checking is a bit weaker with default settings. But if you run the tests using npm t then tsd notices that the undefined is missing from the inferred type and will fail this test.

I have not worked out how to fix it yet, just spotted that your change is overwriting ArgType and so losing some of the previously inferred typing. (It is tricky to build on the existing type!)

[shadowspawn]

Take a look at argParser. I think it is doing something similar with string turning into number (say), like choices is narrowing string to 'A' | 'B' | 'C'. Supporting variadic might be a new problem though.

[edwardfoyle]

Yeah variadic is going to be interesting. I just tried it out with this solution and it does not currently work. It might be necessary to introduce something like a ChoiceT generic that gets plumbed through to the InferOptionsCombine and InferArgumentType definitions. This might also address your concern about overwriting ArgType

[shadowspawn]

Yes. Looking at Option:

  export class Option<Usage extends string = '', PresetT = undefined, DefaultT = undefined, CoerceT = undefined, Mandatory extends boolean = false> {

Keeping the types explicit and dumb at this level and doing the hard work in the later Infer routines with all the information available has worked out reasonably well.

At some level in the Infer layers, I think ChoiceT might overwrite ValueT and the deeper layers might Just Work. Might be optimistic, there are often complicated special cases!

[edwardfoyle]

Updated with support for variadic args and options. It's a bit... dense but I basically did what I said above about introducing a ChoiceT generic. I made a small update to InferOptionTypes and InferArgumentType. All the other changes are plumbing.

I tried to do what you suggested above with reusing DefaultT or CoerceT but that quickly turned into whack-a-mole with other edge cases so I added the ChoiceT type.

Also added several more tests

[shadowspawn]

I think Argument is still a problem with some combinations of chaining calls, although now working in many cases. Up until now it was possible to modify ArgType with each chaining call with the new information. This was quite nice instead of needing a big complex Infer afterwards.

However, I am not sure that is still possible with choices(). Might need to follow the pattern in Options and track multiple types and infer with all the types available?

Here are some tests to demonstrate that ordering does not matter for Option, but does for Argument because calling .choices() is losing some information:

import { Argument, Option, Command } from '..';
import { expectType } from 'tsd';
if ('Argument: when default before choices then type is combo') {

const args = new Command()

.addArgument(new Argument('').default('D' as const).choices(['C'] as const))

.parse()

.processedArgs;

expectType<'C' | 'D'>(args[0]); // FAILS

}
if ('Argument: when default after choices then type is combo') {

const args = new Command()

.addArgument(new Argument('').choices(['C'] as const).default('D' as const))

.parse()

.processedArgs;

expectType<'C' | 'D'>(args[0]);

}
if ('Argument: when override argRequired to true after choices then arg is required') {

const args = new Command()

.addArgument(new Argument('foo').choices(['C'] as const).argRequired())

.parse()

.processedArgs;

expectType<'C'>(args[0]);

}
if ('Argument: when override argRequired to true after choices then arg is required') {

const args = new Command()

.addArgument(new Argument('foo').choices(['C'] as const).argOptional())

.parse()

.processedArgs;

expectType<'C' | undefined>(args[0]);

}
if ('Argument: when override argRequired to true before choices then arg is required') {

const args = new Command()

.addArgument(new Argument('foo').argRequired().choices(['C'] as const))

.parse()

.processedArgs;

expectType<'C'>(args[0]);

}
if ('Argument: when override argRequired to true before choices then arg is required') {

const args = new Command()

.addArgument(new Argument('foo').argOptional().choices(['C'] as const))

.parse()

.processedArgs;

expectType<'C' | undefined>(args[0]); // FAILS

}
if ('Option: when default before choices then type is combo') {

const opts = new Command()

.addOption(new Option('--foo ').default('D' as const).choices(['C'] as const))

.parse()

.opts();

expectType<{ foo: 'C' | 'D' }>(opts);

}
if ('Option: when default after choices then type is combo') {

const opts = new Command()

.addOption(new Option('--foo ').choices(['C'] as const).default('D' as const))

.parse()

.opts();

expectType<{ foo: 'C' | 'D' }>(opts);

}
if ('Option: when makeMandatory before choices then arg is mandatory') {

const opts = new Command()

.addOption(new Option('--foo ').choices(['C'] as const).makeOptionMandatory())

.parse()

.opts();

expectType<{ foo: 'C' }>(opts);

}
if ('Option: when makeMandatory after choices then arg is mandatory') {

const opts = new Command()

.addOption(new Option('--foo ').makeOptionMandatory().choices(['C'] as const))

.parse()

.opts();

expectType<{ foo: 'C' }>(opts);

}

[shadowspawn]

I suspect Argument could track two types instead of just one: the base ArgType (which is either string or from argParser() or from choices(), and includes variadic) and another for the extra possible types (undefined and from default()).

Not sure if that will be easier to reason about than saving up the types for a big Infer when addArgument.

[shadowspawn]

Ah ha! I just found an existing bug showing that Argument repeatedly modifying ArgType does not cover all the (pre-choices) permutations.

The two-type approach I was wondering about would be able to cope but still require some extra logic.

Currently leaning towards the Option approach of tracking multiple types. I do prefer the idea of Option and Argument being similar in handling, much as I wish it was all easier...

Failing case:

const args = new Command()
  .addArgument(new Argument('foo').default('missing').argOptional())
  .parse()
  .processedArgs;
expectType<string>(args[0]); // FAILS because argOptional added back undefined

[shadowspawn]

I suggest you wait while I have a go at rewriting the Argument type handling. (Unless you are really keen!)

[shadowspawn]

I focused on reworking Argument generics without trying to make choices() easy as such, so choices is still to come: #31

[edwardfoyle]

I'll wait for that other PR to go in before picking this one up :) Might be a couple days before I can circle back to this anyway.

[edwardfoyle]

Just pushed up some changes. Looks like I bungled the merge a bit. Will come back to that in a bit but I think all the functional parts are there

[edwardfoyle]

Oh I see, PR was just based off the wrong branch

[edwardfoyle]

Reverted some autoformatting. I think this is ready for another review now

[shadowspawn]

LGTM. Thanks!

[abetomo]

👍

[shadowspawn]

Released in v10.0.3