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

Support for Monorepos / Workspaces #43

Merged
merged 6 commits into from
Oct 26, 2020

Conversation

charlespierce
Copy link
Contributor

Improve the ergonomics of working with Volta in a monorepo environment.

Rendered RFC

Copy link
Contributor

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Some general thoughts:

  • It would be really nice to see some basic / simplistic ASCII diagrams of file folder layouts as you explain the resolution bits. Like:
.
├── package.json
└── packages
   ├── bar
   │  └── package.json
   └── foo
      └── package.json
  • It should be a bit clearer that the yarn: false (and it is inferred but not stated, npm: false) bits are unrelated to having multiple package.json files (e.g. this is feature in its own right).

  • The prose is a bit confusing on what exactly goes in extends. I am thinking it is a relative path from the current file to another manifest file that has a volta property? Does that target manifest have to be package.json, or can the file name be any value?

Comment on lines 55 to 57
## Pinning Tools

When a user runs `volta pin`, we will always make the changes to the _first_ `package.json` that we find. This means that if a user wants to `pin` a tool for the root project, they will need to first navigate to the root directory (out of any subprojects) and _then_ run `volta pin`. This gives users the most flexibility to control where they want to `pin` tools, without trying to guess which location they were intending or requiring interactivity.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make the behavior of volta pin workspaces aware? Most of the time when you are working in a monorepo you are running your terminal in the top level project root. Assuming that is the case, then it seems like we could make volta pin notice that there is a workspaces property in that package.json and from that we can update all of the package.json files for you (with the right volta: { extends: '../../package.json' } bit.

Having to do it manually is fine (and the result with extends proposed here is much better), but IMHO we can still do exactly what the user wants 98% of the time...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree we want to do what the user wants, but I'm also a little wary of a command that normally only affects a single package.json suddenly having knock-on effects in a bunch of different files. My concern is that if someone wants to pin the top-level without updating the subprojects to have extends, then we don't really give a way to do that (apart from manual edits). Either that or we have to add a flag to the CLI that only applies if you are A) In the root of a Yarn Workspaces project and B) Trying to pin the top-level without making any changes to subprojects, which is a little to specific to be useful as a command-line flag, IMO.

That said, the benefits to getting started with a workspace (where we know we have a reverse mapping to the individual projects) are very nice, so it feels like it would be a good QoL improvement. I'll add it and we'll see if there are other comments / issues raised about how it works.

text/0000-monorepo-support.md Outdated Show resolved Hide resolved
text/0000-monorepo-support.md Outdated Show resolved Hide resolved
@charlespierce
Copy link
Contributor Author

@rwjblue Thanks for the comments! On the yarn: false thing, I was thinking that it was related to having multiple roots, but you're right that we already do a merge and so it would be a new feature unrelated (though applicable to this use-case). I'll actually remove that entirely and make it a separate RFC.

Thanks for the suggestion on how to format some examples! I wanted to include some, but struggled a bit with how to format everything and eventually decided getting it out in some form was better, but I like that layout and should be able to make some!

For the extends, I'll update the text, but you're right that it's a path to another manifest file. It doesn't strictly have to be a package.json, but it does need to be a JSON file with a top-level object that has a volta property. It also doesn't necessarily need to be a relative path (though it probably should be if you want to collaborate with other people).

@charlespierce
Copy link
Contributor Author

Updated to improve the description in a few places and add examples to the Details subsections

Copy link
Contributor

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

@charlespierce - The updates look great to me, thank you for iterating on them!

ef4 added a commit to embroider-build/embroider that referenced this pull request May 29, 2020
This is needed until volta's monorepo support (volta-cli/rfcs#43) lands.
@dherman
Copy link
Contributor

dherman commented Jun 2, 2020

I'd like to add another option into the discussion, which is to have a less explicit linking model, closer to how I believe both Yarn and Cargo do workspaces. In this model, instead of "extends" being the way a nested package connects itself to the workspace, it would be the root manifest of the workspace that would indicate which nested packages are part of the workspace.

The reason I bring this up is that I'm concerned that with explicit backlinks, it requires the creator of each nested package to take responsibility for participating in the monorepo. For some monorepo use cases, there are separate teams responsible for the maintenance of individual packages vs the maintenance of the overall monorepo. With some sort of declarative language (typically, filesystem paths + globs) for include- and/or exclude-lists, a central team can manage the overall monorepo and establish conventions so that other teams can add new packages without having to know how to "opt in" to using the monorepo's configuration.

I mentioned this to @charlespierce yesterday and he brought up a few concerns to consider:

  • Performance: This would likely require more filesystem crawling. Potentially expensive, but would be useful to do experiments to see if this is a problem in practice or not.
  • Ability to use non-package.json manifests (as discussed in #282): I think this is solvable orthogonally, but worth thinking through.
  • Nesting: Does this rule out the idea of a workspace within a workspace? Is that important? We weren't sure.

Nevertheless, I think it's worth discussing because we should think about how companies or large open source projects with large monorepos would be able to modularize the responsibility of managing the workspace configuration. My concern is that the RFC as-is diffuses responsibility for managing the workspace configuration to all sub-packages.

@charlespierce
Copy link
Contributor Author

@dherman Updated the critique to include your points about the modular responsibility. While I ultimately come to the same conclusion, given the nature of Volta, I think it's important to call out, thanks!

@charlespierce charlespierce changed the base branch from master to main June 15, 2020 17:30
@tujoworker
Copy link

Both Prettier and ESlint are supporting packages to extend with:

  "prettier": "@scope/prettier-config-package",
  "eslintConfig": {
    "extends": "@scope/eslint-config-package"
  },
  "volta": {
    "extends": "../../volta-config-package/volta-config.json"
  }

For Volta, as for now, we have to have json file, either directly as a package.json or another json file.
Is that RFC about that?

I would love have this feature, because it makes handling of configuration in larger mono-repos, very much easier.

@charlespierce
Copy link
Contributor Author

Hi @tujoworker, this RFC right now is about the ability to use another JSON file to extend the Volta config. Can you expand a bit on the feature you're asking about? I'm not sure I see a clear path to Volta "extending" from packages, since Volta sits at a higher level than individual packages. One possible option would be to support a folder instead of a file, and then automatically append package.json to it, but I'm not sure that improves things much over the direct path.

The difficulty with using a package is that it creates a Catch-22: In order to install the dependencies, we need to know the Node / npm / Yarn version to run. But in order to know that, we would have to have dependencies installed so that we can "extend" from a package.

@tujoworker
Copy link

Yeah, I will expand a bit; the thoughts are - to have the config only in a separate and dedicated package.
Because workspace packages can be placed where ever they are defined in the workspace config, the paths to the json file can be different from package to package.
So, defining the Volta config inside an independent package - without any dependencies by itself - would make it both declarative and easy to refer to - in contrast to a path/folder.

But if this really is not possible because of the catch-22 scenario - then just ignore my thoughts / wishes.

@rwjblue
Copy link
Contributor

rwjblue commented Oct 26, 2020

@charlespierce - This seems to be fully implemented, should we merge?

@charlespierce
Copy link
Contributor Author

@rwjblue Yeah, I think so. There are still some outstanding questions about the possibility of supporting some sort of automatic hierarchy, but I think that's more fine-tuning the existing implementation than completely rewriting it.

@charlespierce charlespierce merged commit 087245e into volta-cli:main Oct 26, 2020
@charlespierce charlespierce deleted the monorepos branch October 26, 2020 17:19
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.

4 participants