yarnpkg / berry

📦🐈 Active development trunk for Yarn ⚒
https://yarnpkg.com
BSD 2-Clause "Simplified" License
7.44k stars 1.11k forks source link

[Feature] explicit "run global" behavior instead of special-casing `:` #3424

Open seansfkelley opened 3 years ago

seansfkelley commented 3 years ago

Describe the user story

Inspired by https://github.com/yarnpkg/berry/issues/3048. Note: some of the specifics may have changed since https://github.com/yarnpkg/berry/pull/3423 was merged -- I haven't had a chance to try it out -- but much of the below remains relevant.

: is frequently used in package.json script definitions to represent the concept of "scope" or "specificity". Some tools have even built the concept in as a first-class behavior, such as npm-run-all's globbing feature to run a bunch of related scripts at once.

This idiomatic usage of colon-as-scope more or less coexists peacefully with the existing colon-means-global behavior as far as run is concerned. This is because run will prefer a script defined in the current workspace before attempting to run a global script from another workspace. This preference behavior is crucial, because in the case where multiple workspaces define the same script, run will fail (since there are multiple definitions) if you run it in the "global manner", i.e., from a package that does not define that script name locally.

However, workspaces foreach does not interact nicely with the colon-means-global behavior. Scripts that are defined using colon-as-scope and work as expected when run from the package -- because run prefers the local script definition -- cannot be run in bulk with workspaces foreach run foo:bar because that command will find conflicting definitions for the global script. This is very surprising because it breaks the expectation that workspaces foreach run is just "bulk run".

Furthermore, the colon-means-global behavior is very non-obvious and fragile. For instance, adding a script with the same name as a global script to a workspace can cause unrelated packages/commands to start failing because there are now two definitions for the global script. Or, removing the second definition of a script will silently make the remaining definition global. Spooky action at a distance!

Describe the solution you'd like

I think all of the former problems can be resolved by making "global scripts" an explicit opt-in feature. I imagine this as a --global/-g flag to run and workspaces foreach.

Doing so should resolve all the aforementioned issues and preserve the existing feature set, albeit with slightly more work if you are using global scripts:

This change could be partially adopted in a non-breaking manner by adding the reverse of the -g flag to workspaces foreach -- that is, a flag that tells it to prefer local scripts, and to ignore the possibility of invoking global scripts. Perhaps it could be called --local/-l. For my part, this would fix my immediate problem.

Describe the drawbacks of your solution

This would be a breaking change, though as noted above, it can be partially adopted with a new backwards-compatible flag.

The only other downside I can see is that it's marginally more keystrokes to invoke a script globally.

Describe alternatives you've considered

This doesn't make sense as a plugin since it's about undoing some interpretations that the core run command makes about your project.

kachkaev commented 2 years ago

A possible way to avoid breaking changes would be to add a new option to .yarnrc.yml. It can be then removed in Yarn v4.

olivierlacan commented 8 months ago

This caused a huge gotcha for our team as the root package.json had a test script defined that used yarn workspaces foreach --all --interlaced -vv run test.

Each workspace package.json defines their own test script which in our case call back to the global script in the root package.json because we run tests with a multi-project configuration via Jest from the root, and we want to be able to run the test script both from the root of the monorepo (yarn test) and from the specific workspace (cd apps/api && yarn test).

But we noticed (sometime between 4.0 and 4.1 releases) that we hadn't considered how circular that is.

Having named our root script g:test would have prevented this, but in essence this resulted in a loop where:

Obviously, we've overcomplicated our monorepo config to make test execution flexible, so this is not likely to be a common concern. But I think global scripts depending on such a loose (and overloaded) convention as : prefixing doesn't sufficiently separate/isolate scripts that invoke workspace scripts.

Our solution was, in the root package.json:

{
  "scripts": {
    "g:test": "yarn workspaces foreach --all --interlaced -vv run test",
    "g:test:api": "yarn jest --selectProjects api",
    "g:test:client": "yarn jest --selectProjects client",
  }
}

In the apps/api workspace package.json:

{
  "scripts": {
    "test": "yarn g:test:api",
  }
}

Expectation

I believe Yarn should issue a warning if it detects scripts in the root package.json that have the same name as scripts (with different definitions) in the workspace package.json files. Particularly if the script in the root uses foreach, given that there may be valid reasons for having scripts with the same name in the root and in workspaces.