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

RFC: npm workspaces: auto switch context based on cwd #343

Merged
merged 1 commit into from
Feb 15, 2022

Conversation

ruyadorno
Copy link
Contributor

@ruyadorno ruyadorno commented Mar 17, 2021

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 to cd ./<workspace-folder> and npm 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

  • Use the logic to figuring out if a workspace-aware command was run from the context of a child workspace folder but only print a warning message to users, similar to our existing set of "Did you mean?" warning messages. Educating the user on the usage of the --workspace config rather than automatically switching the context.
  • Only implement the auto-switch behavior in arborist-reify related methods. namely: ci|dedupe|find-dupes|install-ci-test|install-test|install|link|rebuild|update|uninstall|update
  • Make it so that the auto-switch behavior of the cli is disabled by default and needs some type of opt-in mechanism.
  • Implement the auto-switch behavior as an opt-in with very explicit ways of activating it (e.g: via a config option that can be set in .npmrc or cli)
  • Do not implement any auto-switch mechanism

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.

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 sets workspaces=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:

.
├── node_modules/...
├── package.json
└── packages
    ├── a
    │   └── package.json
    └── b
        └── package.json

These variations should be functional equivalent:

  1. Running from the top-level using the workspace config
  2. Changing the current working dir and then running the command
  • Adding a new dependency to a nested workspace:
    • $ npm install abbrev -w packages/a
    • $ cd ./packages/a && npm install abbrev
  • Running a script in a nested workspace:
    • $ npm run lint -w packages/a
    • $ cd ./packages/a && npm run lint
  • Bumping the version of a nested workspace:
    • $ npm version patch -w packages/a
    • $ cd ./packages/a && npm version patch
  • Adding a new dependency to all nested workspaces:
    • $ npm install english-days --ws
    • $ cd packages/a && npm install english-days && cd ../b && npm install english-days
  • Running tests in all nested workspaces:
    • $ 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

  • Yarn v1 will make sure to add a new dependency in the top-level 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>

@darcyclarke darcyclarke added Agenda will be discussed at the Open RFC call Proposal labels Mar 24, 2021
@ljharb
Copy link
Contributor

ljharb commented Mar 24, 2021

We talked about this in today's Open RFC call. What are your thoughts about this:

  1. by default, nothing is context-aware
  2. add a --workspace-root arg/config that can point to, for example, ../, that explicitly opts a command in to being context-aware
  3. a workspace-aware npm init would create an .npmrc with workspace-root=path/to/root in it inside the child workspace
  4. we should also add an npm workspace init command that is used to convert an existing project into an "npm workspaces" project, which would also workspace-root= into .npmrc files into each child workspace.
  5. every command that is workspace-aware becomes context-aware, iff the child workspace the command is run in is in fact a configured child workspace of the workspace root.

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.

@ruyadorno
Copy link
Contributor Author

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 workspace-root config would be a great way to make it safer but I'm only concerned about the extra reading .npmrc which is not at all there yet.

@wesleytodd
Copy link

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 workspace-root would effect this. I want to ensure we are clear that the section here will still remain true with this addition.

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 -w. Currently (npm@7.9.0) installs fail if you cd into the child and depend on a workspace sibling package. Right now I consider this a bug, but depending on when this rfc lands and is implemented I am not sure it is worth spending time to fix.

@panayot-cankov
Copy link

panayot-cankov commented Jun 8, 2021

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 workspace-root would effect this. I want to ensure we are clear that the section here will still remain true with this addition.

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 -w. Currently (npm@7.9.0) installs fail if you cd into the child and depend on a workspace sibling package. Right now I consider this a bug, but depending on when this rfc lands and is implemented I am not sure it is worth spending time to fix.

We used to resolve such sibling dependencies by hand using npm link or referring them by path. I love the idea of resolving the context by reading the package.json files in parent directories, checking if the current package is in a parent's workspaces. And from that point on, things like npm ls to execute aware that some deps may be installed at root level and not in the current workspace. When npm init is performed locally - why and when do you do that? I do when I want to work on a repo in isolation, and would expect all modules to be installed locally in the node_modules of the child workspace, so it doesn't refer anything installed at the workspace root, but then sibling dependencies I'd expect them to be treated as if they were described as "mysibling": "../mysibling" in the dependencies and npm make a symlink.

Edit: So do we need anything more than having an npm i in a child workspace filter the installed dependencies to that child's dependencies and just install them in the root workspace?

@ruyadorno
Copy link
Contributor Author

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":

