Added ability to run script on all workspaces #6244
Conversation
|
|
Looks good, thanks! How are options handled? Will We have some mechanisms to avoid this, can you check whether it could be easily done here?: https://github.com/yarnpkg/yarn/blob/master/src/cli/index.js#L183-L199 |
| @@ -42,3 +47,17 @@ test('workspaces info should list the workspaces', (): Promise<void> => { | |||
| }); | |||
| }); | |||
| }); | |||
|
|
|||
| test('workspaces run should spawn command for each workspace', (): Promise<void> => { | |||
There was a problem hiding this comment.
I thought test would cover the passing of the arguments, or am I missing something. It sounds like I may need to either leave run in the arg list or add an additional -- for the user?
There was a problem hiding this comment.
The problem with those tests is that when you call runWorkspaces, it directly invokes the command, without going through our CLI code. If it did, you would see that the --flag1 flag would be interpreted as a command-line option for the yarn workspaces run command, and wouldn't actually be passed to the argument array.
There was a problem hiding this comment.
I think the additional testing I did and added to the description may cover the case you are mentioning. I was able to validate that --all does properly get passed to the subsequent sub-processes.
There was a problem hiding this comment.
@arcanis Can you confirm if my additional testing steps are adequate?
|
I updated the description with some more testing that I ran to verify that the command was properly invoked with the correct arguments passed. |
This directly relates to yarnpkg/yarn#6244
src/cli/commands/workspaces.js
Outdated
| ); | ||
| } | ||
|
|
||
| await Promise.all(childProcesses); |
There was a problem hiding this comment.
Do you mind if we execute the processes synchronously, at least for a first iteration? Something to keep in mind is that Yarn currently shouldn't be executed concurrently on the same cache - so running it in parallel here might cause cache corruption issues by default 😕
There was a problem hiding this comment.
Good to know. That makes total sense!
arcanis
force-pushed
the
workspaces-run
branch
from
54f4c24 to
7c27b7b
Compare
|
Thank you! Can't wait to test this 😍 |
|
Any plans for documentation updates for this one? Edit: Also, quickly trying this out, it looks like the behavior is to fail when all workspaces don't have the "script" being ran... Why not just skip over the ones that don't have it? |
|
RE: "Why not just skip over the ones that don't have it?" Similar to how test runners fail when no tests are found, if yarn did skip non-existent scripts, it may cause unintended errors (typos, forgetfulness). So failing is a safer default. Also, to me, Something like Personally, I prefer explicit per-workspace "ignores" than implicit ones. So in a monorepo, I'd expect scripts being run globally from the top level to exist in every package for consistency. You can easily add a script to a workspace to say something like "echo 'No tests.'" if there's nothing to do and it makes your intentions explicit. |
|
To echo @jahed, the intent was to match workspace run, but for all workspaces so failing on missing was intended. @dsifford, as for docs. I already have a PR up for it. Looks like it needs to be merged, I will post there. yarnpkg/website#855 |
This directly relates to yarnpkg/yarn#6244
|
Got it, thanks. Just for sake of discussion, why not have Is there any use-case for top-level scripts currently? I guess it could become weird/buggy if workspaces shared a script with the same name as the top-level script though... |
|
Too magical imo. Once we implement nested workspaces, "top-level scripts" will become even more blurry. I like Yarn scripts being simple, because they're easy to reason about. |
|
I agree with above. IMO, the power of workspaces is that any workspace could be removed and put into their own repo without breaking anything. |
Summary
Added the ability to run a script on every workspace in a project.
yarn workspaces run scriptMotivation was to be able to run build or test across an entire monorepo during a CI process. While it was possible to run a script on a workspace individually by name
yarn workspace workspace-1 script. This required you to know and list all workspaces.Test plan
I added more tests that ensure that the commands are being spawned properly. Since most of the code was copied from functioning parts of the project, I felt this was sufficient.
Update
Based on feedback I validated that arguments can be passed.
Here were the commands I ran.
cd __tests__/fixtures/workspace/run-basicyarn workspace workspace-1 add jestyarn workspace workspace-2 add jestyarn workspace workspace-1 run jest --all--> sanity checkyarn workspaces run jest --allWhile jest doesn't pass because there are no tests, you can see from the output the arguments are passed properly.
Here are the spawned child commands