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

difference between npm and yarn behavior with nested dependencies #3951

Closed
sthuck opened this issue Jul 18, 2017 · 21 comments

Comments

@sthuck
Copy link

commented Jul 18, 2017

Hi all,
I found a slight difference in behavior between npm (v4) and yarn 0.24.5 to yarn 0.27.5.

my package.json dependencies has something like this:

    "@types/angular": "~1.5.0",
    "@types/angular-mocks": "^1.5.0"

now @types/angular-mocks package.json looks like this:

 "dependencies": {
        "@types/angular": "*"
    },

In npm v4 and yarn 0.24.5, the @types/angular: "~1.5.0" I have in my package.json, satisfies this dependency, so I have only 1 @types/angular installed, in node_modules/@types/angular, and it's version is 1.5.23. Which is great.

But in yarn 0.27.5, I think the "@types/angular": "*" is being interpreted as a dependency on "latest".

Whatever the reason is, this causes me to now have 2 @types/angular packages.
Version 1.5.23 in node_modules/@types/angular, and version 1.6.26 (latest) in node_modules/@types/angular-mocks/node_modules/@types/angular. This obviously causes issues for typescript.

Im not sure if this is the intended behavior... If it is the intended behavior, and if someone by chance has an idea which commit is responsible for this change, I would love to know. Like I wrote, in 0.24.5 this issue didn't happen. I tried finding it myself but without success.

node: v6.9.5, npm: 4.1.2, yarn: 0.27.5

@BYK

This comment has been minimized.

Copy link
Member

commented Jul 18, 2017

Hmm this is interesting. I think we treat * as latest but it actually means "any would do". @arcanis @kaylieEB @voxsim can you back me up on this?

If so I think we have a bug in our hands.

@arcanis

This comment has been minimized.

Copy link
Member

commented Jul 19, 2017

I wouldn't class this as a bug. We have no strict guarantee on how the hoisting process will put things in the end - for what it's worth, we could implement a --no-hoisting flag and your use case would break similarly.

I feel like the fix should be on @types/angular, not us - they just have to use a peerDependencies entry instead of dependencies.

@BYK

This comment has been minimized.

Copy link
Member

commented Jul 19, 2017

@arcanis - semver clearly defines * as any version: https://github.com/npm/node-semver#x-ranges-12x-1x-12-

No matter how and what we hoist, we should accept anything for *, right?

@arcanis

This comment has been minimized.

Copy link
Member

commented Jul 19, 2017

Yes and no. This is a bit tricky.

  • Yes, using * means that the hoister is indeed allowed to consider that the dependency can be safely merged with any other version if it wants to

  • However, it doesn't mean that the hoister is required to do so. Semantically speaking, a dependencies entry literally means "this package MUST be able to require a package of version $VERSION". There's absolutely no mention about this version being compatible with any other. If you need this behavior, then you have to use peerDependencies, which provide the extra guarantee that the selected version MUST be the same as the one provided by the parent package (which is exactly what is asked for in the first post).

