Compatibility: npm, yarn and pnpm run scripts

[ayu-exorcist]

Fix downstream issue: vuejs/create-vue#393

Related downstream PR: vuejs/create-vue#394

-- double dash behavior in npm/yarn/pnpm

• npm run-script: double dash cannot be omitted. See official doc
• yarn1.x run-script: first double dash should be omitted. See yarn code
• pnpm run-script: first double dash should be omitted. See pnpm run code and pnpm exec code

[bcomnes]

EDIT sorry I read this as an issue, not a PR. Looking

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 69.56522% with 7 lines in your changes missing coverage. Please review.

Project coverage is 95.93%. Comparing base (79e2c97) to head (b8d3ded).
Report is 28 commits behind head on master.

Files	Patch %	Lines
lib/index.js	69.56%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
- Coverage   96.82%   95.93%   -0.89%     
==========================================
  Files          35       35              
  Lines        2080     2142      +62     
==========================================
+ Hits         2014     2055      +41     
- Misses         66       87      +21     

Flag	Coverage Δ
macos-latest-18	95.84% <69.56%> (-0.91%)	⬇️
macos-latest-lts/*	95.84% <69.56%> (?)
ubuntu-latest-18	95.45% <69.56%> (-0.90%)	⬇️
ubuntu-latest-lts/*	95.45% <69.56%> (?)
windows-latest-18	95.47% <69.56%> (-0.91%)	⬇️
windows-latest-lts/*	95.47% <69.56%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bcomnes]

This seems fine. Let's try it out. If this causes issues for anyone, please open an issue!

[ur5us]

@bcomnes Before this change, the following was working fine but is now broken:

"build:tailwindcss": "run-p 'build:tailwindcss:* {@}' --",
"build:tailwindcss:admin": "tailwindcss --postcss -i ./app/assets/stylesheets/admin/application.tailwind.css -o ./app/assets/builds/admin/application.css",
"build:tailwindcss:pages": "tailwindcss --postcss -i ./app/assets/stylesheets/application.tailwind.css -o ./app/assets/builds/application.css",

…and then called via yarn build:tailwindcss -- --watch

Am I missing something?

[bcomnes]

Ok good to know. I'll revert and take a closer look at the change.

[bcomnes]

I'm leaning to thinking this is probably a correct behavior change, but will likely need to go out in a breaking change. I didn't realize that earlier today. Sorry about that.

Will take a closer look tomorrow and update tests to cover the breaking change.

[bcomnes]

@ayu-exorcist can you clarify the problem more? What do you need to do right now to get it to work? What doesn't work in that arrangement? What will we need to break to implement your behavior? Specific examples are helpful. I'm trying to read upstream but your insight will be helpful here as well.

[ayu-exorcist]

"build:tailwindcss": "run-p 'build:tailwindcss:* {@}' --",

Please change "run-p 'build:tailwindcss:* {@}' --" to "run-p 'build:tailwindcss:* -- {@}' --"

For NPM, -- should not be omitted. When running with Yarn/PNPM, npm-run-all2 will remove it internally.

@ur5us

[ur5us]

@ayu-exorcist No, that does not work!

[ayu-exorcist]

@bcomnes I don't familiar with npm-run-all2. Needs some help to proceed with the modifications.

@ur5us You need update npm-run-all2 to v6.2.2

[bcomnes]

@ayu-exorcist just to clarify I reverted this change in v6.2.2 for now. If it goes in it needs to be a breaking change.

[ayu-exorcist]

@ayu-exorcist just to clarify I reverted this change in v6.2.2 for now. If it goes in it needs to be a breaking change.

@bcomnes Yep, we need to have a provide unified command that is compatible with npm/yarn/pnpm double-dash.

[bcomnes]

Ok reading through this again.

This should be resolved in the upstream npm-run-all2

Here is my understanding:

• npm shipped with the ability to run scripts with arguments with the npm run foo -- --args-go-here originally.
• Yarn ships the ability to run scripts with passed args while omitting --: yarn run foo --args-go-here, but also supported the -- yarn run foo -- --this-also-works
• Pnpm you pass pnpm run --args-go-here. pnpm doesn't handle -- in any special way (it just gets passed as a flag), meaning that its incompatible with the npm style of passing flags to scripts.

Unfortunately this is the worst of all worlds. yarn introduces a cute omission feature with npm compatibility, pnpm adopts the cute omission pattern, but drops npm compatibility, and npm is what it is. In general, these tools are not interchangeable when it comes to writing run scripts. Pick a pattern, and assert which package manager you expect them to work with. I'm guessing this is what people have worked around for quite a long time.

So the breaking change here would be:

• Rewrite your run scripts to match the npm style -- args passing, as the examples are written in the docs for nom-run-all2, even if you are running yarn or pnpm.
• The new version of npm-run-all2 would then strip these -- args when run with yarn/pnpm
• In doing so, your run scripts that use npm-run-all2 would then work with any package manager.

I'm going to have to think this over a bit more, since in general as mentioned, pick one and adjust is generally the most pragmatic approach. Trying to support all 3 at once opens up a ton of edge cases to deal with. Also people using yarn and pnpm probably have opinions about adding -- where it's not conventional to do so.

[ur5us]

@bcomnes @ayu-exorcist Not sure whether it’s any helpful but here’s the issue when using "run-p 'build:tailwindcss:* -- {@}' --" on v6.2.1:

With the following in my package.json

"build:tailwindcss": "run-p 'build:tailwindcss:* -- {@}' --",
"build:tailwindcss:admin": "tailwindcss --postcss -i ./app/assets/stylesheets/admin/application.tailwind.css -o ./app/assets/builds/admin/application.css",
"build:tailwindcss:pages": "tailwindcss --postcss -i ./app/assets/stylesheets/application.tailwind.css -o ./app/assets/builds/application.css",

I then get this:

yarn build:tailwindcss --watch
yarn run v1.22.22
$ run-p 'build:tailwindcss:* -- {@}' -- --watch
$ tailwindcss --postcss -i ./app/assets/stylesheets/booking/application.tailwind.css -o ./app/assets/builds/booking/application.css {@}
$ tailwindcss --postcss -i ./app/assets/stylesheets/application.tailwind.css -o ./app/assets/builds/application.css {@}

Note the literal {@} at the end of command invocation. So the argument forwarding is not working. On v6.2.0 and now v6.2.2 where this change was reverted the {@} is properly substituted to --watch.

[ayu-exorcist]

Solved it! More see: RegExp/test in MDN

[bcomnes]

@ur5us Just want to reiterate, you should have not had to change anything with this going in. Sorry about the disruption.

Moving discussion over to the new PR: #144