Open bakkot opened 2 years ago
The magic to make it work is a bit opaque to me, but the results look good from experimenting. The strict: false
ones are annoyingly inexact, but look accurate! And careful callers will need to cope.
@bakkot the types seem solid to me, I appreciate that there are useful affordances, e.g., having typing for arrays, having the array default to empty if allowPositionals
not set.
@bakkot could we publish the types to both to node:util
and to pkgjs/parseargs
in DefinitelyTyped, alternatively is there an easy way to for us to reference the node:util
types when using parseArgs
, and we can document this in the README?
There’s no easy way in DT for one package to reference the types for another, so the best approach i think is to duplicate them in both DT packages.
@bcoe Since you control parseArgs
, you can include the types for it in the package itself rather than putting them on DefinitelyTyped. And in that case, yes, you can re-export the types for the version of parseArgs
in node:util
from DefinitelyTyped, once those exist.
That said, it looks like types for 18.0.0 are still WIP, so I'm probably not going to submit these for util.parseArgs
for a while yet. Maybe it makes sense to just provide them for pkgjs/parseargs
for now? And in that case, would you be OK with having them live in this repo instead of DefinitelyTyped for the moment?
I find it vastly superior for types to be in DT instead of the package directly, since it avoids conflating the semver of the implementation with that of the types.
My comments are at a non-expert level.
To keep the version numbers sane and allow parseArgs
package to include changes which have not yet landed in node, I was expecting separate type definitions (even it matching) for parseArgs
package and node:util
. Rather than one referencing the other.
As for whether to include in package, a comment each way!
I'm not concerned about the conflation of types and code by publishing within package. I consider the types part of the package.
However, the TypeScript publishing guidance, https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html#handbook-content says:
If your types are generated by your source code, publish the types with your source code. Both TypeScript and JavaScript projects can generate types via declaration.
Otherwise, we recommend submitting the types to DefinitelyTyped, which will publish them to the @types organization on npm.
Nice work @bakkot!! I copied your TS Playground and included some light suggestions, specifically:
Exact
to error on unknown config propertiesIsEqual
to infer various boolean
configs e.g. IsStrict
, IsAllowPositionals
, IsMultiple
IsStrict extends true : true-branch : false-branch
)type import
them in scripts and imo would be a good idea to establish a solid naming conventionAdditionally, I added a question on the first usage example where:
config.strict === true && optionConfig.multiple === true
The values
type infers values.foo
as foo: string[]
. However, if the user never passes --foo
then values.foo
would be undefined
IIRC.
@aaronccasanova Neat! Comments:
Re: "Shouldn't foo
values also have undefined
"
Uhh I guess you're right. I assumed that multiple
options would be present as an empty array if not passed, but that doesn't seem to be the case.
Your changes have broken some of the types. Specifically, evil
is now inferred as string | undefined
, whereas the correct type is string[] | string | undefined
(because the point of the evil
property is that we don't know if it's multiple
or not). The ExtractIsStrict
thing you're doing is meaningfully different from the IfDefaultsTrue
and IfDefaultsFalse
helpers I had, which handle true
, false
, and boolean
(i.e. present but not known to be either true or false) as distinct cases.
I assumed that multiple options would be present as an empty array if not passed, but that doesn't seem to be the case.
Yeah, we don't traverse the config
and set defaults on the parsed results
. The update is simply adding undefined
to the true-branch
of IsMultiple extends true
:
Specifically, evil is now inferred as
string | undefined
, whereas the correct type isstring[] | string | undefined
I'm seeing the correct types.
The
ExtractIsStrict
thing you're doing is meaningfully different from theIfDefaultsTrue
andIfDefaultsFalse
helpers I had, which handletrue
,false
, andboolean
(i.e. present but not known to be either true or false) as distinct cases.
We can confidently infer strict
since we default to true
and validate the input is a boolean
. My thinking was we only have to check is if the input is explicitly false
but perhaps there are holes in that logic.
I'm seeing the correct types.
That's because you're looking at a variable which is explicitly annotated with that type. Look at the type for values.evil
(or remove the string[]
from the type annotation and see that it still checks).
We can confidently infer strict since we default to true and validate the input is a boolean. My thinking was we only have to check is if the input is explicitly false but perhaps there are holes in that logic.
The hole is, knowing that it is definitely a boolean and is not known to be true
does not tell you it's definitely false
. The type system might just not be able to tell which it is.
Oh I see! Hmm now I'm wondering if that applies to all the Extract
default utilities in my prototype.
For strict
and allowPositionals
it doesn't end up mattering because the types for "not known to be true
" are the same as the types for "known to be false
", but the distinction is relevant for multiple
.
Welp, I totally understand the rationale for IfDefaultsTrue
and IfDefaultsFalse
now! 👍 I love the composition in ParsedPositionals
:
I tested every permutation and it's very robust 👏
Here is an updated prototype reverting the IsEqual
/Extract
utilties. This version includes:
values.foo
when the option is not provided to the CLIAs mentioned above, these are light suggestions so feel free to dismiss 🙂
Nice, LGTM.
There is not currently any TSDoc, or indeed JSDoc, for offering guidance beyond the types? (Edit: we wrote JSDoc for the internal routines, but not the exported routine!)
For example in Visual Studio Code, where I think the documentation is coming via autodownloaded TypeScript definition files, I see rich assistance for path.resolve
:
Whereas currently for parseArgs
I see raw:
@shadowspawn Good catch, updated with the description from the readme (slightly wordsmith'd for this context):
/**
* Provides a high-level API for command-line argument parsing. Takes a
* specification for the expected arguments and returns a structured object
* with the parsed values and positionals.
*
* `config` provides arguments for parsing and configures the parser. It
* supports the following properties:
*
* - `args` The array of argument strings. **Default:** `process.argv` with
* `execPath` and `filename` removed.
* - `options` Arguments known to the parser. Keys of `options` are the long
* names of options and values are objects accepting the following properties:
*
* - `type` Type of argument, which must be either `boolean` (for options
* which do not take values) or `string` (for options which do).
* - `multiple` Whether this option can be provided multiple
* times. If `true`, all values will be collected in an array. If
* `false`, values for the option are last-wins. **Default:** `false`.
* - `short` A single character alias for the option.
*
* - `strict`: Whether an error should be thrown when unknown arguments
* are encountered, or when arguments are passed that do not match the
* `type` configured in `options`. **Default:** `true`.
* - `allowPositionals`: Whether this command accepts positional arguments.
* **Default:** `false` if `strict` is `true`, otherwise `true`.
*
* @returns The parsed command line arguments:
*
* - `values` A mapping of parsed option names with their string
* or boolean values.
* - `positionals` Positional arguments.
*
*/
Took another pass and caught two more edge cases:
config
object should resolve empty parse results e.g.const results = parseArgs()
const results = parseArgs({})
type ActualResults = {
values: { [longOption: string]: string | boolean | string[] | boolean[] | undefined }
positionals: string[];
}
type ExpectedResults = {
values: {}
positionals: [];
}
strict:false
and multiple: true
should resolve(string | boolean)[]
instead of string[] | boolean[]
e.g.
// Dynamic config
const config: Config = {
strict: false,
options: { foo: { type: 'string', multiple: true } }
}
const {values} = parseArgs(config)
type ActualValues = { [longOption: string]: string | boolean | string[] | boolean[] | undefined }
type ExpectValues = { [longOption: string]: string | boolean | (string | boolean)[] | undefined }
<img width="710" alt="Screen Shot 2022-05-30 at 10 15 34 AM" src="https://user-images.githubusercontent.com/32409546/171056422-87487e90-1a0e-4928-95a2-d9ce700899e2.png">
I addressed the above edge cases and added a TON of tests to this playground.
Note: This version goes above and beyond to establish robustness between the inference utilties and the test cases (at the cost of readbility).
Nice!
Re: tests, one minor nit: I find it hard to read when there's non-local aliases for the actual results. E.g.
Expect<IsEqual<ParseArgs<{ strict: false }>['positionals'], ParsePositionals>>
vs
Expect<IsEqual<ParseArgs<{ strict: false }>['positionals'], string[]>>
or
type ExpectedValues = {
foo: ParseResultsValue<string>;
bar: ParseResultsValueMultiple<boolean>;
baz: ParseResultsValueMultipleUnknown<boolean>;
}
vs
type ExpectedValues = {
foo: string | undefined;
bar: boolean[] | undefined;
baz: boolean[] | boolean | undefined;
}
Totally! This hit to readability also occurs in the usage...
Perhaps it's best to scrap the ParseResults*
utilities. I mainly initialized them to establish a relationship between the results
inference types and the test cases and avoid human error (i.e. typos).
Here is an update removing the extraneous ParseResults*
utilities.
Note: If folks want to test these types locally simply copy/paste the entire playground into an index.d.ts
file at the root of the parseArgs
repo and add this minimal tsconfig.json
:
{
"compilerOptions": {
"strict": true,
"allowJs": true,
"checkJs": true
}
}
The visible type names might need to be more qualified to disambiguate when used in node:util
?
For example, currently have:
type ConfigOptions = { [longOption: string]: OptionConfig };
(Noticed when testing using copy-paste instructions from above, thanks.)
@bakkot Great job!
It looks like v18 of @types/node
is already published. Perhaps it’s time to add types for parseArgs()
as well?
@mrazauskas I'm waiting on https://github.com/pkgjs/parseargs/pull/129 / https://github.com/nodejs/node/pull/43459 to land so we can do the types all at once. But if that stalls much longer I'll open a PR to DefinitelyTyped, yeah.
OK, node PR landed. Types need to be updated to recognize the optional boolean in the config and to support the new tokens
property in the output (which should be omitted if the tokens
boolean is definitely absent or is false
, present if it is definitely true
, and otherwise optional). The type for tokens
should be something like
type Token =
| { kind: 'option', index: number, name: string, rawName: string, value: string | undefined, inlineValue: boolean | undefined }
| { kind: 'positional', index: number, value: string }
| { kind: 'option-terminator', index: number }
type Tokens = Token[]
It's actually possible to do way better than that for option tokens if the options
config is passed; I will play with it a bit and see how good I can get it.
Opened https://github.com/DefinitelyTyped/DefinitelyTyped/pull/61512 with the types roughly as discussed here (except with good types for tokens
). Would appreciate any reviews on that PR.
A whole lot of accurate typing assistance, thanks @aaronccasanova and @bakkot .
parseArgs
is now available in @types/node@18
.
A PR has been opened to copy these for @pkgjs/parseargs
:
Nothing for node 16 yet.
The new default
in the input option configuration has not been added to types yet.
Edit: simple type for default
added for node 18 in https://github.com/DefinitelyTyped/DefinitelyTyped/pull/62717/files
I have a prototype implementation of types for this package, with some type-level shenanigans so that you get exact types on the result as long as you pass the config directly to parseArgs. E.g.:
Any feedback before I submit them upstream? You can play around with the types in the playground linked, no need to install anything.