So, to sum up: the hoister is about trimming the fat and avoiding filling your disk with garbage - not ensure your dependencies are correctly setup (that's the responsibility of package authors). Ideally, your code should work with no hoisting, or with super-weird hoisting that would create ten levels of indirection. If you don't, it will eventually break, whether it's in Yarn or in another package manager.

Of course, we could potentially fix this (most likely not without spending some serious time, and breaking other behaviors). But I fear that by doing so, we would actually enable this kind of "We can do whatever we want, the package managers will have to make it work anyway" behavior, which I think is detrimental to everyone in the long term (Yarn, but also other package managers who would have to have the exact same behavior, and package consumers that would have to use sub-par package managers that can't apply aggressive optimizations because some packages do not play by the rules - we've already been hit by that in a few occasions). 😉


Now, from an implementation viewpoint, here is what causes the behavior described in the OP. First, you have to understand that what we called the "hoister" is actually splitted in two different parts:

  • The optimizer currently runs during resolution, and tries to avoid resolving ranges that have already been satisfied by previous resolution requests, but the requests are mostly independants (so for example, in the OP situation, when we resolve angular, we have no idea that a sub-dependency has also a dependency on angular, and conversely). The result of the optimizer is what is stored inside the lockfile.

  • The hoister runs once the resolution (and optimization) has completed, and I think simply tries to merge together packages that share a exact same version, without any consideration for their original range.

So during the first step, both angular versions are resolved to their respective versions, independently from each other, then the hoister tries to merge them, see that their versions don't match, and just doesn't merge them. And as described in the first part of this comment, this is to be expected. If you want them to be merged, you would instead use a peerDependencies entry, which would not be resolved and wouldn't suffer the issue described here.

@BYK

This comment has been minimized.

Copy link
Member

commented Jul 19, 2017

@sthuck - Okay, we've had an offline chat with @arcanis about this and now I understand why he is right and this is the expected behavior. Here's the thing: hoisting is not a requirement, it is an optional optimization. So if a dependency lists a sub-dependency as *, it should still be okay getting a different version of it being installed, just for itself. If the expectation is to have a single copy of that sub-dependency and not care about the version, then it should be listed as a peerDependency.

Does this make sense?

@sthuck

This comment has been minimized.

Copy link
Author

commented Jul 19, 2017

@BYK yep. Will open a PR with DefinitelyTyped to fix their dependencies.

@TheTedAdams

This comment has been minimized.

Copy link

commented Jul 27, 2017

@sthuck have you made this PR? I'm having massive issues right now because @types/react just switched from Component<P,S> to Component<P={},S={}> and changed lots of depending type files to no longer supply an S. The problem is all of the types depending on @types/react: "*" are getting an older version of @types/react installed in a nested node_modules folder and my build is massively screwed...

@sthuck

This comment has been minimized.

Copy link
Author

commented Jul 30, 2017

@TheTedAdams
Seems like the issue is with the script used to publish type definitions from DefinitelyTyped. If this is still relevant for you, would love it if you +1 or explain your use case to demonstrate this issue isn't specific to one package.
microsoft/types-publisher#366

@cwmoo740

This comment has been minimized.

Copy link

commented Jul 31, 2017

I have a further question about

treat * as latest

installing

yarn add @types/react resolves to 15.6.0, but yarn add @types/react@* resolves to 15.0.33.

This creates duplicate @types/react dependencies everywhere unless I peg the version of @types/react to an outdated one, from all of the @types packages using dependencies instead of peerDependencies.

Edit: on further testing it looks like that module resolution changed after deleting yarn.lock and re-generating a brand new one. It was slightly confusing how picking these nested dependencies changed after deleting the lock file.

@TheTedAdams

This comment has been minimized.

Copy link

commented Jul 31, 2017

@cwmoo740 that is consistent with my experience. @types/react was resolving to the new 15.6 but @types/radium and any others that had a dependency on @types/react: "*" was getting a nested node_modules folder with an older version of @types/react. Deleting my yarn.lock fixed the issue...

@arcanis

This comment has been minimized.

Copy link
Member

commented Aug 1, 2017

@cwmoo740 That seems to be a different issue, we should probably fork this discussion in a new ticket. That being said, I've just tested it and got 16.0.0 in every case I've tested (no range, * as range without preexisting version, * as range with a preexisting version).

@glentakahashi

This comment has been minimized.

Copy link

commented Aug 22, 2017

@arcanis your suggestion to use peerDependencies wont' work with npm@3, since they no longer install peerDependencies by default.

See microsoft/types-publisher#371 (comment)

@mjhoffm2

This comment has been minimized.

Copy link

commented Aug 29, 2017

I just ran into this issue or something related trying to switch a project for npm to yarn. I needed to delete my node_modules/@types folder and run "npm install" in order to fix the problem.

@danielweck

This comment has been minimized.

Copy link

commented Aug 29, 2017

Yep, same problem here: in our project we had to revert to Yarn version 0.24.5 (from 0.27.5 and even 0.28.4). Actually, I personally decided to use NPM (latest v5) instead.

@glentakahashi

This comment has been minimized.

Copy link

commented Sep 6, 2017

It's not automatic, but you can now use the resolutions block introduced in 1.0.0 to fix this issue manually without messing with yarn.lock!

https://github.com/glentakahashi/types-test

@donaldpipowitch

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2017

Hi everyone :)

