-
Notifications
You must be signed in to change notification settings - Fork 240
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
RFC: npm workspaces: auto switch context based on cwd #343
Conversation
We talked about this in today's Open RFC call. What are your thoughts about this:
This way, since there's really two "expected behavior" models here (one is, context-aware, i'm inside a workspace root so you should know that; another is, not context-aware, i cd'd into a project so it should act like a project by itself), this provides a way that both defaults to the context-aware model but uses a very explicit config mechanism so that users can easily discover and override the behavior when desired. |
thanks for the notes @ljharb I also watched the record 😊 Another possible problem I had in mind was the mechanism for auto switching the cwd context is compatibility with the current Lerna (and similar tools) workflows. I do believe the proposed idea of having a |
I am working in a workspace with the current implementation (mostly to test it all out) and I think this RFC covers it as is, but it is unclear how the addition of a One part which is not called out, but is also required, is that references to workspace sibling packages should be installable when cd'd into a child workspace or run with |
We used to resolve such sibling dependencies by hand using Edit: So do we need anything more than having an |
one idea floated at npm/cli#2032 that might be interesting exploring is to alternatively printing a warning message instead of the auto switch, similar to "Did you mean":
|
One thing I'd also like to put out for consideration is how things like this will work for nested monorepos; I have a set of repos currently using lerna and npm 6, where one is a git-subrepo in the other such that it can be exported to a public repo. I use a special post-install script that is able to export a subset of the lock to the subrepo, that way the public repo can be used as though it were never nested and things are always kept in sync. I have a prototype I had planned to merge to use yarn 2 workspaces for this (using some trickery to ensure it keeps walking up to the next root). But, I'm testing out npm 7 now that it no longer completely breaks when workspaces are nested (and have a comparable script using arborist to prune a new lock), and it would be a shame if the context-aware setup were to stop looking for the root too early. At least with yarn 2 I can use plugins to trick it into doing what I want; npm offers no comparable way, but "thankfully" commands are not yet context aware anyway. |
@ruyadorno I feel like doing the inference on all command runs is a lot of overhead with little benefit (especially if we only decide to output some contextual log similar to "Did you mean"); I'm a big fan of all the recommendations proposed by @ljharb here & think we should update the RFC to reflect those implementation details & maybe rename this to "npm workspace root context/configuration" or similar (ie. dropping the "auto") |
Implementation plan:
Result, given a project with a workspaces at
Open Question(s)
|
This is incompatible with my existing use cases - every single folder with a package.json i have also has an npmrc with package-lock=false specified as well as some values for npm version. |
Those are not read or loaded when those folders are first-class workspaces, though. Only when they're the nearest path found by walking up from the current working directory. You can preserve your current use-case (such as it is) by putting Almost every config value (other than workspace-root) is incompatible in a workspace-specific setting, because workspaces are reified in a single project. (Or, at least, if not fundamentally "incompatible", then at least "wildly complicated to implement".) Consider if you set |
@isaacs yes, i'm saying that my use case is "cd into the subproject, and run all commands in there" which means the subproject's npmrc file needs to be able to apply, even if workspace-aware commands end up proxying to the workspace root. The whole point of this config value is that not every use of workspaces is "as a single project". |
Ok, so, let's say you have a workspace at If I If we treat If you want to use "workspaces" as "not a single project", then I think the right approach might be that you don't have "workspaces", you just have "a monorepo with a bunch of different project folders". Like, in your case, I think you wouldn't really want |
Why is that a problem? In that scenario, the dependencies of foo and bar are potentially different packages, and it seems like a bug to me that installing at the root gets them from the same registry and thus dedupes them. "Monorepo with a bunch of different project folders" is a primary use case for lerna and yarn workspaces, and I think it should also be one for npm. I agree they shouldn't have their configs smooshed together, but I'd still want those two commands to actually do the same thing - which is, " |
"Use different configs for different workspaces being operated on by the same top-level npm command" is a much bigger RFC than this one, and would require massive refactoring of our workspace and configuration implementation. It's only being exposed here because we are contemplating having Consider something like If we say that every workspace operation is a whole separate instance of npm, with different configs and everything, that's going to make it much less suitable for the use case of "a bunch of related packages intended to be used together, which ought to be continually integrated in development". |
The challenge, i think, is that every config value is not the same. Some, like loglevel, don’t make sense on a per-child-project level. Some, like the npm version ones, absolutely do. |
Unless I'm mistaken, having each workspace explicitly list which root they belong to means that I'll never be able to port my projects (which are laid out as a nested pair of monorepos, as described in #343 (comment)), so that's not super exciting to hear. If I have yarn 3 working pretty well with a plugin, at least. |
@jakebailey So, if I understand this correctly, you end up with a folder structure kind of like this:
And then, whenever you install in In that case.... yeahhhh.... this gets complicated 😅 We definitely do not support nested workspaces today. As a matter of reasonable policy and support commitments, I'm inclined to say that if you're using a project structure we explicitly don't support, and you're updating lockfiles and Of course, we're kind folks, and don't want to break anyone's workflows, at least not without providing some blessed path forward for you. Because, yeah, you're right, if you're using nested workspaces, and we move forward with this plan, then an install from Maybe the best path forward is for us to take a step back from this, and consider how nested workspace projects might be a thing we could support properly, and then revisit the implicit workspace/root stuff. Alternatively, we could decide "no, nested workspaces is not a thing we'll support any time in the foreseeable future", so your path forward will be to just put There is no reasonable approach to inferring the workspace name and project root from the cwd that doesn't involve some kind of explicit marker in the workspace project itself. But, maybe we could also go back to the idea of putting it in @ljharb Workspace-specific configs will definitely require a major refactor of our config layer. The bad news is, that's a big breaking change, which will come at soonest in npm v8 (or maybe v9, if v8 is just "drop support for node 10, but no other changes"). The good news is, it's one we're planning already. |
At the moment, they are using lerna where each package has their own lock and unhoisted |
8857e1a
to
2e23a97
Compare
cleaned up the RFC to reflect the direction we decided to take (which ended up being much closer to the original proposal and different from all the comments previously in this thread). |
|
||
## Implementation | ||
|
||
Add a recursive check that walks up the file system structure and validates if the current `prefix` is defined as the **"workspaces"** property of a `package.json` file of a parent directory as part of `@npmcli/config`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not use the "prefix" config for anything new. It breaks nvm, and the $PREFIX
env var breaks termux.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh don't worry this is only internal implementation details here but now that you mentioned I double checked and it's actually incorrect, it should mention localPrefix
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref: npm/config#28
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for confirming :-)
wip: npm/config#28 |
@ruyadorno do you feel npm/cli#4338 should be handled in the context of this RFC or would it be a separate RFC? |
@thernstig the way it is described in npm/cli#4338 makes it sound like a different RFC (feel free to start out as an RRFC issue) that can possibly be incompatible with the work we're describing here? My hope here is that landing the features from this RFC will be enough to prevent confusion for workspaces users. |
Follow up from our OpenRFC meeting: I'll document the possibility of setting |
@ruyadorno imagine a user goes |
it should not fail, it should respect the config values defined at the |
thanks @thernstig that's probably a good point to also add to the RFC before approving it 😊 I'll make sure it's there |
96d5104
to
6ec6b9f
Compare
npm workspaces: auto switch context based on cwd
Summary
Running workspace-aware commands from the context of a child workspace folder should work as expected.
Motivation
Most users expect to be able to navigate to a nested workspace directory and use npm commands in that context.
Unfortunately that might not work as intended depending on the command, for example a user trying to add a new dependency
<pkg>
would expect tocd ./<workspace-folder>
andnpm install <pkg>
to produce a valid install tree adding the new dependency to that workspace but instead that creates a separate install within that directory, failing to hoist/dedupe dependencies to the top-level and generating an extra and unintended package-lock file.Detailed Explanation
Given that the cli currently support workspace-aware commands (via
--workspaces
and--workspace
configs) a potential fix to that mismatch between the expected user experience and the current behavior of the npm cli is to automatically switch the internal context to ensure that if the user is trying to run a command that already support the workspaces configurations.Rationale and Alternatives
--workspace
config rather than automatically switching the context.ci|dedupe|find-dupes|install-ci-test|install-test|install|link|rebuild|update|uninstall|update
.npmrc
or cli)Implementation
Add a recursive check that walks up the file system structure and validates if the current
prefix
is defined as the "workspaces" property of apackage.json
file of a parent directory as part of@npmcli/config
.In case that is so, we can then tweak the internal
localPrefix
config value to point to the top-level workspace while defining the current working directory as the workspace configuration (-w {cwd}
) to be used.Since this is going to be the new default behavior inside any workspace folder, a way to opt-out is to use the
--no-workspaces
config option. Another possible way is to have a.npmrc
file within the workspace folder that setsworkspaces=false
in order to always opt-out of treating that folder as a workspace of the root project.More examples:
Given a workspace-configured project as such:
These variations should be functional equivalent:
$ npm install abbrev -w packages/a
$ cd ./packages/a && npm install abbrev
$ npm run lint -w packages/a
$ cd ./packages/a && npm run lint
$ npm version patch -w packages/a
$ cd ./packages/a && npm version patch
$ npm install english-days --ws
$ cd packages/a && npm install english-days && cd ../b && npm install english-days
$ npm test --ws
$ cd packages/a && npm test && cd ../b && npm test
Note: When auto switching context the
.npmrc
file containing config values located at the root project should be respected. A warning is thrown in case a.npmrc
file is present in the current working directory when auto-switching.Prior Art
node_modules
folder when running from a child workspace folder, both are equivalent:yarn worskpace <workspace-name> add <pkg>
cd <workspace-path> && yarn add <pkg>