Skip to content
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

update workspace-run-commands to more detail #87

Merged
merged 1 commit into from
Feb 6, 2018

Conversation

ianstormtaylor
Copy link
Contributor

cc @bestander from discussion in #83

This expands the existing workspace-run-commands to add more detail, and borrow a few more of the useful pieces of the functionality Lerna offers for running commands in workspaces.

Specifically, it introduces:

  • Executing arbitrary commands, not just package scripts with yarn workspaces exec.
  • Passing extra arguments to the package scripts with yarn workspaces run.
  • Controlling whether to run in parallel or not with --parallel.
  • Controlling the concurrency with --concurrency.
  • Filtering the commands to a specific set of workspaces with the --workspaces flag.
  • Filtering the commands to a specific set of packages with the --packages flag.

Almost all of this functionality is based on Lerna's prior art, which has been super useful to managing monorepos in my experience—for all things from running tests, to building source, to automating other useful tasks.


One open question I have is how the Yarn team thinks about what a "workspace" is. In the package.json configuration, they're defined like:

"workspaces": [
  "packages/*",
  "services/*",
  "utils/*"
]

Which makes it simple to have multiple directories of workspaces, or to list individual top-level directories as workspaces too. This is super useful in my experience.

But it also calls into question how we should be referring to specific folders inside those directories. Is the packages folder a "workspace"? Or is each of the directories inside packages a "workspace"?

This question becomes more important when trying to figure out how to let people filter things. Because it would be super useful to be able to do something like:

$ yarn workspaces run --filter 'services/*' test