Command not supported inside a workspace; please run it from the project root directory.

@jakebailey
Copy link

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.

@darcyclarke
Copy link
Contributor

@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")

@isaacs
Copy link
Contributor

isaacs commented Jul 30, 2021

Implementation plan:

  • When reifying, place a .npmrc file in each workspace folder, containing workspace-root = {relative path}
  • Add a new internal-only config level 'workspace'
  • When loading the project config level, if the found project folder has a .npmrc file containing workspace-root={path}, then load it as the 'workspace' config. Set the workspace-root and workspace configs at the 'workspace' level, and load the 'project' config level from the workspace root.
  • If a workspace root is set, then that is the localPrefix, and the 'workspace' config is set to the relative path from the workspace-root to the nearest project folder, at the 'workspace' config level.
  • No other config values may be placed in a .npmrc file that specifies workspace-root. (Avoid "which one of these is relevant" foot-gun.)

Result, given a project with a workspaces at packages/foo and packages/bar:

  • npm i - creates files packages/foo/.npmrc and packages/bar/.npmrc, both containing workspace-root=../..
  • cd packages/foo; npm <cmd> - identical to npm <cmd> -w foo
  • cd packages/foo; npm <cmd> -w bar - identical to npm <cmd> -w bar (ie, implicit workspace overridden by explicit workspace defined at higher config priority level)
  • rm packages/foo/.npmrc; cd packages/foo; npm i --workspace-root=../.. - identical to npm i -w foo (workspace-root config can be set anywhere)
  • cd packages/foo/a/b/c/d/e; npm <cmd> - identical to npm <cmd> -w foo (ie, walks up from cwd to find workspace and project roots)

