rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

[RRFC] Add nohoist option for workspaces

Open socialwyze-franklin opened this issue 3 years ago • 60 comments

Motivation ("The Why")

Yarn already supports this option. It makes it possible to work with tooling that expects certain modules to be located in inside the same package that consumes them.

Example

In order for a React Native app to build properly within a monorepo, some packages that react-native depends on must reside inside the node_modules folder within the app package. With yarn, I'm able to accomplish it with this configuration:

  "workspaces": {
    "packages": [
      "api",
      "app",
      "shared/*"
    ],
    "nohoist": [
      // my-app is the name of the package inside the app folder
      "my-app/**"
    ]
  }

How

See: https://classic.yarnpkg.com/blog/2018/02/15/nohoist/

References

socialwyze-franklin avatar Nov 11 '20 19:11 socialwyze-franklin

What I'd prefer, rather than adding "nohoist" and having to manually configure it, is finding a way for nothing to hoist, so that each folder's node_modules directory only ever contained the dep graph that its own package.json would have produced.

ljharb avatar Nov 11 '20 20:11 ljharb

What I'd prefer, rather than adding "nohoist" and having to manually configure it, is finding a way for nothing to hoist, so that each folder's node_modules directory only ever contained the dep graph that its own package.json would have produced.

sounds very much likes "pnpm" approach of doing things.

simllll avatar Nov 19 '20 16:11 simllll

Current status:

  • At the very least, the canonical option should be hoist: false rather than nohoist: true to avoid the "notfalse" config antipattern. (Whatever direction we go, we can support nohoist as well.)
  • Some discussion around whether this should be per-workspace, or for the project as a whole.
  • Main hazard: can easily result in workspace projects that install fine in dev, but cannot be correctly used together in production. Eg, packages/foo has a peerDep on [email protected] and packages/bar has a peerDep on [email protected]. Without hoisting, this conflict is not evident in development. As @ljharb points out, this is desired behavior for projects that provide separate connectors for different react versions, but definitely something that should be an opt-in.
  • Discussed other pnpm-style tree generation approaches, and/or having separate "tree strategy" settings for different workspaces. Eg, packages/foo uses global-style, packages/bar uses legacy-peer-deps, etc. (This would be a pretty significant refactor for arborist, but not impossible.) Decided that this is orthogonal to the question of whether we hoist to workspace parent dir or not.

isaacs avatar Dec 02 '20 18:12 isaacs

I'd amend this to say, "hoisting" is the improper term in every case - that what is actually desired is control around "sharing".

I think by default, the root, and individual subpackages, should share all peer deps, and all subpackages themselves, but observably share nothing else (iow, it's fine if they actually share the same copy of lodash, but a workspace shouldn't be able to require lodash unless its own dep graph includes it).

ljharb avatar Dec 02 '20 18:12 ljharb

Hi guys is there any progress on this issue? I really want to use npm for workspaces but I can't because react-native projects require a nohoist option..

zameschua avatar Jul 23 '21 14:07 zameschua

This would actually be very helpful for monorepos with Angular and certain dependencies that look for angular core and common as peer deps.

I'm encountering a scenario where I have an angular theme library that looks for angular/core and angular/common as a peer dep but it's getting hoisted to <project_root>/node_modules. Angular isn't because I have other packages that depend on a newer version of rxjs then angular.

BitForger avatar Oct 18 '21 15:10 BitForger

I would also like to add my support for a nohoist option. I've ran into problems with eslint configurations in a monorepo because it expects to be installed at the local package level.

rmuchall avatar Oct 22 '21 03:10 rmuchall

I agree. A nohoist option would be very useful.

d3lm avatar Oct 22 '21 06:10 d3lm

I'd like to add an additional use case for being able to disable hoisting in workspaces. I recently had to abandon workspaces in favour of Lerna to resolve this for myself.

I am working on a very large project which has quite a few serevrless components to it. We have a bunch of queueing and lambada workers. The project is setup with the following folder structure

/applications/admin-application
/applications/queue-system/
/applications/queue-system/lambda-1
/applications/queue-system/lambda-2
/applications/queue-system/lambda-etc
/applications/queue-system/cdk-infrastructure
/packages/shared-code-packages

each of those folders is a npm package which maintains its own set of dependencies. Some shared some not, some packages that only exist in this project and wont be published to any registry.

Part of publishing an aws lambda is the package needs to contain all of the dependencies in a package (ignoring layers for now). What we want to be able to do is, after an install be able to zip up each of the lambdas with all and only their dependencies so the cdk deployment can pick that up and push it out too aws.

With hoisting everything is in the root which is many millions of unneeded files for each of the lambdas. Not only that, you must handle the folder structure of walking up the tree and pulling everything down to the lambda folder or you end up with a deeply nested folder structure in the lambda.

I was very close to creating some sort of symlink system of constructing the node_modules folder in each of the lambdas at deploy time, then zipping that up. But Lerna more or less just works