To restrict the command to run only in the services/* workspaces above.

Lerna solves this by instead calling the configuration packages, and then only ever refers to packages by their name in package.json instead, which simplifies things. It's possible that Yarn should follow their lead, and ignore the idea of a "workspace name", and only treat package names. (Which would eliminate the --workspaces flag in this pull request.)

In this sense, the --packages and --workspaces flags are just strawmen. If there were to only be a single filtering mechanism, it could use --only and --ignore flags instead.

That said, it feels slightly weird for Yarn to declare workspaces the way it does, and then not to let people filter by services/* or packages/* in this case. Thoughts?

@rally25rs
Copy link
Contributor

Overall I like the idea. Monorepos are becoming more popular so increasing the tooling around them makes sense. I also don't like that currently I still have a lerna dependency just so I can publish in a sane way.

Can we include doing actual docs around workspaces to this RFC? The complete lack of docs is pretty frustrating. Even as a contributor I barely understand how they work. The existing yarn workspace command seems to take a "workspace name" but what is that? How do you name workspaces? The docs website does not have a page for that command.

Also in src/commands/ there is a workspace and a workspaces. What's the difference?

~/Projects/yarn-test 🐒   yarndev workspaces
yarn workspaces v1.4.1
error Invalid subcommand. Try "info"
info Visit https://yarnpkg.com/en/docs/cli/workspaces for documentation about this command.

~/Projects/yarn-test 🐒   yarndev workspace
yarn workspace v1.4.1
error Missing workspace name.
info Visit https://yarnpkg.com/en/docs/cli/workspace for documentation about this command.

Copy link
Member

@bestander bestander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the improvements, it makes sense to include it in Yarn.

But it also calls into question how we should be referring to specific folders inside those directories. Is the packages folder a "workspace"? Or is each of the directories inside packages a "workspace"?

I don't have a strong opinion here.
To me referring workspaces by name from package.json seems intuitive and easy.

Anyway, I would trust you to chose the easiest solution to the open questions and send a PR.

@bestander
Copy link
Member

If no one objects let's merge this PR next week

@bestander
Copy link
Member

I agree with @rally25rs, we need a champion to document Workspaces docs.

@bestander
Copy link
Member

@giridharangm, do you agree with the changes?

@giridharangm, @ianstormtaylor are you ready to split the work and send PRs for those features?

@ianstormtaylor
Copy link
Contributor Author

@bestander I realize this is not as helpful, but unfortunately I won't have the time to be able to submit a PR implementing this. I was just trying to be helpful by fleshing out the spec for the commands more, ensuring that we port some of the good aspects of Lerna, so that whoever does work on them has something more concrete to work from.

Btw, another open question I think during implementation will be:

  • If you pass something like --parallel/concurrent to yarn workspaces run are those flags masked from the scripts underneath? Because the regular yarn run doesn't have flags I think, which allows it to be unambiguous in what's passed directly to further scripts.

@bestander
Copy link
Member

@ianstormtaylor, whatever help you can offer is appreciated.

If you pass something like --parallel/concurrent to yarn workspaces run are those flags masked from the scripts underneath? Because the regular yarn run doesn't have flags I think, which allows it to be unambiguous in what's passed directly to further scripts.

Good question.
I think you can pass through arguments with -- separator yarn run <command> --verbose -- -command-specific-arg.
I know a few people are not happy about this syntax and it is open to discussion

@ianstormtaylor
Copy link
Contributor Author

Ah okay cool, I didn't realize there were also flags on the regular yarn run, so it's not introducing any new issues, that's good!

@DeltaCircuit
Copy link
Contributor

@ianstormtaylor thanks for the refinements. It looks great!.

@bestander as much as I'd like to contribute, unfortunately, my hands are already full with multiple projects (both prod and dev) now. Besides, I don't have that much knowledge about yarn's code base and would require me to start from the ground up 😞 . I'm awfully sorry...

@bestander
Copy link
Member

Ok, let's merge it and look for champions

@bestander bestander merged commit 80e2071 into yarnpkg:master Feb 6, 2018
@donaldpipowitch
Copy link

donaldpipowitch commented Feb 6, 2018

Thank you all for the work. Just a short question: This RFC enforces that the script name is available in all packages when you run yarn workspaces run <cmd> unlike Lerna, right? (E.g. Lerna silently skips commands which aren't configured in the scripts section of a package.json which I quite like.)

@DeltaCircuit
Copy link
Contributor

@donaldpipowitch Yes. it'll throw an error if the specified script is missing from any one of the packages.

@ianstormtaylor
Copy link
Contributor Author

Skipping does sound like it could be useful to me too. @donaldpipowitch do you have a specific use case in mind?

@spion
Copy link

spion commented Feb 6, 2018

Hi! Sorry about the duplicate comment - just thought I'd mention (since the RFC does not list alternatives) this alternative:

https://github.com/whoeverest/wsrun

which was made specifically with yarn in mind.

@DeltaCircuit
Copy link
Contributor

@ianstormtaylor IMHO, skipping would make unintentional side effects. I think it's better to let the user know that script doesn't exists in that package rather than silently (with a subtle warning) skips it.

In a CI pipeline, what if the user forgot to add the test script (it's rare, but just in case) to one of the packages? Even though if we throw a warning (silent) for missing script so that the CI would fail, I guess its more than just a "warning". It should be treated an "error".

Clearly, you guys are the pro(s) around here. So, what do you think?

@jamiebuilds
Copy link
Contributor

In Bolt we went with the flags --ignore/only [name glob] and --ignore-fs/only-fs [path glob] which covers everything and are really obvious names

@ianstormtaylor
Copy link
Contributor Author

@thejameskyle that sounds like a good way to go to me! Good point that the "workspace names" are always going to be valid paths anyways. I'd suggest using --ignore-path/only-path instead of fs.

(I almost submitted this comment asking what does fs stand for there before figuring it out 😄 )

@jamiebuilds
Copy link
Contributor

Yeah I'd be happy with --ignore-path/only-path as well.

We already have implementations for many of the pieces of this btw:

@jamiebuilds
Copy link
Contributor

Also this is Bolt's full API (some of which is unimplemented:

# Run a command on your project package
bolt project/p [add/upgrade/remove/run/exec]
bolt project/p [script name]

# Run a command on a single workspace by its name
bolt workspace/w [workspace name] [add/upgrade/remove/run/exec/link/unlink]
bolt workspace/w [workspace name] [script name]

# Run a command on all of your workspaces
bolt workspaces/ws [list/add/upgrade/remove/run/exec] <...workspaces flags>
bolt workspaces/ws [script name] <...workspaces flags>

# Run a command on changed (with git) workspaces
bolt changed/c [list/install/run/exec] <...workspaces flags>
bolt changed/c [script name] <...workspaces flags>

Anything that runs on multiple packages gets the workspaces flags --only/ignore/only-fs/ignore-fs

@ianstormtaylor
Copy link
Contributor Author

@thejameskyle looks really good!

@donaldpipowitch
Copy link

donaldpipowitch commented Feb 7, 2018

Skipping does sound like it could be useful to me too. @donaldpipowitch do you have a specific use case in mind?

It seems to become a habit while working with Lerna that many of our packages don't share some commands in all cases. Say we develop some React components in one monorepo and we add several examples as standalone applications showcasing these components, than the components maybe have a unit command to run unit tests which the examples don't (necessarily) have and the examples maybe have a serve command, because they have an index.html file in their root which can be served which the components don't have.

I'd be fine with some additional param like --skip-missing/--ignore-missing to opt-in into this Lerna-like behavior, so I wouldn't need to add additional filtering with an additional --ignore/--only pattern. (That would be a different use case for: --ignore some package which has this command, but shouldn't be executed.)

@jonaskello
Copy link

jonaskello commented Feb 22, 2018

Both lerna run and oao run-script seems to be doing fine by just skipping missing scripts. This seems to be the community standard, so if yarn will not follow that, I think there should be a really good reason. I think the deviation from de facto standard should be motivated with a real-world scenario where skipping is really bad. I cannot think of one myself, but I can think of many real-world scenarios where skipping is the wanted behaviour (like the ones mentioned by @donaldpipowitch above), and I think that is why both lerna and oao went that route.

If skipping is the more commonly wanted behaviour (which it seems to be given both lerna and oao does that and AFAIK none of their users has complained) then it follows that verifying that all packages have a script is the less common scenario. I would therefore propose that it would be better to have a switch like --all that can be added when you want to be 100% sure that all packages have the script. With the --all switch, yarn would error if a package is missing the script. Without it yarn would follow standard behaviour and just silently skip missing scripts.

We have several monorepos and I have never felt the need to make sure that all packages have a certain script. The most common scripts we have are start, build, bundle, test, graphql-code-gen. Not all packages have those scripts.

The only scenario I've seen mentioned in this discussion as a use-case for forcing all packages to have a script is running the test script in a CI pipeline mentioned by @giridharangm. In that scenario I think the --all flag could be used. I also agree with @giridharangm that forgetting to add the test script is rare and if yarn should do something it should be an error and not a warning. Warnings are not very helpful in a CI pipeline scenario. Maybe I have missed some other scenarios mentioned by others?

@donaldpipowitch
Copy link

Short question: Was it ever discussed alongside with this RFC to include meta data about the called processes/scripts as environment variables?

Say you call yarn workspaces run watch in a workspace-based project with three packages and all three packages need to listen to a unique port. You could use some "find open port"-lib for this, but because of data races this can become quite flaky in my experiences, when watch is called parallel. It would probably be less error prone, if we could access an environment variable like YARN_WORKSPACES_RUN_INDEX which has the values 0, 1 and 2 to indicate that the script was called nth-times by yarn in this case and we could use this value to define a port in the watch script like this: const port = 8080 + parseInt(YARN_WORKSPACES_RUN_INDEX || 0, 10).

@lukebatchelor
Copy link

lukebatchelor commented Jan 10, 2019

👋
I'm looking at spiking some of this, specifically the --workspaces and --packages flags. Just wanted to double check if those are the final decision, or whether the --only, --onlyFs, --ignore, --ignoreFs from bolt might be considered (I actually implemented them in bolt and in lerna).

Happy to go with either though, --workspaces and --packages should be flexible enough for what we need, but I find bolts API easier to work with (no negating globs).

Also, I see that it was suggested to add yarn workspaces exec as well. This is actually another thing we need in our project, but I actually found that it's possible right now with a (possibly unintended, definitely undocumented) work around.

Yarns exec command can run directly in the workspaces

$ yarn workspaces run exec pwd

yarn workspaces v1.12.3
yarn exec v1.12.3
/Users/lbatchelor/src/yarn-workspaces-testing/packages/bar
✨  Done in 0.07s.
yarn exec v1.12.3
/Users/lbatchelor/src/yarn-workspaces-testing/packages/baz
✨  Done in 0.07s.
yarn exec v1.12.3
/Users/lbatchelor/src/yarn-workspaces-testing/packages/foo
✨  Done in 0.07s.
✨  Done in 1.33s.

The noise can be cleaned up pretty easily too

$ yarn -s workspaces run exec -s pwd
/Users/lbatchelor/src/yarn-workspaces-testing/packages/bar
/Users/lbatchelor/src/yarn-workspaces-testing/packages/baz
/Users/lbatchelor/src/yarn-workspaces-testing/packages/foo

Just wanted to check if that was intended. I'd be happy to document it if so.

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.

9 participants