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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

util: add parseArgs module #42675

Merged
merged 2 commits into from May 24, 2022
Merged

util: add parseArgs module #42675

merged 2 commits into from May 24, 2022

Conversation

bcoe
Copy link
Member

@bcoe bcoe commented Apr 9, 2022

Adds util.parseArgs helper for higher level command-line argument parsing.


Background

There has been an ongoing effort in pkgjs/parseargs to define a command-line argument parsing API for Node.js core.

The effort has taken several years to get to this point, with many amazing contributors across a variety of companies and projects: IBM, Microsoft, Google, Shopify, npm, nvm, commander, yargs, just to name a few.

Where parseArgs is at

@shadowspawn, who's been most recently pushing a lot of the work on parseArgs forward, indicates in pkgjs/parseargs/issues/87 that "we're close to having a decent MVP."

I agree, and thought it would be worth having a candidate PR open on Node.js to get additional feedback.

This is a controversial topic, be kind

There's a lot of nuance and opinion, as to what constitutes a "decent MVP" for a command-line argument parser. Please be respectful of opinions you might not agree with 馃槉

Before leaving feedback on this thread, I suggest reading past issues on parseArgs. Perhaps there's already been a discussion that touches on your feedback (or there's an ongoing discussion).

Similarly, if themes of discussion jump out in this PR, let's move them to an issue in parseArgs (this should help keep this PR less confusing).

ToDo

CC: @ljharb, @bakkot, @iansu, @darcyclarke, @ruyadorno, @shadowspawn, @nodejs/tooling, @Trott (who I believe asked after this), @aaronccasanova.

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Apr 9, 2022

Review requested:

@nodejs-github-bot nodejs-github-bot added lib / src needs-ci labels Apr 9, 2022
@mcollina
Copy link
Member

@mcollina mcollina commented Apr 9, 2022

I would recommend this to be exposed as node:parseargs instead (or analog in the node: prefix). process is already overloaded with so much functionality, I would prefer not adding new things there that could stay separate.

doc/api/process.md Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

@ljharb ljharb commented Apr 9, 2022

It definitely should not be exposed only under the prefix, for a ton of reasons (but that鈥檚 being discussed elsewhere)

doc/api/errors.md Outdated Show resolved Hide resolved
doc/api/process.md Outdated Show resolved Hide resolved
doc/api/process.md Outdated Show resolved Hide resolved
lib/internal/process/parse_args/utils.js Outdated Show resolved Hide resolved
lib/internal/process/parse_args/utils.js Outdated Show resolved Hide resolved
lib/internal/process/parse_args/utils.js Outdated Show resolved Hide resolved
lib/internal/process/parse_args/utils.js Outdated Show resolved Hide resolved
lib/internal/process/parse_args/utils.js Outdated Show resolved Hide resolved
lib/internal/process/parse_args/utils.js Outdated Show resolved Hide resolved
lib/internal/process/parse_args/utils.js Outdated Show resolved Hide resolved
lib/internal/process/parse_args/utils.js Outdated Show resolved Hide resolved
lib/internal/process/parse_args/utils.js Outdated Show resolved Hide resolved
test/parallel/test-parse-args.mjs Outdated Show resolved Hide resolved
@bcoe
Copy link
Member Author

@bcoe bcoe commented Apr 10, 2022

I would recommend this to be exposed as node:parseargs

@mcollina done, opted for node:parse_args.

@bnb
Copy link
Member

@bnb bnb commented Apr 10, 2022

I am -1 on it being exposed only under the node: prefix. I do not much mind what it's called, but I'd strongly prefer that we do not limit new additions to that unless we're willing to fully break and force it for all modules, retroactively, and force all core modules under it.

I am, however, +1 on this landing assuming we can come to consensus on it.

@aaronccasanova
Copy link

@aaronccasanova aaronccasanova commented Apr 10, 2022

My assumption thus far was that parseArgs would be exposed from the util module. However, I can see process.parseArgs() being a common replacement for process.argv and I think it would feel very intuitive for folks. That being said, I have no strong preference and would be happy with any consensus.

Copy link
Contributor

@cjihrig cjihrig left a comment

Left a few comments.

Regarding the node: scheme - there is a vote on this in nodejs/TSC#1206, which is expected to be completed Monday (so a decision can be made in time for Node 18). I think the outcome there should dictate how this module is exposed.

doc/api/parse_args.md Outdated Show resolved Hide resolved
lib/internal/errors.js Outdated Show resolved Hide resolved
doc/api/parse_args.md Outdated Show resolved Hide resolved
test/parallel/test-parse-args.mjs Outdated Show resolved Hide resolved
test/parallel/test-parse-args.mjs Outdated Show resolved Hide resolved
lib/parse_args.js Outdated Show resolved Hide resolved
lib/internal/parse_args/parse_args.js Outdated Show resolved Hide resolved
lib/internal/parse_args/parse_args.js Outdated Show resolved Hide resolved
doc/api/parse_args.md Outdated Show resolved Hide resolved
@bcoe
Copy link
Member Author

