feat(node): types for util.parseArgs

[bakkot]

• Use a meaningful title for the pull request. Include the name of the package modified.
• Test the change in your own code. (Compile and run.)
• Add or edit tests to reflect the change.
• Follow the advice from the readme.
• Avoid common mistakes.
• Run npm test <package to test>.

Select one of these and delete the others:

If changing an existing definition:

• Provide a URL to documentation or source code which provides context for the suggested changes: util: add parseArgs module nodejs/node#42675 / util: add tokens to parseArgs nodejs/node#43459
• If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.

(actually there's a few other additions in 18.7, but I didn't feel up to adding those in this PR.

---

This PR has a lot of type-level logic used to get actually useful types from the new API. I don't know what this project's policy on those is. You can read the thread where we iterated on these at pkgjs/parseargs#127.

[typescript-bot]

@bakkot Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through.

This is a live comment which I will keep updated.

1 package in this PR

• node — on npm, on unpkg
  • owner-approval: @peterblazejewicz

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

You can test the changes of this PR in the Playground.

Status

• ✅ No merge conflicts
• ✅ Continuous integration tests have passed
• ✅ Most recent commit is approved by a DT maintainer

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

---

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 61512,
  "author": "bakkot",
  "headCommitOid": "73c880949180d158447f755df2c36b204a1d8366",
  "mergeBaseOid": "e7822af4aa254a890bc14fd407b856b77a587063",
  "lastPushDate": "2022-08-10T06:29:37.000Z",
  "lastActivityDate": "2022-08-10T19:00:09.000Z",
  "mergeOfferDate": "2022-08-10T18:57:08.000Z",
  "mergeRequestDate": "2022-08-10T19:00:09.000Z",
  "mergeRequestUser": "bakkot",
  "hasMergeConflict": false,
  "isFirstContribution": true,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "node",
      "kind": "edit",
      "files": [
        {
          "path": "types/node/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/test/util.ts",
          "kind": "test"
        },
        {
          "path": "types/node/util.d.ts",
          "kind": "definition"
        }
      ],
      "owners": [
        "Microsoft",
        "DefinitelyTyped",
        "jkomyno",
        "alvis",
        "r3nya",
        "btoueg",
        "smac89",
        "touffy",
        "DeividasBakanas",
        "eyqs",
        "Hannes-Magnusson-CK",
        "hoo29",
        "kjin",
        "ajafff",
        "islishude",
        "mwiktorczyk",
        "mohsen1",
        "n-e",
        "galkin",
        "parambirs",
        "eps1lon",
        "SimonSchick",
        "ThomasdenH",
        "WilcoBakker",
        "wwwy3y3",
        "samuela",
        "kuehlein",
        "bhongy",
        "chyzwar",
        "trivikr",
        "yoursunny",
        "qwelias",
        "ExE-Boss",
        "peterblazejewicz",
        "addaleax",
        "victorperin",
        "ZYSzys",
        "NodeJS",
        "LinusU",
        "wafuwafu13",
        "mcollina"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "peterblazejewicz",
      "date": "2022-08-10T18:58:34.000Z",
      "isMaintainer": true
    },
    {
      "type": "approved",
      "reviewer": "rbuckton",
      "date": "2022-08-10T18:56:24.000Z",
      "isMaintainer": true
    },
    {
      "type": "approved",
      "reviewer": "shadowspawn",
      "date": "2022-08-10T06:52:46.000Z",
      "isMaintainer": false
    },
    {
      "type": "stale",
      "reviewer": "r3nya",
      "date": "2022-08-04T05:39:45.000Z",
      "abbrOid": "4b66236"
    },
    {
      "type": "stale",
      "reviewer": "mcollina",
      "date": "2022-08-03T20:58:53.000Z",
      "abbrOid": "4b66236"
    },
    {
      "type": "stale",
      "reviewer": "aaronccasanova",
      "date": "2022-07-30T20:29:57.000Z",
      "abbrOid": "3d72450"
    },
    {
      "type": "stale",
      "reviewer": "SimonSchick",
      "date": "2022-07-30T06:08:45.000Z",
      "abbrOid": "f55bc7a"
    }
  ],
  "mainBotCommentID": 1200064815,
  "ciResult": "pass"
}