I slightly lost the overview on this issue. Given the following package.json:

{
  "name": "yarn-star",
  "version": "1.0.0",
  "private": true,
  "dependencies": {
    "@types/react": "^15.0.0",
    "@types/react-router-dom": "^4.0.7"
  }
}

$ yarn in v0.28.4:

├── @types/react@15.6.2
└─┬ @types/react-router-dom@4.0.7
  ├── @types/react@16.0.5
  └─┬ @types/react-router@4.0.15
    └── @types/react@16.0.5

$ npm install in v5.4.1:

├── @types/react@15.6.2
└─┬ @types/react-router-dom@4.0.7
  ├── @types/react@15.6.2  deduped
  └─┬ @types/react-router@4.0.15
    └── @types/react@15.6.2  deduped

$ pnpm install in v1.13.2:

├── @types/react@15.6.2
└─┬ @types/react-router-dom@4.0.7
  ├── @types/react@16.0.5
  └─┬ @types/react-router@4.0.15
    └── @types/react@16.0.5

Is there consensus if yarns behaviour is a bug or not?
Is the resolutions feature in yarn@1.0.0 a temporary workaround for this problem or is it the recommended way to solve this?

@BYK

This comment has been minimized.

Copy link
Member

commented Sep 7, 2017

@donaldpipowitch doesn't look like a bug to me since @types/react-router lists @types/react@* as a dependency, not a peerDependency: https://unpkg.com/@types/react-router@4.0.15/package.json

@donaldpipowitch

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2017

@BYK Thanks for the feedback.

Sorry for pinging you directly @RyanCavanaugh, but maybe you can comment on this? Your comment here microsoft/types-publisher#371 (comment) sounds like this wouldn't be a feasible solution for @types. Would love to find some consensus here between these two big technologies :)

I personally would favour npm's behaviour. Not just for @types, but in general. This can also make bundling easier (less bloat).

@RyanCavanaugh

This comment has been minimized.

Copy link

commented Sep 7, 2017

We want a way to say "There should be some version of this dependency installed automatically, but multiple packages can form a consensus to avoid multiple versions from being installed". NPM's dependencies does the first but not the second; peerDependencies does the second but not the first. It doesn't look like Yarn has a fix, either. In an ideal world @types would just have its own #$^!% package manager to manage this, but we've already deployed a lot of software+users on the "Run npm install @types/react" workflow so we can't really change it. And trying to make all this work on two package managers with nonidentical behavior is just another headache on top of that.

@donaldpipowitch

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2017

Thank you @RyanCavanaugh for taking the time. (I'm actually glad that @types hasn't its own package manager. Still remember tsd^^)

So I guess for TypeScript users or users who want as few modules as possible (e.g. for bundling) the resolutions feature is currently the way to go...?

@cwmoo740

This comment has been minimized.

Copy link

commented Sep 7, 2017

@donaldpipowitch I've also had success manually editing the yarn.lock file to force @types/react@* to resolve to the version I want

@BYK BYK added needs-discussion and removed cat-discussion labels Oct 27, 2017

balthild added a commit to ResistanceCN/website-react that referenced this issue Nov 23, 2017

longlho added a commit to formatjs/react-intl that referenced this issue Aug 15, 2019

fix: move react & @types/react to devDep, fixes #1389
This is strictly to fix `yarn` problem of not deduping packages
See yarnpkg/yarn#3951 (comment)
for more details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.