@bcoe bcoe commented Apr 10, 2022

I am -1 on it being exposed only under the node: prefix. I do not much mind what it's called, but I'd strongly prefer that we do not limit new additions to that unless we're willing to fully break and force it for all modules

@cjihrig @mcollina @bnb, can we agree that we will have the functionality available in node:parse_args, this would allow us to get this PR to an MVP with this assumption -- and if we then decide to also expose the feature in process or util, it's a non breaking addition.

@mcollina
Copy link
Member

@mcollina mcollina commented Apr 10, 2022

I'm totally ok for it to stay inside util, I'm just not ok with using process.

@bcoe
Copy link
Member Author

@bcoe bcoe commented Apr 10, 2022

@cjihrig @mcollina @Trott how would folks feel about this feature experimental in docs only? with strong warnings in the documentation that this is a work in progress?

On one hand, a runtime experimental warning makes it difficult to create real-world command line applications, as they'll have noisy output.

On the other hand, users might not realize it's experimental and get frustrated if the API breaks.

@mcollina
Copy link
Member

@mcollina mcollina commented Apr 10, 2022

I'm ok in being doc-only experimental.

@Trott
Copy link
Member

@Trott Trott commented Apr 10, 2022

@cjihrig @mcollina @Trott how would folks feel about this feature experimental in docs only? which strong warnings in the documentation that this is a work in progress?

I'd punt that to @nodejs/tsc and @nodejs/releasers. I'm personally fine with it, but I also wouldn't want that to sneak by anyone.

I'm not sure if this is actually feasible, but maybe we can split the difference and have a warning be emitted (so if anything is listening for warnings, it picks it up) but ignored by the default "print the warning to the console" listener.

lib/internal/parse_args/utils.js Outdated Show resolved Hide resolved
@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 10, 2022

would folks feel about this feature experimental in docs only?

I don't mind due to the nature of this API.

@bcoe bcoe changed the title process: add parseArgs method lib: add parseArgs module Apr 11, 2022
@bcoe bcoe added notable-change tsc-agenda labels Apr 11, 2022
@targos targos added the semver-minor label Apr 11, 2022
@bcoe
Copy link
Member Author

@bcoe bcoe commented Apr 11, 2022

@nodejs/tsc two questions I would love feedback on from the TSC:

  1. would it be okay to expose this feature as util.parseArgs, as well as in the module node:parse_args?
  2. would it be okay to indicate that this feature is experimental in docs only (which would make it easier for people to test the API for real world applications), or should it have a runtime experimental warning.

@RaisinTen
Copy link
Member

@RaisinTen RaisinTen commented Apr 11, 2022

  1. would it be okay to expose this feature as util.parseArgs, as well as in the module node:parse_args?

I think it's okay to expose this as a part of the util module. It probably doesn't require a completely new module of its own because it's just a single function.

  1. would it be okay to indicate that this feature is experimental in docs only (which would make it easier for people to test the API for real world applications), or should it have a runtime experimental warning.

I'm okay with it.

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented May 23, 2022

@bcoe
Copy link
Member Author

@bcoe bcoe commented May 23, 2022

Fixes pkgjs/parseargs#109

@Trott
Copy link
Member

@Trott Trott commented May 24, 2022