[typescript-bot]

🔔 @microsoft @DefinitelyTyped @jkomyno @alvis @r3nya @btoueg @smac89 @Touffy @DeividasBakanas @eyqs @Hannes-Magnusson-CK @hoo29 @kjin @ajafff @islishude @mwiktorczyk @mohsen1 @n-e @galkin @parambirs @eps1lon @SimonSchick @ThomasdenH @WilcoBakker @wwwy3y3 @samuela @kuehlein @bhongy @chyzwar @trivikr @yoursunny @qwelias @ExE-Boss @peterblazejewicz @addaleax @victorperin @ZYSzys @nodejs @LinusU @wafuwafu13 @mcollina — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

[SimonSchick]

As indicated in your own comment; I'm a little worried about the complexity of these type defs, this is some really advanced stuff and I had to read it over about 8 times before I understood what is going on.

I wonder if a simpler approach would be beneficial both for maintainers and consumers.

[bakkot]

If you can suggest a simpler approach which will make

let { values } = parseArgs({ options: { x: { type: 'string' }, m: { type: 'boolean', multiple: true } } });
let x: string | undefined = values.x;
let m: boolean[] = values.m ?? [];
// @ts-expect-error `y` was not one of the configured options
let y = values.y;

typecheck (and similarly for other parts), I'd be very happy to adopt it! Otherwise, though, I think this complexity is worth it for consumers; having less precise types here would be really annoying.

[peterblazejewicz]

I agree with Simon here, thx!

[rbuckton]

If you are willing to limit the complexity for positionals and tokens, a strongly typed values can be achieved far more simply:

// NOTE: Uses ParseArgsConfig, ParseArgsOptionsConfig, and ParseArgsOptionConfig as defined in PR

type ParseArgsValue<T extends ParseArgsOptionConfig> =
    T extends { multiple: true } ? ParseArgsValue<Omit<T, 'multiple'>>[] :
    T extends { type: 'string' } ? string :
    T extends { type: 'boolean' } ? boolean :
    string | boolean;

type ParseArgsValues<T extends ParseArgsConfig> =
    T extends { strict: true } ?
        { -readonly [P in keyof T['options'] & string]?: ParseArgsValue<NonNullable<T['options']>[P]>; } :
        & Partial<Record<string, ParseArgsValue<any>>
        & Record<keyof T['options'] & string, ParseArgsValue<any>>>;
        // NOTE: `ParseArgsValue<any>` is `string | boolean | (string | boolean)[]`

type ParseArgsToken =
    | { kind: 'option', index: number, name: string, rawName: string, value: string, inlineValue: boolean }
    | { kind: 'option', index: number, name: string, rawName: string, value: undefined, inlineValue: undefined }
    | { kind: 'option-terminator', index: number }
    | { kind: 'positional', index: number, value: string };

type ParseArgsResults<T extends ParseArgsConfig> = {
    values: ParseArgsValues<T>;
    positionals: string[];
    tokens: T extends { tokens: true } ? ParseArgsToken[] : undefined;
};

export function parseArgs<T extends ParseArgsConfig>(config: T): ParseArgsResults<T>;

[SimonSchick]

Apparently the spec for this is still in flux, we should probably hold off or use a simpler implementation until this has stabilized for a (minor) version or 2?

See https://nodejs.org/api/util.html#utilparseargsconfig

[typescript-bot]

@bakkot One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!

[bakkot]

Apparently the spec for this is still in flux

No, it should be settled now; that's why I waited until now to submit this PR. Development was happening at https://github.com/pkgjs/parseargs and the tokens feature added in 18.7.0 was the last thing that was planned.

[typescript-bot]

@SimonSchick Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

[mcollina]

lgtm

[shadowspawn]

There is a lot of magic in the typings, but just two tests to either show them off or test they are working as intended.

Is it reasonable to have more complete unit test style coverage, or are DefinitelyTyped tests or node tests usually fairly high level?

