Skip to content
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

TypeScript types #127

Open
bakkot opened this issue May 26, 2022 · 28 comments
Open

TypeScript types #127

bakkot opened this issue May 26, 2022 · 28 comments
Labels
help wanted Extra attention is needed

Comments

@bakkot
Copy link
Collaborator

bakkot commented May 26, 2022

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.:

let { values, positionals } = parseArgs({
  options: {
    foo: { type: 'string', multiple: true },
    bar: { type: 'boolean' },
    evil: { type: 'string', multiple: Math.random() < 0.5 },
  },
});
let foo: string[] = values.foo; // works
let bar: boolean | undefined = values.bar; // works
let evil: string[] | string | undefined = values.evil; // if we can't precisely infer `multiple`, we can't say if it's an array or not
positionals[0]; // error: since we did not explicitly specify `allowPositionals: true`, the `positionals` array must be empty

Any feedback before I submit them upstream? You can play around with the types in the playground linked, no need to install anything.

@shadowspawn
Copy link
Collaborator

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.

@bcoe bcoe added the help wanted Extra attention is needed label May 27, 2022
@bcoe
Copy link
Collaborator

bcoe commented May 27, 2022

@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?

@ljharb
Copy link
Member

ljharb commented May 27, 2022

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.

@bakkot
Copy link
Collaborator Author

bakkot commented May 28, 2022

@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?

@ljharb
Copy link
Member

ljharb commented May 28, 2022

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.

@shadowspawn
Copy link
Collaborator

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.

@aaronccasanova
Copy link
Collaborator

aaronccasanova commented May 29, 2022

Nice work @bakkot!! I copied your TS Playground and included some light suggestions, specifically:

  • Copy/pasted the Exact and IsEqual utility types from type-fest
    • Use Exact to error on unknown config properties
    • Use IsEqual to infer various boolean configs e.g. IsStrict, IsAllowPositionals, IsMultiple
  • Restructured all type conditions to avoid negated types (e.g. IsStrict extends true : true-branch : false-branch)
  • Tentatively renamed inference types as it is common to type import them in scripts and imo would be a good idea to establish a solid naming convention

Additionally, 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.

@bakkot
Copy link
Collaborator Author

bakkot commented May 29, 2022

@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.

@aaronccasanova
Copy link
Collaborator

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:

image

Specifically, evil is now inferred as string | undefined, whereas the correct type is string[] | string | undefined

I'm seeing the correct types.

image

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.

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.

@bakkot
Copy link
Collaborator Author

bakkot commented May 29, 2022

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.

@aaronccasanova
Copy link
Collaborator

image

Oh I see! Hmm now I'm wondering if that applies to all the Extract default utilities in my prototype.

@bakkot
Copy link
Collaborator Author

bakkot commented May 29, 2022

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.

@aaronccasanova
Copy link
Collaborator

Welp, I totally understand the rationale for IfDefaultsTrue and IfDefaultsFalse now! 👍 I love the composition in ParsedPositionals:
image

I tested every permutation and it's very robust 👏

Here is an updated prototype reverting the IsEqual/Extract utilties. This version includes:

  • Updated naming conventions
  • Errors for unknown config properties
  • Proposed type fix for values.foo when the option is not provided to the CLI

As mentioned above, these are light suggestions so feel free to dismiss 🙂

@bakkot
Copy link
Collaborator Author

bakkot commented May 29, 2022

Nice, LGTM.

@shadowspawn
Copy link
Collaborator

shadowspawn commented May 30, 2022

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:

Screen Shot 2022-05-30 at 19 03 17

Whereas currently for parseArgs I see raw:

Screen Shot 2022-05-30 at 19 05 39

@bakkot
Copy link
Collaborator Author

bakkot commented May 30, 2022

@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.
 * 
 */

@aaronccasanova
Copy link
Collaborator

Took another pass and caught two more edge cases:

  • An empty 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
}

Screen Shot 2022-05-30 at 10 15 34 AM

@aaronccasanova
Copy link
Collaborator

aaronccasanova commented May 30, 2022

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).

@bakkot
Copy link
Collaborator Author

bakkot commented May 30, 2022

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;
}

@aaronccasanova
Copy link
Collaborator

Totally! This hit to readability also occurs in the usage...
image

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.

image

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
  }
}

@shadowspawn
Copy link
Collaborator

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.)

@mrazauskas
Copy link

@bakkot Great job!

It looks like v18 of @types/node is already published. Perhaps it’s time to add types for parseArgs() as well?

@bakkot
Copy link
Collaborator Author

bakkot commented Jul 4, 2022

@mrazauskas I'm waiting on #129 / nodejs/node#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.

@bakkot
Copy link
Collaborator Author

bakkot commented Jul 28, 2022

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.

@bakkot
Copy link
Collaborator Author

bakkot commented Jul 30, 2022

Opened DefinitelyTyped/DefinitelyTyped#61512 with the types roughly as discussed here (except with good types for tokens). Would appreciate any reviews on that PR.

@shadowspawn
Copy link
Collaborator

A whole lot of accurate typing assistance, thanks @aaronccasanova and @bakkot .

@shadowspawn
Copy link
Collaborator

shadowspawn commented Aug 29, 2022

parseArgs is now available in @types/node@18.

A PR has been opened to copy these for @pkgjs/parseargs:

Nothing for node 16 yet.

@shadowspawn
Copy link
Collaborator

shadowspawn commented Oct 15, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

6 participants