I think this is ready to land, but it could use one more approval or re-approval since changes have been made since the last approval (and the commit queue won't merge something in that case). I'll rubber-stamp re-approve if no one wants to give it a proper review, but hopefully someone would like to look at it one more time?

Copy link
Member

@mcollina mcollina left a comment

lgtm

@Trott Trott added commit-queue-squash commit-queue labels May 24, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue label May 24, 2022
@nodejs-github-bot nodejs-github-bot merged commit 4a3ba87 into nodejs:master May 24, 2022
50 checks passed
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented May 24, 2022

Landed in 4a3ba87

bengl pushed a commit that referenced this issue May 30, 2022
Adds util.parseArgs helper for higher level command-line argument
parsing.

PR-URL: #42675
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruy Adorno <ruyadorno@github.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joe Sepi <sepi@joesepi.com>
Reviewed-By: Ian Sutherland <ian@iansutherland.ca>
bengl added a commit that referenced this issue May 31, 2022
Notable changes:

* deps: update undici to 5.3.0 (Node.js GitHub Bot) #43197
* (SEMVER-MINOR) util: add parseArgs module (Benjamin Coe) #42675
* (SEMVER-MINOR) http: add uniqueHeaders option to request and createServer (Paolo Insogna) #41397
* deps: upgrade npm to 8.11.0 (npm team) #43210
* deps: patch V8 to 10.2.154.4 (Micha毛l Zasso) #43067
* (SEMVER-MINOR) deps: update V8 to 10.2.154.2 (Micha毛l Zasso) #42740
* (SEMVER-MINOR) fs: make params in writing methods optional (LiviaMedeiros) #42601
* (SEMVER-MINOR) http: add uniqueHeaders option to request and createServer (Paolo Insogna) #41397
* (SEMVER-MINOR) net: add ability to reset a tcp socket (pupilTong) #43112

PR-URL: TBD
@bengl bengl mentioned this pull request May 31, 2022
bengl added a commit that referenced this issue May 31, 2022
Notable changes:

* deps: update undici to 5.3.0 (Node.js GitHub Bot) #43197
* (SEMVER-MINOR) util: add parseArgs module (Benjamin Coe) #42675
* (SEMVER-MINOR) http: add uniqueHeaders option to request and createServer (Paolo Insogna) #41397
* deps: upgrade npm to 8.11.0 (npm team) #43210
* deps: patch V8 to 10.2.154.4 (Micha毛l Zasso) #43067
* (SEMVER-MINOR) deps: update V8 to 10.2.154.2 (Micha毛l Zasso) #42740
* (SEMVER-MINOR) fs: make params in writing methods optional (LiviaMedeiros) #42601
* (SEMVER-MINOR) http: add uniqueHeaders option to request and createServer (Paolo Insogna) #41397
* (SEMVER-MINOR) net: add ability to reset a tcp socket (pupilTong) #43112

PR-URL: #43266
bengl added a commit that referenced this issue Jun 1, 2022
Notable changes:

* deps: update undici to 5.3.0 (Node.js GitHub Bot) #43197
* (SEMVER-MINOR) util: add parseArgs module (Benjamin Coe) #42675
* (SEMVER-MINOR) http: add uniqueHeaders option to request and createServer (Paolo Insogna) #41397
* deps: upgrade npm to 8.11.0 (npm team) #43210
* deps: patch V8 to 10.2.154.4 (Micha毛l Zasso) #43067
* (SEMVER-MINOR) deps: update V8 to 10.2.154.2 (Micha毛l Zasso) #42740
* (SEMVER-MINOR) fs: make params in writing methods optional (LiviaMedeiros) #42601
* (SEMVER-MINOR) http: add uniqueHeaders option to request and createServer (Paolo Insogna) #41397
* (SEMVER-MINOR) net: add ability to reset a tcp socket (pupilTong) #43112
* (SEMVER-MINOR) Revert "build: make x86 Windows support temporarily experimental" (Micha毛l Zasso) [#42740](#42740)
  * This means 32-bit Windows binaries are back with this release.

PR-URL: #43266
bengl added a commit that referenced this issue Jun 1, 2022
Notable changes:

* deps: update undici to 5.3.0 (Node.js GitHub Bot) #43197
* (SEMVER-MINOR) util: add parseArgs module (Benjamin Coe) #42675
* (SEMVER-MINOR) http: add uniqueHeaders option to request and createServer (Paolo Insogna) #41397
* deps: upgrade npm to 8.11.0 (npm team) #43210
* deps: patch V8 to 10.2.154.4 (Micha毛l Zasso) #43067
* (SEMVER-MINOR) deps: update V8 to 10.2.154.2 (Micha毛l Zasso) #42740
* (SEMVER-MINOR) fs: make params in writing methods optional (LiviaMedeiros) #42601
* (SEMVER-MINOR) http: add uniqueHeaders option to request and createServer (Paolo Insogna) #41397
* (SEMVER-MINOR) net: add ability to reset a tcp socket (pupilTong) #43112
* (SEMVER-MINOR) Revert "build: make x86 Windows support temporarily experimental" (Micha毛l Zasso) [#42740](#42740)
  * This means 32-bit Windows binaries are back with this release.

PR-URL: #43266
bengl added a commit that referenced this issue Jun 1, 2022
Notable changes:

* deps: update undici to 5.4.0  (Node.js GitHub Bot) #43262
* (SEMVER-MINOR) util: add parseArgs module (Benjamin Coe) #42675
* (SEMVER-MINOR) http: add uniqueHeaders option to request and createServer (Paolo Insogna) #41397
* deps: upgrade npm to 8.11.0 (npm team) #43210
* deps: patch V8 to 10.2.154.4 (Micha毛l Zasso) #43067
* (SEMVER-MINOR) deps: update V8 to 10.2.154.2 (Micha毛l Zasso) #42740
* (SEMVER-MINOR) fs: make params in writing methods optional (LiviaMedeiros) #42601
* (SEMVER-MINOR) http: add uniqueHeaders option to request and createServer (Paolo Insogna) #41397
* (SEMVER-MINOR) net: add ability to reset a tcp socket (pupilTong) #43112
* (SEMVER-MINOR) Revert "build: make x86 Windows support temporarily experimental" (Micha毛l Zasso) [#42740](#42740)
  * This means 32-bit Windows binaries are back with this release.

PR-URL: #43266
bengl added a commit that referenced this issue Jun 2, 2022
Notable changes:

* deps: update undici to 5.4.0  (Node.js GitHub Bot) #43262
* (SEMVER-MINOR) util: add parseArgs module (Benjamin Coe) #42675
* (SEMVER-MINOR) http: add uniqueHeaders option to request and createServer (Paolo Insogna) #41397
* deps: upgrade npm to 8.11.0 (npm team) #43210
* deps: patch V8 to 10.2.154.4 (Micha毛l Zasso) #43067
* (SEMVER-MINOR) deps: update V8 to 10.2.154.2 (Micha毛l Zasso) #42740
* (SEMVER-MINOR) fs: make params in writing methods optional (LiviaMedeiros) #42601
* (SEMVER-MINOR) http: add uniqueHeaders option to request and createServer (Paolo Insogna) #41397
* (SEMVER-MINOR) net: add ability to reset a tcp socket (pupilTong) #43112
* (SEMVER-MINOR) Revert "build: make x86 Windows support temporarily experimental" (Micha毛l Zasso) [#42740](#42740)
  * This means 32-bit Windows binaries are back with this release.

PR-URL: #43266
italojs pushed a commit to italojs/node that referenced this issue Jun 6, 2022
Notable changes:

* deps: update undici to 5.4.0  (Node.js GitHub Bot) nodejs#43262
* (SEMVER-MINOR) util: add parseArgs module (Benjamin Coe) nodejs#42675
* (SEMVER-MINOR) http: add uniqueHeaders option to request and createServer (Paolo Insogna) nodejs#41397
* deps: upgrade npm to 8.11.0 (npm team) nodejs#43210
* deps: patch V8 to 10.2.154.4 (Micha毛l Zasso) nodejs#43067
* (SEMVER-MINOR) deps: update V8 to 10.2.154.2 (Micha毛l Zasso) nodejs#42740
* (SEMVER-MINOR) fs: make params in writing methods optional (LiviaMedeiros) nodejs#42601
* (SEMVER-MINOR) http: add uniqueHeaders option to request and createServer (Paolo Insogna) nodejs#41397
* (SEMVER-MINOR) net: add ability to reset a tcp socket (pupilTong) nodejs#43112
* (SEMVER-MINOR) Revert "build: make x86 Windows support temporarily experimental" (Micha毛l Zasso) [nodejs#42740](nodejs#42740)
  * This means 32-bit Windows binaries are back with this release.

PR-URL: nodejs#43266
LiviaMedeiros pushed a commit to LiviaMedeiros/done that referenced this issue Jun 28, 2022
Adds util.parseArgs helper for higher level command-line argument
parsing.

PR-URL: nodejs/node#42675
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruy Adorno <ruyadorno@github.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joe Sepi <sepi@joesepi.com>
Reviewed-By: Ian Sutherland <ian@iansutherland.ca>
LiviaMedeiros pushed a commit to LiviaMedeiros/done that referenced this issue Jun 28, 2022
Notable changes:

* deps: update undici to 5.4.0  (Node.js GitHub Bot) nodejs/node#43262
* (SEMVER-MINOR) util: add parseArgs module (Benjamin Coe) nodejs/node#42675
* (SEMVER-MINOR) http: add uniqueHeaders option to request and createServer (Paolo Insogna) nodejs/node#41397
* deps: upgrade npm to 8.11.0 (npm team) nodejs/node#43210
* deps: patch V8 to 10.2.154.4 (Micha毛l Zasso) nodejs/node#43067
* (SEMVER-MINOR) deps: update V8 to 10.2.154.2 (Micha毛l Zasso) nodejs/node#42740
* (SEMVER-MINOR) fs: make params in writing methods optional (LiviaMedeiros) nodejs/node#42601
* (SEMVER-MINOR) http: add uniqueHeaders option to request and createServer (Paolo Insogna) nodejs/node#41397
* (SEMVER-MINOR) net: add ability to reset a tcp socket (pupilTong) nodejs/node#43112
* (SEMVER-MINOR) Revert "build: make x86 Windows support temporarily experimental" (Micha毛l Zasso) [#42740](nodejs/node#42740)
  * This means 32-bit Windows binaries are back with this release.

PR-URL: nodejs/node#43266
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready commit-queue-squash lib / src needs-ci notable-change semver-minor util
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet