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] Using -- to pass additional arguments incorrectly wraps those arguments in quotes #2720

Closed
Pomax opened this issue Feb 18, 2021 · 6 comments
Labels
Bug thing that needs fixing Priority 2 secondary priority issue Release 7.x work is associated with a specific npm 7 release

Comments

@Pomax
Copy link

Pomax commented Feb 18, 2021

Current Behavior:

Using -- to pass additional arguments incorrectly wraps those arguments in quotes

Expected Behavior:

Using -- to pass additional arguments does what https://docs.npmjs.com/cli/v6/commands/npm-run-script claims it will do:

npm will pass all the arguments after the -- directly to your script:

I take "directly" here to mean "without any kind of rewriting"

Steps To Reproduce:

a package.json with:

"scripts": {
  "x": "echo test",
  "y": "npm run x -- --flag"
}

shows:

npm run y               

> y
> npm run x -- --flag

> x
> echo test "--flag"

test --flag

instead of

npm run y               

> y
> npm run x -- --flag

> x
> echo test --flag

test --flag

Reason this is a bug

this makes it impossible to forward flags with escaped quotes. For example, passing a quoted string (in which the quotes must be preserved) to esbuild becomes impossible:

"scripts": {
    "esbuild:base": "esbuild --bundle --loader:.js=jsx --loader:.jsx=jsx",
    "esbuild:base:main": "npm run esbuild:base -- ./source/js/main.js --outfile=./network-api/networkapi/frontend/_js/main.js",
    "esbuild:dev:main": "npm run esbuild:base:main -- --define:process.env.NODE_ENV=\\\"development\\\" --watch"
}

Note the escaped development string, which is escaped once so " doesn't break the script string, and escaped a second time so that it's passed correctly as CLI argument.

This should run, but instead because things get wrapped in quotes, this errors out:

npm run esbuild:dev:main

> esbuild:dev:main
> npm run esbuild:base:main -- --define:process.env.NODE_ENV=\"development\" --watch


> esbuild:base:main
> npm run esbuild:base -- ./source/js/main.js --outfile=./network-api/networkapi/frontend/_js/main.js "--define:process.env.NODE_ENV=\\development\\" "--watch"


> esbuild:base
> esbuild --bundle --loader:.js=jsx --loader:.jsx=jsx "./source/js/main.js" "--outfile=./network-api/networkapi/frontend/_js/main.js" "--define:process.env.NODE_ENV=\\\\development\\\\" "--watch"

> error: Invalid define value (must be valid JSON syntax or a single identifier): \\\\development\\\\

Note the line above the error: we suddenly have quoted arguments in which --define:process.env.NODE_ENV=\\\"development\\\" has been rewritten such that it actually loses quotes: "--define:process.env.NODE_ENV=\\\\development\\\\" no longer has a quote preceding the d in development.

Nothing should be rewriting args passed further along with --, those should stay exactly as they were.

Environment:

Windows 10 Pro x64
Node 15..8.0
npm 7.3.0

@Pomax Pomax added Bug thing that needs fixing Needs Triage needs review for next steps Release 7.x work is associated with a specific npm 7 release labels Feb 18, 2021
@EdisonHarada
Copy link

EdisonHarada commented Feb 22, 2021

NPM is removing double quotes from any part of the command:

In my case I have:

....
  "scripts": {
    "download": "FOR /F \"tokens=*\" %i IN ('git rev-parse --abbrev-ref HEAD') DO crowdin download --branch %i"
  },
...

It tries to execute as:
FOR /F tokens=* ^%i IN ("git rev-parse --abbrev-ref HEAD") DO crowdin download --branch ^%i

Instead of:
FOR /F "tokens=*" ^%i IN ("git rev-parse --abbrev-ref HEAD") DO crowdin download --branch ^%i

I tried escaping in other ways but didn't work :/

@Pomax
Copy link
Author

Pomax commented Feb 22, 2021

That does not look like it's related to using -- for runtime argument forwarding, so you might want to file that as a new, separate issue (it might be related, but that's something that the npm folks can determine).

@wraithgar wraithgar added Priority 2 secondary priority issue and removed Needs Triage needs review for next steps labels Mar 22, 2021
@wraithgar
Copy link
Member

This was fixed in run-script here: npm/run-script#22 and should be included w/ the next npm release (going out this week)

@EdisonHarada
Copy link

@wraithgar It's working like a charm.

I think you can close this one 👍

@cyberbiont
Copy link

I am on npm 7.11.0 and problem with arguments passed in quotes is still here.
Not sure if this is directly related, but I cannot pass arguments to the executable that is called in the npm script.
For example

.... 
"scripts": { 
    "g": "gulp"
    "tasks": "npm run g -- --tasks"
}

If I run npm run tasks, a list of gulp tasks is shown.
In contrast, executing npm run g -- --tasks directly from the command line (that I believe in this case should invoke gulp with --tasks argument) and gulp starts executing default gulp task.

if I change 'g' task to echo gulp, npm run g -- --tasks from CLI prints gulp "--tasks"

@wraithgar
Copy link
Member

@cyberbiont please open a new issue for this, as this was specifically a windows wrapping bug, and yours may be different in ways the issue template should show us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Priority 2 secondary priority issue Release 7.x work is associated with a specific npm 7 release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants