Skip to content

Don't try to run scripts that aren't defined #6841

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

peterlebrun
Copy link

@peterlebrun peterlebrun commented Dec 19, 2018

Fixes #6739

This is my first pull request with yarn (I love yarn, I got my whole company on it!), so please let me know if I've missed anything from the contributing instructions.

Right now, yarn workspaces run script will exit with error if script is not defined in all workspaces. This is because yarn workspaces run naively spawns a child process without determining if the script exists. This pull request checks that the script exists in the workspace before attempting to spawn a new child process to run the script.

I also output the workspace name before each command attempt, so that we can keep track of where the script does/doesn't exist.

I have added unit tests, and tested it within yarn/packages/pkg-tests:

image

@eps1lon
Copy link
Member

eps1lon commented Jan 16, 2019

This also includes a fix for #6389

@peterlebrun
Copy link
Author

@eps1lon following discussion with @arcanis I have removed changes relating to the workspace names. I will submit a separate PR for issue #6389.

At this point, this PR only addresses issue #6739.

@peterlebrun peterlebrun force-pushed the yarn-workspaces-run branch 2 times, most recently from 2065365 to 3d9eb07 Compare January 16, 2019 21:34
@buildsize
Copy link

buildsize bot commented Jan 17, 2019

File name Previous Size New Size Change
yarn-[version].noarch.rpm 1.11 MB 1.11 MB 342 bytes (0%)
yarn-[version].js 4.47 MB 4.47 MB 1.2 KB (0%)
yarn-legacy-[version].js 4.66 MB 4.66 MB 1.24 KB (0%)
yarn-v[version].tar.gz 1.12 MB 1.12 MB -330 bytes (0%)
yarn_[version]all.deb 815.78 KB 815.68 KB -96 bytes (0%)

@peterlebrun
Copy link
Author

@arcanis I have updated this PR to work with yarn workspaces run -s exec ...

@markuskobler
Copy link

Is anything else needed to get this merged?

@arcanis
Copy link
Member

arcanis commented Feb 4, 2019

Mostly time for me to review it. I plan to make a review pass tomorrow.

@TerribleDev
Copy link

@arcanis what can we do to get this over the line? 🚗

@rally25rs
Copy link
Contributor

This change results in a problem where installed binaries can no longer be run in workspaces. For example if your packages use jest for unit testing, you can no longer yarn workspaces run jest

example:

~/Projects/yarn-test2 🐒   yarn workspace pkg1 add jest --dev
yarn workspace v1.16.0-0
yarn add v1.16.0-0
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
warning "workspace-aggregator-84402bc9-4dfe-4818-a7ac-77ec7e134aa0 > jest > jest-cli > @jest/core > [email protected]" has unmet peer dependency "jest-resolve@^24.1.0".
warning "workspace-aggregator-84402bc9-4dfe-4818-a7ac-77ec7e134aa0 > jest > jest-cli > jest-config > [email protected]" has unmet peer dependency "jest-haste-map@^24.0.0".
[4/4] 🔨  Building fresh packages...

success Saved 1 new dependency.
info Direct dependencies
info All dependencies
└─ [email protected]
✨  Done in 8.31s.
✨  Done in 8.61s.

existing yarn:

~/Projects/yarn-test2 🐒   yarn workspaces run jest
yarn workspaces v1.13.0
yarn run v1.13.0
$ /Users/jvalore/Projects/yarn-test2/packages/pkg1/node_modules/.bin/jest
 PASS  ./some.test.js
  pkga
    ✓ has tests (4ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        0.998s, estimated 1s
Ran all test suites.
✨  Done in 1.64s.
✨  Done in 2.02s.

with this change:

~/Projects/yarn-test2 🐒   yarn workspaces run jest
yarn workspaces v1.16.0-0
warning No scripts defined in workspace pkg1.
✨  Done in 0.09s.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants