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] npx doesn't pass $1 properly to executed package on Linux/Darwin #3067

Closed
bk201- opened this issue Apr 13, 2021 · 4 comments · Fixed by npm/run-script#78
Closed

[BUG] npx doesn't pass $1 properly to executed package on Linux/Darwin #3067

bk201- opened this issue Apr 13, 2021 · 4 comments · Fixed by npm/run-script#78
Assignees
Labels
Bug thing that needs fixing Priority 2 secondary priority issue Release 7.x work is associated with a specific npm 7 release

Comments

@bk201-
Copy link

bk201- commented Apr 13, 2021

Current Behavior:

Executing this script passes to the husky wrong argument, but it properly worked at npm@6

npx husky add .husky/commit-msg 'npx --no-install commitlint --edit "$1"'

The result is:

npx --no-install commitlint --edit ""

Expected Behavior:

The result is:

npx --no-install commitlint --edit "$1"

Steps To Reproduce:

  1. Created a new folder
  2. git init
  3. npm init
  4. npm install husky -D
  5. npm set-script prepare "husky install"
  6. npm run prepare
  7. npx husky add .husky/prepare-commit-msg 'npx --no-install jira-prepare-commit-msg "$1"'

I made some research here

Environment:

  • Ubuntu 20.04
  • Node: 15.3.1
  • npm: 7.9.0
@bk201- bk201- 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 Apr 13, 2021
@darcyclarke darcyclarke removed the Needs Triage needs review for next steps label May 28, 2021
@darcyclarke
Copy link
Contributor

This was a change we've made in v7 where we run scripts in a shell & don't have a way of reintroducing that v6 behaviour, which we believed to be problematic. .

@darcyclarke darcyclarke reopened this May 28, 2021
@darcyclarke darcyclarke added the Priority 2 secondary priority issue label May 28, 2021
@darcyclarke
Copy link
Contributor

@isaacs ping - I think you got this working?

@lydell
Copy link

lydell commented May 28, 2021

@darcyclarke This behavior change is super surprising. Let’s say you used to have husky and commitlint installed globally:

husky add .husky/commit-msg 'commitlint --edit "$1"'

Now, you install them locally instead. How do you run then? Easy, just put npx at the start:

npx husky add .husky/commit-msg 'npx --no-install commitlint --edit "$1"'

Wrong! Now your command doesn’t work anymore. Since this uses npx in two layers, good luck figuring out the escaping…

I reported the same thing but for backticks here: #3306

And I’ve got a PR fixing it here: npm/run-script#31

@isaacs
Copy link
Contributor

isaacs commented May 28, 2021

I can reproduce this. I'm not sure that npm/run-script#31 is quite the right way to go, but may be with some modifications.

In the meantime, this command does work properly:

npx --package=husky -c "husky add .husky/prepare-commit-msg 'npx --no-install jira-prepare-commit-msg \"\$1\"'"

That works because it uses the --call option rather than the (currently quite baroque) "create a fake package.json script and tell run-script to run it". Then, the only tricky bit is getting the quotes right.

The ideal solution here is going to look like:

  1. Create matrix of (a) way in which we currently exec arbitrary scripts (exec, npx, exec -c, npx -c, run-script) ✖️ (b) every platform we run on (cmd where the target is a batch script, cmd where the target is a binary, powershell, sh-likes) ✖️ (c) funkiness of the arguments (contain quotes, contain back-ticks, contain $vars, contain backslashes, any of those things but escaped, any of those things but quoted, etc.)
  2. For each item in the matrix, identify expected behavior, current behavior, and breakage that we expect by replacing "current" with "expected" (if any)
  3. turn that matrix into a test suite
  4. make the tests pass (probably yes by using puka in some way, but definitely by refactoring some of the stuff we're doing today to be more reasonable, so that npm exec doesn't go through @npmcli/run-script)

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
5 participants