util: add default value option to parsearg by Eomm · Pull Request #44631 · nodejs/node

Eomm · 2022-09-13T22:54:21Z

This PR adds to the parseArgs utility feature a new default input field option.
The developer will be able to define a default value to retrieve when the input arguments are processed.

This PR has a downstream to pkgjs/parseargs#142

MoLow · 2022-09-14T04:41:45Z

doc/api/util.md

      `false`, values for the option are last-wins. **Default:** `false`.
    * `short` {string} A single character alias for the option.
+    * `default` {string | boolean | string[] | boolean[]} The default option
+      value when it is not set by args.


Document that this must match the arg type?

Done, adding the multiple case too

mcollina

lgtm

lib/internal/util/parse_args/utils.js

bakkot · 2022-09-22T22:14:15Z

After thinking on it for a while, I think undefined should be a legal value for default, and should be treated exactly like default being missing. (So, in particular, setting default: undefined should not cause the key to appear in the values object.) cc @shadowspawn since you expressed the opposite opinion above.

Specifically, accepting undefined makes it easier to do stuff like

let options = { endpoint: { type: 'string', default: process.env.MY_PROJECT_ENDPOINT } };
let { values } = parseArgs({ options });

which is a convenient pattern, especially since this utility almost certainly doesn't want to handle this sort of default-to-env behavior itself.

If undefined isn't allowed, then you would have to write something like

let options = { endpoint: { type: 'string' } };
if (process.env.MY_PROJECT_ENDPOINT != null) {
  opts.endpoint.default = process.env.MY_PROJECT_ENDPOINT;
}
let { values } = parseArgs({ options });

which is much less pleasant all around.

tniessen · 2022-09-23T09:37:55Z

Does this have the same semantics as destructuring with defaults?

let { values: { foo = 'default' } } = util.parseArgs({ options: { foo: { type: 'string' } } })

shadowspawn · 2022-09-24T01:35:37Z

After thinking on it for a while, I think undefined should be a legal value for default, and should be treated exactly like default being missing. (So, in particular, setting default: undefined should not cause the key to appear in the values object.)

The env use case seems pretty reasonable and convenient. I like it.