(Tests can be added later, so this is not a blocker! But would give gentle readers here more insight into the benefits of the typings without mentally parsing the magic. 😓 Or typing things into a playground or context, which I am off to do more... )

(Edit: I tried adding some simple tests. Crikey, disappointingly tricky and noisy getting $ExpectType to check types as seen in editor. Apparently losing the const of in-line literals, and then lint suppression for the as const as seen in the current PR.)

[bakkot]

Crikey, disappointingly tricky and noisy getting $ExpectType to check types as seen in editor.

Yeah that's why I didn't add more 😅 . Especially since it doesn't always expand out all of the generics and aliases (which makes the test useless), plus equality is apparently based on the literal text of the rendered type, so you have to get the unions in the right order and use the same quotes and everything.

I pushed up a few more for the most common cases, though. If there's more common cases you think worth testing let me know and I can try to come up with readable tests.

[shadowspawn]

I went through all my playground tests again, and all still passed.

[typescript-bot]

@rbuckton, @r3nya, @mcollina, @peterblazejewicz, @aaronccasanova, @SimonSchick Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

[rbuckton]

How does this affect parseArgs({})? Since all properties are optional on ParseArgsConfig, it is essentially a subtype of {}, which would result in the weakly typed result as opposed to the precise result.

[bakkot]

parseArgs({}) is basically useless - it amounts to "I want to reject any positional arguments and any non-configured options, and I have no configured options". So that invocation will reject any non-empty arguments list.

So I'm not really expecting this to come up in practice, and as such I'm OK with the less precise types in this case.

But if there's a better way to special-case parseArgs({}) while preserving the mainline cases, I'm happy to take suggestions.

(I guess I could add another branch here for T extends { [n: string]: never } which gives the precise types for the empty config?)

Edit: actually, I've just recalled that an earlier version did have this as a special case, and I cut it out in the name of reducing complexity. I can re-introduce it if you think it's warranted.

[rbuckton]

I'd rather not make this even more complicated than it already is.

While it would be nice if the less precise type just fell out from the existing definitions, I'm pretty sure addressing that would just add even more complexity.

[typescript-bot]

@r3nya, @mcollina, @peterblazejewicz, @aaronccasanova, @SimonSchick Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

[peterblazejewicz]

@rbuckton thanks!

[typescript-bot]

@r3nya, @mcollina, @aaronccasanova, @SimonSchick Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

[bakkot]

Ready to merge

[bakkot]

Thanks for shepherding me through this, all!

[thw0rted]

I thought that this package wasn't adding types for "Experimental" ("Stability - 1") APIs? The docs still call this feature Experimental. Is there a written policy somewhere about when an API should be added?

[shadowspawn]

The related commentary I have seen is that suitable types for end-users is a technical priority.

• https://github.com/nodejs/node/blob/HEAD/doc/contributing/technical-priorities.md#suitable-types-for-end-users

[thw0rted]

Thanks @shadowspawn . My previous comment was more from the perspective of the DT node package maintainers. Your link shows the Node team's priorities -- are some of the DT maintainers affiliated with Node? Or do you think the paragraph you linked is a good justification for adding (certain?) experimental APIs?

I'm asking because, in the process of working on this pretty significant PR I've been faced with the choice of which global DOM-like APIs to expose. I was going by the guiding principle that I should not add interfaces that are still labeled experimental, but I can't actually find a reference in writing to back that up.

Maybe we need a meta-issue (or Discussion) here to establish a baseline for when a given API should be added? Just to be clear, I wasn't complaining that about this addition, I was just trying to ensure consistency in the approach used by various contributors.

[shadowspawn]

I suspect there are mixed viewpoints! I see the benefit in types to match the implementation, but chasing a moving target can be wasted effort. There was a query earlier in this thread whether the api was in flux. This thread has the authors of the code, so hopefully the balance is right.

[bakkot]

FWIW there's types for lots of other experimental node APIs already - e.g. the new-in-v18 test runner, which is also experimental.

I think it's really valuable to have these types because the whole point of shipping an API marked as "experimental" is that we want people to actually start using it in real life so we can see if there's any sharp edges, and for TypeScript projects that means such an API needs to have types. For many people the types are an important part of the experience, after all.