will-fagerstrom avatar Nov 01 '21 23:11 will-fagerstrom

Our use case is very similar to what @will-fagerstrom just described, with a twist of Lambda Layers.

We pack all 3rd party deps to the shared layer (as described here) and nohost all internal dependencies to the lambdas themselves. Currently it works perfectly with Yarn 1. Our workspaces config looks similar to the following:

  "workspaces": {
    "packages": [
      "api/*",
      "services/*",
      "client/*"
    ],
    "nohoist": [
      "@octobat/bat-services-lambda-layer/**",
      "@octobat/bat-services-*/@octobat/*"
    ]
  },

I would rather go back to NPM than to Yarn 2+.

khitrenovich avatar Nov 01 '21 23:11 khitrenovich

If and when something like this lands, https://github.com/npm/rfcs/pull/375 is probably going to be the first step. If you have a moment, please go weigh in there about how this will affect your use case, if it will satisfy your needs, etc.

I will update that PR now with my notes from our discussion in the open RFC call a few days ago.

isaacs avatar Nov 05 '21 19:11 isaacs

https://github.com/npm/rfcs/pull/375#issuecomment-962150982

isaacs avatar Nov 05 '21 19:11 isaacs

@isaacs -

Thank you for pointing out the proposal in #375. Unfortunately, I don't think it will help in the case I described above.

As you've mentioned in the proposal,

  1. The wording of "nohoist" is overly specific to a particular implementation. That is, the declaration describes the "what" of package folder tree implementation, instead of the "why" of dependency graph resolution semantics.

This is exactly the way we use nohost option in Yarn. It has nothing to do with the version resolution per se. Instead, we are dealing purely with the deduplication aspect of hoisting. The primary reason for that is the need to package individual parts of the project tree to be used outside - either individually (AWS Lambdas, as described by @will-fagerstrom) or in conjunction with other dependency resolution mechanisms (AWS Lambda + AWS Lambda Layers use case above).

khitrenovich avatar Nov 05 '21 19:11 khitrenovich

@khitrenovich So, iiuc, you're relying on all dependencies being local to the workspace's node_modules folder so that you can npm pack that folder (or zip/tgz/git archive, whatever) and get all your deps?

I think #375 will still satisfy that need, no? Basically, "nohoist" will be the default, unless you have a peer dependency, or a dependency on a sibling workspace. If we implement it by symlinking sibling workspaces into one another when they have a dependency on each other, and they are listed as bundleDependencies, and you're bundling the workspace up with npm pack, then it seems like it'll do the right thing.

isaacs avatar Nov 05 '21 20:11 isaacs

@khitrenovich So, iiuc, you're relying on all dependencies being local to the workspace's node_modules folder so that you can npm pack that folder (or zip/tgz/git archive, whatever) and get all your deps?

Not exactly.

We are developing a serverless system, with backend implemented mostly with AWS Lambdas. Each packaged lambda is supposed to be self-contained (that is, contains both its own code and the dependencies). Yet, it is possible to compose a single lambda from several so-called "layers" and it is possible to share those layers between several lambdas. (more)

Here is a sample of our current workspaces configuration:

  "workspaces": {
    "packages": [
      "api/*",
      "services/*",
      "client/*"
    ],
    "nohoist": [
      "@octobat/bat-services-lambda-layer/**",
      "@octobat/bat-services-*/@octobat/*"
    ]
  },

Our lambdas (@octobat/bat-services-* workspaces above) rely both on internal dependencies (sibling workspaces under the same root project) and on external (3rd party) dependencies. The external dependencies are typically the same for most of the lambdas and rarely change, so we pack them all in a shared lambda layer (@octobat/bat-services-lambda-layer) in order to speed up the deployment. Those dependencies should be hoisted up for all the workspaces except for the lambda layer workspace itself (see the first line on the nohist config). The rest of the dependencies (that is, internal dependencies) appear in the lambda package directly for the ease of development/debugging. We cannot hoist them, otherwise we cannot properly pack them into the workspace artifact (see the second line on the nohist config). Obviously, they are symlink'ed and the packaging tool follows symlinks to obtain the actual content.

Besides the lambda-based backend services the project contains API definitions, client-side workspaces etc. - all those are not affected by nohost configuration and currently benefit from the default hoisting mechanism.

Maybe I'm misinterpreting what #375 is about, but I don't see how it will be possible to configure the same dependency of the same version to be hoisted in one workspace and not hoisted in another workspace.

khitrenovich avatar Nov 05 '21 22:11 khitrenovich

Maybe I'm misinterpreting what #375 is about, but I don't see how it will be possible to configure the same dependency of the same version to be hoisted in one workspace and not hoisted in another workspace.