Open Question(s)

  • Should the {workspace}/.npmrc file containing workspace-root be created by default? (Ie, should --save-workspace-root default to true or false?)
    • pro - less likely to have multiple workspaces, where implicit behavior is enabled in some, and not others. Just Works (without the ambiguity identified as a problem previously)
    • con - may be considered a breaking change from current behavior (perhaps this is only relevant if current behavior is desirable?)
    • con - adds a file to the workspace folder, which we weren't currently doing. (Prevents the "workspaces that dont' know they're workspaces" use case, but that's not something we support particularly well anyway. Do we wish to?)
    • pro - break crisply for anyone currently using the workspace .npmrc file as a project config (since that doesn't work right now anyway, and likely never will)

@isaacs isaacs mentioned this pull request Jul 30, 2021
4 tasks
@ljharb
Copy link
Contributor

ljharb commented Jul 30, 2021

No other config values may be placed in a .npmrc file that specifies workspace-root.

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.

@isaacs
Copy link
Contributor

isaacs commented Jul 31, 2021

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 save-workspace-root = false in your root level .npmrc file, so that those workspace-specific .npmrc files (which can only set the workspace-root) will continue being ignored. Performing actions within those workspace folders as the cwd will not infer a workspace context, and will continue to work as they currently do.

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 offline = true in one workspace, and prefer-online = true in another. Or different auths, or registry settings, or loglevels. For anything we do from the root project level, we would have to track every config-dependent action per-workspace, which is just not tenable.

@ljharb
Copy link
Contributor

ljharb commented Jul 31, 2021

@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".

@isaacs
Copy link
Contributor

isaacs commented Aug 2, 2021

Ok, so, let's say you have a workspace at foo which has a .npmrc file containing @myscope:registry = https://registry.foo.com, and another workspace at bar which has a .npmrc file containing @myscope:registry = https://registry.bar.com. The root project has a .npmrc file containing @myscope:registry = https://registry.root.com

If I cd foo ; npm i, and we do not treat it as a workspace, no problem. The modules in foo/node_modules/... will be installed with the @myscope packages being fetched from https://registry.foo.com. They won't be workspace-aware at all. Same if I cd bar ; npm i. However, if I install from the root project, then the dependencies of foo and bar will all be fetched from https://registry.root.com/.

If we treat cd foo ; npm i as an implicit npm i -w foo in the root project, then this poses a problem. It gets even more confusing if they have different loglevel, audit-level, cache, prefer-online/offline/prefer-offline settings, and so on. We can't reasonably "shadow" the root project configs, because all workspaces are reified in a single tree, in a single command invocation.

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 cd foo ; npm <cmd> to be the same as npm <cmd> -w foo, because you don't want them being reified together or sharing configs.

@ljharb
Copy link
Contributor

ljharb commented Aug 2, 2021

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, "cs foo; npm <⌘>, since that should be the canonical source of truth.

@isaacs
Copy link
Contributor

isaacs commented Aug 3, 2021

"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 cd foo ; npm <cmd> be identical to npm <cmd> -w foo.

Consider something like loglevel. If one workspace sets loglevel=verbose and another sets loglevel=silent, and we run npm i -w foo -w bar, should we log at the verbose level, or not at all?

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".

@ljharb
Copy link
Contributor

ljharb commented Aug 4, 2021

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.

@jakebailey
Copy link

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 .npmrc files aren't checked in, then it could at least be set up so that when there's a parent root workspace that includes the nested workspace, the .npmrc files can differ to ensure it works, but if they are intended to be checked in and aren't created on the fly, then that won't work as a workaround.

I have yarn 3 working pretty well with a plugin, at least.

@isaacs
Copy link
Contributor

isaacs commented Aug 11, 2021

@jakebailey So, if I understand this correctly, you end up with a folder structure kind of like this:

project1  { "workspaces": ["packages/*", "packages/project2/packages/*] }
+-- packages
    +-- foo { "name":"foo", "version":"1.2.3" }
    +-- project2 { "workspaces": ["packages/*"] }
        +-- packages
            +-- bar { "name":"bar", "version":"1.2.3" }

project1 is the project root, with foo and project2 (and project2's workspaces) as workspaces.

project2 is a git submodule, but when checked out on its own, is also a project root with bar as a workspace.

And then, whenever you install in project1, you have some custom scripts that munge the lockfiles and keep everything in sync, so that updates to bar's deps will get tracked in both project1 and project2?

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 node_modules out of band, then essentially you're depending on internal APIs, and sorry, your setup might get broken even within a semver-major line.

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 project1 will place workspace-root = ../../../.. in project1/packages/project2/packages/bar/.npmrc, and then a subsequent install from project2 will place workspace-root = ../.. in the same place. What's worse, project1/packages/project2 will include workspace-root = ../.., which will be very weird when you check out project2 on its own.

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 save-workspace-root = false in both projects' .npmrc files, and keep doing what you're doing. (Or we make it opt-in, but since there are likely many more users who want cd foo ; npm i to work the same as npm i -w foo than there are users with nested workspaces, I'm still inclined to say opt-out is the better approach.)

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 node_modules/.npmrc or some such, so it's not something that gets checked 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.

@jakebailey
Copy link

And then, whenever you install in project1, you have some custom scripts that munge the lockfiles and keep everything in sync, so that updates to bar's deps will get tracked in both project1 and project2?

At the moment, they are using lerna where each package has their own lock and unhoisted node_modules, which is not ideal. I'd be looking for a case where (in your above example), the only lockfiles are in project1 and project2, where project2's lock is the subset of project1 that is referenced in project2, and kept up to date. This is what I've been able to achieve with yarn 3 thanks to plugins; there's a plugin to write out the extra lock files, then another plugin that changes yarn's "where is the real root?" algorithm that can use markers in parent projects to know "oh, I should continue looking upward and not stop just because I found a lock file".

@ruyadorno
Copy link
Contributor Author

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`.
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for confirming :-)

@ruyadorno
Copy link
Contributor Author

wip: npm/config#28

@thernstig
Copy link

@ruyadorno do you feel npm/cli#4338 should be handled in the context of this RFC or would it be a separate RFC?

@ruyadorno
Copy link
Contributor Author

ruyadorno commented Feb 9, 2022

@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.

@ruyadorno
Copy link
Contributor Author

Follow up from our OpenRFC meeting: I'll document the possibility of setting workspaces=false in a child workspace in order to opt-out a specific child workspace folder from following the root.

@thernstig
Copy link

@ruyadorno imagine a user goes cd ./<workspace-folder> and npm install <pkg> (as per this RFC), would it fail if they have an .npmrc in the root of the repo? Or would this RFC handle that? If that is handled here, then it is already covered.

@ruyadorno
Copy link
Contributor Author

imagine a user goes cd ./<workspace-folder> and npm install <pkg> (as per this RFC), would it fail if they have an .npmrc in the root of the repo? Or would this RFC handle that? If that is handled here, then it is already covered.

it should not fail, it should respect the config values defined at the .npmrc file at the root of the repo.

@ruyadorno
Copy link
Contributor Author

thanks @thernstig that's probably a good point to also add to the RFC before approving it 😊 I'll make sure it's there

@ruyadorno ruyadorno removed the Agenda will be discussed at the Open RFC call label Feb 15, 2022
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.

8 participants