cc @shadowspawn since you expressed the opposite opinion [above (https://github.com//pull/44631#discussion_r971695523).

My comment was in context of the commented code, that I didn't see a need to treat undefined as a valid value for output values. So not in opposition to your suggestion that is useful to allow for input config.

mcollina

lgtm

Eomm · 2022-09-24T07:09:35Z

If undefined isn't allowed, then you would have to write something like

Fair usage, I will update this PR 👍🏽

Does this have the same semantics as destructuring with defaults?

Yes, it does. Doing so would be like moving the "default burden logic" to the caller instead of callee, and this is unwanted in my opinion.

shadowspawn

LGTM (not a collaborator)

aduh95 · 2022-09-24T12:33:11Z

doc/api/util.md

 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/44631
+    description: add support for default values in input `config`.


Suggested change

description: add support for default values in input `config`.

description: Add support for default values in input `config`.

aduh95 · 2022-09-24T12:38:09Z

lib/internal/util/parse_args/parse_args.js

+        if (optionType === 'string' && !multipleOption) {
+          validateString(defaultValue, `options.${longOption}.default`);
+        } else if (optionType === 'string' && multipleOption) {
+          validateStringArray(defaultValue, `options.${longOption}.default`);
+        } else if (optionType === 'boolean' && !multipleOption) {
+          validateBoolean(defaultValue, `options.${longOption}.default`);
+        } else if (optionType === 'boolean' && multipleOption) {
+          validateBooleanArray(defaultValue, `options.${longOption}.default`);
+        }


nit: we could avoid the code repetition here.

Suggested change

if (optionType === 'string' && !multipleOption) {

validateString(defaultValue, `options.${longOption}.default`);

} else if (optionType === 'string' && multipleOption) {

validateStringArray(defaultValue, `options.${longOption}.default`);

} else if (optionType === 'boolean' && !multipleOption) {

validateBoolean(defaultValue, `options.${longOption}.default`);

} else if (optionType === 'boolean' && multipleOption) {

validateBooleanArray(defaultValue, `options.${longOption}.default`);

}

let validator;

switch(optionType) {

case 'string':

validator = multipleOption ? validateStringArray : validateString;

break;

case 'boolean':

validator = multipleOption ? validateBooleanArray : validateBoolean;

break;

}

validator(defaultValue, `options.${longOption}.default`);

Eomm · 2022-09-26T09:50:31Z

Completed all the feedbacks

Not sure about the failed CI check: should I rebase this branch?

Users can set a default value for every expected input argument

nodejs-github-bot · 2022-09-30T17:08:31Z

CI: https://ci.nodejs.org/job/node-test-pull-request/46960/

nodejs-github-bot · 2022-10-01T10:32:42Z

CI: https://ci.nodejs.org/job/node-test-pull-request/46970/

mcollina

lgtm

shadowspawn · 2022-10-15T05:44:12Z

This PR has landed in v18.11.0

parseArgs has also been included in Node.js v16. Is there any additional work require to land this PR in v16, so default is also available there, or will it happen automatically?

aduh95 · 2022-10-15T13:07:29Z

Node.js 16.x is going into maintenance mode next tuesday (see https://github.com/nodejs/Release#release-schedule), and there are no scheduled release until then, so PRs won't be "automatically" backported anymore. That means someone would need to open a backport PR for this to land on v16.x.

shadowspawn · 2022-10-15T23:09:31Z

Thanks for info @aduh95

Light question: might adding default support be acceptable as a back port at this late stage? (i.e. v16.x not in maintenance mode yet, but will be very soon!)

aduh95 · 2022-10-16T01:56:02Z

It should be, as long as someone opens a backport PR, see https://github.com/nodejs/node/blob/0f9087971c085ea0c93b9fd867e442afe11c0ae0/doc/contributing/backporting-to-release-lines.md for more info.

targos · 2022-10-16T08:13:26Z

There doesn't have to be a backport PR. Another option (especially if the commit can be cherry-picked without conflicts) is to add the lts-watch-v16.x label.

nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Sep 13, 2022

MoLow reviewed Sep 14, 2022

View reviewed changes

Eomm force-pushed the parseargs-default-value branch from 5196cdd to 0b001a3 Compare September 14, 2022 05:58

mcollina approved these changes Sep 14, 2022

View reviewed changes

lpinca added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 14, 2022

aduh95 reviewed Sep 15, 2022

View reviewed changes

lib/internal/util/parse_args/utils.js Show resolved Hide resolved

mcollina approved these changes Sep 24, 2022

View reviewed changes

shadowspawn approved these changes Sep 24, 2022

View reviewed changes

aduh95 reviewed Sep 24, 2022

View reviewed changes

Eomm added 5 commits September 29, 2022 21:36

util: add default value option to parsearg

d625501

Users can set a default value for every expected input argument

chore: fix linting

0c50213

chore: fix markdown linting

40722aa

fix: default option accepts undefined as value

21fd4fd

fix: code repetition per code review

eb0c843

Eomm force-pushed the parseargs-default-value branch from 1a7aae6 to eb0c843 Compare September 29, 2022 19:37

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2022

mcollina approved these changes Oct 2, 2022

View reviewed changes

jasnell approved these changes Oct 2, 2022

View reviewed changes

mcollina added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed needs-ci PRs that need a full CI run. labels Oct 7, 2022

targos added the lts-watch-v16.x label Oct 16, 2022

Eomm deleted the parseargs-default-value branch October 16, 2022 14:04

This was referenced Oct 17, 2022

util: parseArgs should support required in options[key] #44564

Closed

doc: fix typo in parseArgs default value #45083

Merged

richardlau added backported-to-v16.x and removed lts-watch-v16.x labels Nov 24, 2022

richardlau mentioned this pull request Dec 8, 2022

v16.19.0 proposal #45791

Merged

shadowspawn mentioned this pull request Feb 18, 2024

node compat: parseArgs doesn't support default option denoland/deno#22454

Closed

opalmedapps-bot mentioned this pull request Jan 28, 2025

chore: update node.js to v16.19.0 opalmedapps/docs#55

Closed

1 task

	description: add support for default values in input `config`.
	description: Add support for default values in input `config`.

Conversation

Eomm commented Sep 13, 2022

Uh oh!

MoLow Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

Eomm Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bakkot commented Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tniessen commented Sep 23, 2022

Uh oh!

shadowspawn commented Sep 24, 2022

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Eomm commented Sep 24, 2022

Uh oh!

shadowspawn left a comment

Choose a reason for hiding this comment

Uh oh!

aduh95 Sep 24, 2022

Choose a reason for hiding this comment

Uh oh!

aduh95 Sep 24, 2022

Choose a reason for hiding this comment

Uh oh!

Eomm commented Sep 26, 2022

Uh oh!

nodejs-github-bot commented Sep 30, 2022

Uh oh!

nodejs-github-bot commented Oct 1, 2022

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

shadowspawn commented Oct 15, 2022

Uh oh!

aduh95 commented Oct 15, 2022

Uh oh!

shadowspawn commented Oct 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Oct 16, 2022

Uh oh!

targos commented Oct 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

bakkot commented Sep 22, 2022 •

edited

Loading

shadowspawn commented Oct 15, 2022 •

edited

Loading