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

[BUG] Parsing of flags with boolean & string values is incorrect. #6313

Closed
2 tasks done
ianlewis opened this issue Mar 31, 2023 · 6 comments
Closed
2 tasks done

[BUG] Parsing of flags with boolean & string values is incorrect. #6313

ianlewis opened this issue Mar 31, 2023 · 6 comments
Labels
Bug thing that needs fixing Needs Triage needs review for next steps Release 9.x work is associated with a specific npm 9 release

Comments

@ianlewis
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

This issue exists in the latest npm version

  • I am using the latest npm

Current Behavior

The npm fund command has a --browser flag which can be null (not provided), a boolean value, or a string.

However there are a number of things wrong with it.

  1. npm fund --browser=open should attempt to use the "open" command to open a browser. However the argument resolves to the value true and the system default is used.
  2. npm fund --browser --help should parse the --help flag but doesn't because it's treated as an argument to the --browser flag.

The end result is only boolean values can actually be input as all string values resolve to the boolean value true.

Expected Behavior

  1. npm fund --browser=open should attempt to use the appropriate method to open the browser.
  2. npm fund --browser --help should parse any flags after the --browser flag as a flag rather than an argument to the --browser flag.

Steps To Reproduce

  1. Run npm fund --browser=open on a Linux system.
  2. The system default xdg-open is used to open the browser.

Environment

  • npm: npm HEAD (60460ed)
  • Node.js: v16.17.0
  • OS Name: Debian Linux
  • npm config: None
; "user" config from /usr/local/google/home/ianlewis/.npmrc

//registry.npmjs.org/:_authToken = (protected) 

; "project" config from /usr/local/google/home/ianlewis/opt/node-v16.17.0-linux-x64/bin/node_modules/npm/.npmrc

package-lock = true 

; node bin location = /usr/local/google/home/ianlewis/opt/node-v16.17.0-linux-x64/bin/node
; node version = v16.17.0
; npm local prefix = /usr/local/google/home/ianlewis/opt/node-v16.17.0-linux-x64/bin/node_modules/npm
; npm version = 8.15.0
; cwd = /usr/local/google/home/ianlewis/opt/node-v16.17.0-linux-x64/bin/node_modules/npm
; HOME = /usr/local/google/home/ianlewis
; Run `npm config ls -l` to show all defaults.
@ianlewis ianlewis added Bug thing that needs fixing Needs Triage needs review for next steps Release 9.x work is associated with a specific npm 9 release labels Mar 31, 2023
@wraithgar
Copy link
Member

This has nothing to do w/ the fund command per se. It is an underlying bug w/ our opts parsing that does not work having an overloaded boolean or string flag.

@ianlewis
Copy link
Author

ianlewis commented Mar 31, 2023

This has nothing to do w/ the fund command per se. It is an underlying bug w/ our opts parsing that does not work having an overloaded boolean or string flag.

Right. The reason I mentioned it is that this is how it might affect users currently and how to reproduce the issue without necessarily biasing towards any specific solution to the problem.

@ianlewis ianlewis changed the title [BUG] npm fund --browser flag doesn't handle arguments properly [BUG] Parsing of flags with boolean & string values is incorrect. May 16, 2023
@bdehamer
Copy link
Contributor

Instead of trying to get boolean-and-string flags working, I think we're gonna add support for sets of mutually exclusive flags instead and then have things like --browser and --browser-path as separate, mutually exclusive flags.

npm/statusboard#677

@ianlewis
Copy link
Author

Ok, Thanks that's much easier. I had a PR that proposed that very thing but was rejected.
#6300

@wraithgar
Copy link
Member

@ianlewis yeah I had that in mind too when I was thinking about this. The thing that PR is missing is the mechanism to couple the two configs together so they're mutually exclusive by way of a simple attribute on the config definition. You had it as an explicit check in one of the commands but this would need to be baked into the config definition.

You were ultimately right in that the overloading was wrong, and since browser never worked right we can fix it w/o a semver major.

@ianlewis
Copy link
Author

@ianlewis yeah I had that in mind too when I was thinking about this. The thing that PR is missing is the mechanism to couple the two configs together so they're mutually exclusive by way of a simple attribute on the config definition. You had it as an explicit check in one of the commands but this would need to be baked into the config definition.

You were ultimately right in that the overloading was wrong, and since browser never worked right we can fix it w/o a semver major.

SG, thanks so much for following up on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Needs Triage needs review for next steps Release 9.x work is associated with a specific npm 9 release
Projects
None yet
Development

No branches or pull requests

3 participants