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

nohoist installs peerDependencies if root package.json has it #5520

Closed
SEAPUNK opened this issue Mar 14, 2018 · 7 comments
Closed

nohoist installs peerDependencies if root package.json has it #5520

SEAPUNK opened this issue Mar 14, 2018 · 7 comments
Assignees
Labels
fixed-in-modern This issue has been fixed / implemented in Yarn 2+. triaged

Comments

@SEAPUNK
Copy link

SEAPUNK commented Mar 14, 2018

Do you want to request a feature or report a bug?

bug

What is the current behavior?

if a workspace package has a peerDependency that the root package.json has in its devDependencies (+ dependencies?), and the dependency is nohoisted for the workspace package, it gets installed in the workspace package's node_modules

If the current behavior is a bug, please provide the steps to reproduce.

repo: https://github.com/SEAPUNK/yarn-nohoist-peerdep

clone, yarn install, and see that nice/node_modules/left-pad exists (and subsequently, proj/node_modules/left-pad exists as well)

What is the expected behavior?

left-pad should not be installed in nice/node_modules

Please mention your node.js, yarn and operating system version.

nodejs 8.9.4
yarn 1.5.1
macOS high sierra

@ghost ghost assigned rally25rs Mar 14, 2018
@ghost ghost added the triaged label Mar 14, 2018
@connectdotz
Copy link
Contributor

hmm... the current implementation goes like this: if the peerDependency is fulfilled, it is treated like a regular package, thus subject to all hoist/nohoist logic. (see code).

to make sure left-pad created because of the logic mentioned above, I tried the following with your repo:

  1. if I remove the left-pad from your root package.json devDependencies, none of the child projects install left-pad => meaning nohoist will not force installing peerDependencies
  2. if I change nohoist to explicit package such as: "nohoist": ["nice/**", "proj/nice"], the left-pad didn't get created under proj as expected. => meaning nohoist matching logic worked as expected for peerDependencies.

I can see why you expected the peerDependencies to be exempt from "nohoist", but I can also see if we flip the logic, others can equally argue that peerDependencies should be nohoist if the pattern clearly said "proj/**"... yarn has to pick one definition and right now it is the later.

This is not a bug, however, there are a couple of options you can try:

  1. use explicit nohoist instead of wildcard
  2. if you are worried about repo footprint, you can try yarn install --link-duplicates, which will hard link the duplicate package left-pad so even though it appears in multiple node_modules, there will be only 1 copy in the file system.

good luck.

@SEAPUNK
Copy link
Author

SEAPUNK commented Mar 15, 2018

That doesn't sound right. A peerDependency, just by its wording, means that it expects that dependency to be a peer (node_modules/{nice, left-pad}), not a child (node_modules/nice/node_modules/left-pad). Nohoist should respect that rule.

I am not using explicit nohoist because in my use case I want all of a project's dependencies to be nohoisted to the project, as if it was its own isolated package.

I expect the project layout with my config to look like the following:

package.json
yarn.lock
node_modules/
node_modules/left-pad
nice/
nice/package.json
proj/
proj/package.json
proj/node_modules/
proj/node_modules/nice/ --> /nice/

Is this not reasonable?

@connectdotz
Copy link
Contributor

connectdotz commented Mar 15, 2018

A peerDependency, just by its wording, means that it expects that dependency to be a peer

not really... peerDependencies:

Having a peer dependency means that your package needs a dependency that is the same exact dependency as the person installing your package.

peer dependency is just a dependency with the extra constraint. The word "peer" means this dependency should align with its peer's version otherwise report error.

you could try option-2 (but you will need #5442)

@SEAPUNK
Copy link
Author

SEAPUNK commented Mar 16, 2018

From the description you linked me:

This is useful for packages like react that need to have a single copy of react-dom that is also used by the person installing it.

Emphasis on "a single copy".

Because I'm installing nice into proj, but both proj and nice have the same left-pad in their modules, I don't have a single copy of left-pad: Only proj should have it (after I add it to its dependency list, of course).

@connectdotz
Copy link
Contributor

connectdotz commented Mar 16, 2018

yarn, by default, always deploy "a single copy" of a given package@version, whether it is peerDependencies or not. The only exception is when you specify nohoist.

If you are arguing that yarn should never nohoist the package as long as it is in the peerDependencies, try to think about the opposite use case: what if people do need to nohoist the peerDependencies package because the library doesn't work in the workspaces? I hope you can see yarn can't have it both ways.

I think you are better off with option-2, you will find that it gives you what you asked for and more...

@luwes
Copy link
Contributor

luwes commented Feb 9, 2019

I'm having a similar issue, and in my case having the peer dependency be installed in the package's node_modules is breaking my build because the bundler is treating it as 2 separate libraries :(

If I manually remove the peer dependency it works again.

saulshanabrook added a commit to saulshanabrook/jupyterlab that referenced this issue Jun 26, 2019
This fixes jupyterlab#6709, and is a follow up to jupyterlab#6572. It separates building
vega assets from the rest of our package management. That way, we can
avoid any hoisting issues that we were seeing (possibly from yarnpkg/yarn#5520).

To test this, you can try this notebook which should render
both versions without errors:

https://gist.github.com/saulshanabrook/97c28550ab12e684adc3c325038537ce
@merceyz
Copy link
Member

merceyz commented Jan 2, 2021

Closing as fixed in v2

https://yarnpkg.com/getting-started/migration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed-in-modern This issue has been fixed / implemented in Yarn 2+. triaged
Projects
None yet
Development

No branches or pull requests

5 participants