Closed ayu-exorcist closed 2 months ago
Attention: Patch coverage is 82.60870%
with 4 lines
in your changes missing coverage. Please review.
Project coverage is 96.07%. Comparing base (
b8d3ded
) to head (beb1e8a
). Report is 4 commits behind head on master.
Files | Patch % | Lines |
---|---|---|
lib/index.js | 82.60% | 4 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@bcomnes Can you rerun the error test? I can't see any error logs. Thanks!
Can you clarify what the intended end user changes this will require will be? ie what did you have to do before, and what will you need to do after this change. Also, which version of yarn are you targeting?
Can you clarify what the intended end user changes this will require will be? ie what did you have to do before, and what will you need to do after this change. Also, which version of yarn are you targeting?
I have modified the commit logs and added more descriptions.
Do I understand correctly:
You want to enable people to write a run script like this:
{
"scripts": {
"start": "run-s build \"start-server -- --port {1}\" --"
}
}
That is invoked like this:
$ npm run start 8080
And that prior to your proposed change, this run script wasn't compatible with yarn/pnpm as written, and required the following modification to work with yarn/pnpm:
{
"scripts": {
"start": "run-s build \"start-server --port {1}\" --"
}
}
But in doing so, breaks compatibility with npm
.
Your change works by targeting any quoted --
pattern and remove that when run with yarn/pnpm, in doing so, lets you run the first run script in pnpm and yarn as well?
Your understanding is correct.
Current thoughts on the change (assuming my understanding is correct):
I have a fever right now but I'm going to think about the change some more.
cc @ur5us if you have any thoughts.
Perhaps it should be merged. @bcomnes @ur5us
Sorry for the delay, I had a fever and was traveling. After thinking about it some more, I don’t think I want to land this. Let me try and convince you why:
This is how I understand the issue:
Why I think introducing compatibility layers in npm-run-all2 is undesirable:
My advice: For templates/starter projects that want to support various package mangers, you should make a single selection on generation and adjust your templates based on these incompatibilities. Also if you steer people towards yarn, be sure to start them off on v4 which requires corepack iirc. Older version have bugs that will never be fixed.
I agree with @bcomnes 👍
Some of the explanations can be made in the doc
Open to that.
@bcomnes I, too, am slow to respond because of sickness. Anyway, I don’t have many thoughts other than wanting this package to work the way it was advertised to begin with and more specifically the way I ended up using it, i.e. building multiple CSS bundles with TailwindCSS, executed either via a Procfile(.dev)
or as part of any build process.
If there’s a good and maintainable way to unify various package managers’ behavior, great. But the previous attempt broke it. Maybe this one is better, dunno. I haven’t looked closely at the proposed code change and haven’t tried for my use case.
My gut feeling aligns with your thoughts, i.e. it’s probably futile to try “fix” package managers’ behavior for --
parameter handling and more specifically how some packages use and probably abuse it. I’m not convinced this is a universally solved problem as you’re juggling CLI parameters for the command runner (this package; runp
), the package manager and the actual command one’s interested in (tailwindss
in my case).
Since I haven't seen any compelling reasons against the above arguments, I'm going to close for now. I don't have bandwidth to document yarn/pnpm concerns but open to PRs that improve that.
This is a destructive change.
These commits purpose of this PR is to unify the usage of downstream calling because npm (each version), yarn(v1.x, more see https://github.com/yarnpkg/yarn/pull/4152) and pnpm(v7+, more see https://github.com/pnpm/pnpm/pull/4290 https://github.com/pnpm/pnpm/pull/3492, https://github.com/pnpm/pnpm/pull/7370) handle double-das differently.
Examples:
npm
run scripts with args with thenpm run foo -- --args-go-here
.yarn
run scripts with args with theyarn foo -- --args-go-here
oryarn foo --args-go-here
pnpm
run scripts with args with thepnpm foo --args-go-here
After merged: We can run
npm|yarn|pnpm [run] foo -- --args-go-here
only becasenpm-run-all2
will changeyarn
andpnpm
script toyarn|pnpm [run] foo --args-go-here
for compatibility. (Based on the use of NPM run-scripts!)More details see: https://github.com/bcomnes/npm-run-all2/pull/143