Compatibility: npm, yarn and pnpm run scripts

[ayu-exorcist]

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:

1. npm run scripts with args with the npm run foo -- --args-go-here.
2. yarn run scripts with args with the yarn foo -- --args-go-here or yarn foo --args-go-here
3. pnpm run scripts with args with the pnpm foo --args-go-here

After merged:
We can run npm|yarn|pnpm [run] foo -- --args-go-here only becase npm-run-all2 will change yarn and pnpm script to yarn|pnpm [run] foo --args-go-here for compatibility. (Based on the use of NPM run-scripts!)

More details see: #143

[codecov]

Codecov Report

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 ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #144      +/-   ##
==========================================
+ Coverage   95.93%   96.07%   +0.14%     
==========================================
  Files          35       35              
  Lines        2142     2142              
==========================================
+ Hits         2055     2058       +3     
+ Misses         87       84       -3     

Flag	Coverage Δ
macos-latest-18	95.98% <82.60%> (+0.14%)	⬆️
macos-latest-lts/*	95.98% <82.60%> (+0.14%)	⬆️
ubuntu-latest-18	95.60% <82.60%> (+0.14%)	⬆️
ubuntu-latest-lts/*	95.60% <82.60%> (+0.14%)	⬆️
windows-latest-18	?
windows-latest-lts/*	95.62% <82.60%> (+0.14%)	⬆️

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.

[ayu-exorcist]

@bcomnes Can you rerun the error test? I can't see any error logs. Thanks!

[bcomnes]

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?

[ayu-exorcist]

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.

[bcomnes]

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?

[ayu-exorcist]

Your understanding is correct.

[bcomnes]

Current thoughts on the change (assuming my understanding is correct):

• If this change goes in, we need tests for it.
• It seems like this shouldn't be a breaking change if done correctly. We shouldn't break scripts written for yarn/pnpm that already omit the --. If done right, this would just make scripts written for npm also work on yarn/pnpm by removing the correct -- when present. Argyment forwarding must also continue to work (the last PR it was reported broken)
• Even if it's probably not a breaking change, run scripts are complicated. It's hard to anticipate when -- gets used in other contexts, so it would need to go out in a breaking change either way.
• This is is surprising/unexpected behavior. The current reality is that pnpm/yarn introduced incompatible run scripts and this obfuscates that reality. These are not drop in replacement tools. If this goes in, do we also fix scripts in the pnpm->npm direction too?
• You can use npm-run-all2 with yarn/pnpm already by embracing those incompatibilities. Projects should not be encouraging package manager choice. Package manager choice is specific to the project.
• The offered computability layer is that run scripts trigger with the correct package manager used top level (instead of reverting to npm) and this is the true source of incompatibility.
• Another possible solution downstream to this issue is to ask which package manager the project wants to use when setting up the template, and adjust the run scripts accordingly. Giving people the impression you can swap package managers interchangeably will just waste peoples time as the inevitably run into other sources of incompatibilities.
• Both yarn 4 and pnpm both have npm-run-all2 like features built in now. If you use either, you probably should embrace those.
• No one should be using anything less than yarn 4 at this point.

I have a fever right now but I'm going to think about the change some more.

cc @ur5us if you have any thoughts.

[ayu-exorcist]

Perhaps it should be merged.
@bcomnes @ur5us

[bcomnes]

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:

• This issue is upstream from npm-run-all2. Yarn allows for runscripts that are incompatible with npm (but also supports npm style run scripts). Pnpm necessitates run scripts that are incompatible with npm. In the case where npm-run-all2 is not in use, these are just ecosystem incompatibilities that exist. I don’t like it, but that is just the way it is. If you want compatibility between the runners, it needs to be solved at the package manager level but at this point I don’t think there is much hope for that.
• npm-run-all2 is primarily a utility for npm, with best effort compatibility with yarn and pnpm. When run scripts are run with yarn or pnpm, and npm-run-all2 is in use, the most it does is swap out what launches sub-scripts so that an npm process isn’t introduced into a yarn run or a pnpm run. But in doing so, you are also expected to adjust your scripts accordingly with the package manager runner you are using, just as if npm-run-all2 wasn’t in use.
• By switching to yarn or pnpm for subscripts, npm-run-all2 also inherits the upstream incompatibles. Make sense?

Why I think introducing compatibility layers in npm-run-all2 is undesirable:

• There are work arounds that exist already (write your scripts specific to the package manager the project is using)
• There are many users already doing these work arounds, and we risk breaking that. This is an old package and this issue is already well understood.
• Trying to fix these incompatibilities here will just be confusing (newcomers will learn the wrong thing, and experienced users will be confused how its supposed to work e.g I expect to write pnpm style but with npm-runall2 do I write npm style or pnpm style when running pnpm?)
• Trying to simultaneously support yarn/pnpm/npm in a single project is a fools errand as other incompatibilities exist between lock files, and packages resolution results. Please don’t encourage this. I know yarn said this possible years ago, but we have years of evidence it isn’t.
• I don’t want to maintain compatibility layers for upstream tools that don't prioritize compatibility.

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.

[voxpelli]

I agree with @bcomnes 👍

[ayu-exorcist]

Some of the explanations can be made in the doc

[bcomnes]

Open to that.

[ur5us]

@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).

[bcomnes]

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.