Compatibility: npm, yarn and pnpm run scripts

ayu-exorcist commented 4 months ago

Fix downstream issue: https://github.com/vuejs/create-vue/issues/393

Related downstream PR: https://github.com/vuejs/create-vue/pull/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 commented 4 months ago

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

codecov-commenter commented 4 months ago

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 :warning:

Additional details and impacted files

```diff @@ 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](https://app.codecov.io/gh/bcomnes/npm-run-all2/pull/143/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Bret+Comnes) | Coverage Δ | | |---|---|---| | [macos-latest-18](https://app.codecov.io/gh/bcomnes/npm-run-all2/pull/143/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Bret+Comnes) | `95.84% <69.56%> (-0.91%)` | :arrow_down: | | [macos-latest-lts/*](https://app.codecov.io/gh/bcomnes/npm-run-all2/pull/143/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Bret+Comnes) | `95.84% <69.56%> (?)` | | | [ubuntu-latest-18](https://app.codecov.io/gh/bcomnes/npm-run-all2/pull/143/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Bret+Comnes) | `95.45% <69.56%> (-0.90%)` | :arrow_down: | | [ubuntu-latest-lts/*](https://app.codecov.io/gh/bcomnes/npm-run-all2/pull/143/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Bret+Comnes) | `95.45% <69.56%> (?)` | | | [windows-latest-18](https://app.codecov.io/gh/bcomnes/npm-run-all2/pull/143/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Bret+Comnes) | `95.47% <69.56%> (-0.91%)` | :arrow_down: | | [windows-latest-lts/*](https://app.codecov.io/gh/bcomnes/npm-run-all2/pull/143/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Bret+Comnes) | `95.47% <69.56%> (?)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Bret+Comnes#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bcomnes commented 4 months ago

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

ur5us commented 4 months ago

@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 commented 4 months ago

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

bcomnes commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

"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 commented 4 months ago

@ayu-exorcist No, that does not work!

ayu-exorcist commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

Solved it! More see: RegExp/test in MDN

bcomnes commented 4 months ago

@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: https://github.com/bcomnes/npm-run-all2/pull/144

bcomnes / npm-run-all2