Well, no, effectively nothing would be hoisted at all, except explicitly declared root dependencies. Any non-peer/non-sibling dependencies of the workspace itself would not be hoisted beyond the workspace boundary. For sibling workspace dependencies, we could create a symlink in the dependent workspace's node_modules folder, so that npm pack would follow the symlink if it's in bundleDependencies.

isaacs avatar Nov 05 '21 23:11 isaacs

Same problem here, i'll just move to yarn again

souzaramon avatar Dec 08 '21 19:12 souzaramon

I am having this problem for Next.js

vimtor avatar Dec 14 '21 21:12 vimtor

Same problem here, i'll just move to yarn again

@souzaramon might want to give PNPM a try, as far as I understand it never hoists anything because it utilizes links so there is no duplication...

bd82 avatar Dec 23 '21 22:12 bd82

I'd like to +1 the nohoist option.

I maintain a monorepo that has an electron app and web app that serves the same frontend and shares backend logic but when I try to distribute the electron app, I get an error because electron is hoisted.

Relevant discussion: https://github.com/electron-userland/electron-builder/issues/3984#issuecomment-504968246

KayBeSee avatar Jan 03 '22 17:01 KayBeSee

Same here -- need this for deploying Google Cloud Functions which always use npm for the cloud build even though I use yarn locally. I currently can't deploy cloud functions with Node 16 because npm rejects the workspaces.nohoist section in my package.json.

garyo avatar Jan 20 '22 22:01 garyo

just curious what is the status on this? Is it a priority thing, a question of whether or not to support such use-cases or a matter of figuring out how to get it right or some other thing?

andrewluetgers avatar May 17 '22 18:05 andrewluetgers

In order for a React Native app to build properly within a monorepo, some packages that react-native depends on must reside inside the node_modules folder within the app package.

Doesn't help much with npm not having a nohoist option, but you can define a custom metro.config.js and specify additional node_modules locations via nodeModulesPaths.

An example how such a config would look like is available in the Expo docs: Working with monorepos. This should work for bare react native repos as well as there is nothing Expo specific

nephix avatar May 17 '22 18:05 nephix

Same question Here too. I am really curious about this issue. quiet surprised npm didn't give an option like this.

SpookyJelly avatar May 31 '22 00:05 SpookyJelly

I've been struggling with the lack of a nohoist option today. A certain dependency that I'm using requires another package and it only works if they are in the same node_modules directory. I realize that shouldn't be the case, and it's certainly the fault of the package I'm using, but having no way to workaround it in npm is unfortunate.

I know the upcoming "isolated mode" is intended to help in cases like this and I'm eagerly awaiting that but it would still be nice to have some form of user control in these situations.

iansu avatar May 31 '22 22:05 iansu

@iansu I do have a very hacky workaround that I've been using that works when you don't need that same dependency as a root dep, basically I declare an aliased dep in the root using the same name of the dep I need to skip hoisting in my workspaces thus forcing a conflict and the duplication of that dep inside my workspace node_modules folder.

ruyadorno avatar Jun 01 '22 00:06 ruyadorno

@ruyadorno I've been achieving this by requesting different patch versions of the same package but your method seems a bit better. Thanks.

iansu avatar Jun 01 '22 16:06 iansu

And off to some third party package manager again. I really wanted to keep it simple to get more possible contributors, but not having a solution to such basic dependency problems is a show stopper.

Maybe it would be worth implementing a temp solution like nohoist until a perfect proposal how to name and implement the option arises ;D

FalkF avatar Jun 05 '22 19:06 FalkF

Shameless plug in the interim before this gets adopted (Please please please consider this in NPM)

I made a package for using the workaround @ruyadorno mentioned that has no dependencies

https://github.com/zgriesinger/noist

This allows you to use the workaround with this directly or (and what I would recommend) forking it and publishing your own for your own use that way you don't add unnecessary attack vectors.

zackerydev avatar Jun 21 '22 20:06 zackerydev

My team encountered a problem related to workspace package hoisting, so adding it as a data point to this RFC.

The @npmcli/run-script package recently introduced a bug that impacts Windows build environments, and npm 8.13 updated this package to the problematic version. We should be able to avoid the bug by making sure our team is using npm < 8.13, but workspace hoisting poses a new problem. If npm is a sub-dependency of our project, it will be hoisted to the root node_modules and npm scripts will use that instead of the globally installed version of npm.

For example:

➜ npm ls npm
our-project@version
└─┬ @semantic-release/[email protected]
  └─┬ @semantic-release/[email protected]
    └── [email protected]

Typically the sub-dependency would be under @semantic-release/npm/node_modules/npm but due to using workspaces it gets hoisted to our-project/node_modules/npm. Any npm run script in our-project/package.json will use this local version of npm instead of the global installation.

Our current workaround is to add a direct dependency on [email protected] so the hoisted version doesn't include the problematic version of run-script, but it would be ideal if we could prevent npm from being hoisted.

schmidtk avatar Jul 07 '22 17:07 schmidtk