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

Volta doesn't look at parent package.json in monorepo workspaces #862

Closed
RoystonS opened this issue Oct 26, 2020 · 5 comments
Closed

Volta doesn't look at parent package.json in monorepo workspaces #862

RoystonS opened this issue Oct 26, 2020 · 5 comments

Comments

@RoystonS
Copy link

Volta (0.9.1) currently abandons its search for package information too early when used in monorepos. It consults only the local package's package.json instead of walking up the directory tree and checking the workspace package.json as well.

Let's say I have a typical monorepo containing a top-level package.json and a child package with its own package.json:

  • monorepo/package.json
  • monorepo/packages/pkg1/package.json

(typically there'd be many such child packages, but I'm keeping the example simple here.)

I'd expect the following behaviour:

  • Running Volta inside the top-level monorepo folder would consult monorepo/package.json when looking for package version specifiers. This does work correctly.
  • Running Volta inside the pkg1 folder would consult the local pkg1/package.json when looking for package version specifiers. This works correctly.
  • For package specifiers not found in the local pkg1, Volta should continue up to the parent directory (following something similar to the node package resolution process) and check for package specifiers there too. This does not currently appear to happen. As a result, Volta incorrectly ignores tool pinning and package versions intended for use across the entire monorepo.

Here's a tiny example monorepo, with the structure as above. The top-level package.json contains some Volta tool pinning and the child package does not: https://github.com/RoystonS/volta-monorepo-issue

Here's an example of the output with such a repo:
image

My top-level package.json has node pinned at 12.8.1 and yarn pinned at 1.22.10. However, inside pkg1, I instead get the global default version of node (12.19.0 in my case) and no yarn because I haven't selected a default version.

I'd expect Volta to be continuing up and scanning the workspace package.json instead of deciding - having found a package.json inside pkg1 - that its search was complete.

I can see some discussions of this issue in #378, which was closed some time ago, and there are other complicated discussions of rc files. I'm hoping that in order to get things working correctly in monorepos it's "just" a case of walking up the tree, looking for package.json files?

@rwjblue
Copy link
Contributor

rwjblue commented Oct 26, 2020

Monorepo support was added by way of adding a new extends option to the package.json configuration. I've sent a PR to your demo repo that should work properly (RoystonS/volta-monorepo-issue#1), but the main docs for this are at https://docs.volta.sh/advanced/workspaces.


As to why we didn't go with just walking upwards to find any parent package.json's I think the RFC (volta-cli/rfcs#43, which I think should have been merged but I asked @charlespierce over in that thread to confirm) has the best answer (from the " Requiring Changes in each Subproject" section):

  • It obfuscates the source of the platform - A user trying to figure out why we are using a specific Node version will have to do the same digging to figure out where the settings are. In our model, the user can look at the subproject and it will point them to the root project (or next project in the chain), so they always know where to start when investigating.
  • It requires either duplicating the workspace configuration or tying our implementation tightly to how the package managers define workspaces:
    • If we have our own setting for indicating which projects are part of the workspace, then users will have to duplicate that setting between Yarn / npm / pnpm / lerna and Volta, which comes with additional maintenance costs as they will also need to keep them in sync.
    • If we instead chose to re-use the existing "workspace" key used by Yarn et al., we will need to ensure that our parsing aligns with those other tools exactly, so that we don't accidentally get out of sync ourselves. This also raises the question of how do we handle it when two tools diverge on their implementation?
  • It requires more speculative IO. This is likely less of a significant concern, but is definitely something we would need to plan for and monitor to ensure we don't have significant performance regressions.

@RoystonS
Copy link
Author

@rwjblue Thank you. That's a really helpful response. I hadn't spotted that bit of config (nor the rfcs repo).

Yep, I totally get the concern about the extra IO. I'm slightly less convinced about the obfuscation concern: Node itself already walks up directory trees looking for packages. And for the workspace configuration, I don't think any parsing of workspace keys would have been necessary: just walking upwards would suffice.

But, extends is fine. I'm used to it with eslint, tsc and friends. It wasn't my expectation, as I was expecting package resolution to be more automatic and Node-y than needing an explicit extends, but it's fine, and it certainly solves IO concerns.

@rdsedmundo
Copy link

I also expected this to be available out of the box. Repeating the extends for every package in the workspace is cumbersome. For not relying on the package.json#workspaces volta could simply introduce a .voltarc file and look for it instead (this file would accept the same configs from package.json#volta.

#282 commenters also talked about something similar.

@RoystonS
Copy link
Author

RoystonS commented May 9, 2021

Yes, in practice it turns out to be quite a pain. If I have a default Node version of v14, and a monorepo with 10 packages in it, 9 of which are set to Node v12 via an extends clause, and the 10th was accidentally unset because it was created by another developer who didn't realise they had to do that, it can be really annoying to try to diagnose why that 10th one behaves differently, because it's running with Node 14. It's never the behaviour I want.

So, in practice, I'm finding that - contrary to the text that @rwjblue quoted - the current behaviour actually serves to obfuscate the platform-sourcing behaviour somewhat because it goes against the expectations set by other NodeJS tools which tend to inherit settings down through the file-system. eslint, for instance, actually requires an explicit root: true directive to prevent it inheriting settings.

I'm not going to reopen this issue myself as we're almost used to behaviour now. But it is an irritant that requires me to explain this behaviour every developer in my org who comes across Volta.

@charlespierce
Copy link
Contributor

@rdsedmundo and @RoystonS thanks for the extra context! I appreciate the point about how Volta's approach is breaking the expectations of other tools in the NodeJS ecosystem, which is a significant issue. Between #282 mentioned above, this and #983, I think there's definitely room for significant improvements to Volta's resolution logic. It's not a trivial change and will require a fair amount of design work, but there's a lot of value to be found.

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

No branches or pull requests

4 participants