Pnpm: Deduping algorithm not working properly

Created on 26 May 2018  ·  31Comments  ·  Source: pnpm/pnpm

pnpm version: 2.0.0 (and earlier)

Code to reproduce the issue: https://github.com/nickpape-msft/react-pnpm-repro

Expected behavior:

The package react-dom-16-bundle gets @types/react@16 & @types/react-dom@16.

The package @types/react-dom@16 gets @types/react@16, since the parent dependency (react-dom-16-bundle used 16, and react-dom has a * dependency).

Actual behavior:

The package react-dom-16-bundle gets @types/react@16 & @types/react-dom@16.

The package @types/react-dom@16 gets @types/react@15.

It seems that the deduping algorithm from #952 is not working as intended.

Additional information:

  • node -v prints: 8.9.4
  • Windows, OS X, or Linux?: Windows
feature
Source
picture nickpape

All 31 comments

The package @types/react-dom@16 gets @types/react@16, since the parent dependency (react-dom-16-bundle used 16, and react-dom has a * dependency).

in the repo that you linked, @types/[email protected] is specified in the root, so this is the expected behavior

https://github.com/nickpape-msft/react-pnpm-repro/blob/72953ebb1cd92592a4e04e74dfbeaba81abce724/package.json#L3

zkochan picture zkochan on 30 May 2018

Doesn't that seem strange? I thought in the initial proposal in #952, you implemented the deduping such that we preferred the most recent parent (i.e. we prefer what react-dom-16-bundle is using), and then went backwards up the dependency graph until we got to the root? Was there an issue with that approach?

nickpape picture nickpape on 30 May 2018

no, it was the only way to do it w/o performance regression https://github.com/pnpm/pnpm/issues/952#issuecomment-353732533

zkochan picture zkochan on 30 May 2018

We've come up with a simpler repro at https://github.com/nickpape-msft/react-pnpm-repro
In this repro, we depend on two tarballs, but we only need to care about the 15 version to see the issue. The 15 version of the tarball (package.json is in the repo as), we depend on @types/react version 15.6.6 and @types/react-dom version 15.5.6. @types/react-dom has a dependency on @types/react with semver selector "^15", which causes @types/react version 15.6.16 to get installed alongside 15.6.6

It looks like the selection heuristic only cares about the root package.json, and not about dependent packages' package.jsons when solving version heuristics, which in this case is causing the additional version to get installed (and is breaking TS's typechecking)

iclanton picture iclanton on 6 Jun 2018

ok, I think we can make it work but it will have to be on a config. This deduping will be either nondeterministic or will cause a slowdown

zkochan picture zkochan on 6 Jun 2018

During resolution stage, pnpm will have to wait until all direct dependencies of a package are resolved. Only after all dependencies on one level are resolved, pnpm will be able to proceed to the next level, passing the "preferred versions".

This will have to be changed in https://github.com/pnpm/pnpm/blob/master/packages/supi/src/resolveDependencies.ts

Preferring versions is implemented currently in @pnpm/npm-resolver but only for the root deps and maybe it'll have to be optimized https://github.com/pnpm/npm-resolver/blob/3f57f9e2d0782c8b9e9dc07ce3607b63b974927f/src/pickPackageFromMeta.ts#L39-L73

zkochan picture zkochan on 7 Jun 2018

I just realized that it is possible to do with any additional changes in pnpm, with what in #952 was implemented. Just add "@types/react": "15.6.6 || 16.3.14" as a direct dependency. See https://github.com/nickpape-msft/react-pnpm-repro/pull/2

zkochan picture zkochan on 8 Jun 2018

That seems to work around the extra versions issue, but it doesn't solve the issue where packages get a version that ignores their context.

Can we pursue the option to prefer versions beyond just the root?

iclanton picture iclanton on 15 Jun 2018

@zkochan I think I may have encountered another strange issue that is related.

It is described here: https://github.com/Microsoft/web-build-tools/issues/750

This post describes the shrinkwrap issue: https://github.com/Microsoft/web-build-tools/issues/750#issuecomment-406747137

And here is a link to how to repro the issue: https://github.com/Microsoft/web-build-tools/issues/750#issuecomment-406724770

We can probably work around this using pnpmfile.js, but it is very odd.

nickpape picture nickpape on 21 Jul 2018

We've started seeing this more. This time, while upgrading the TypeScript compiler for just one project in a Rush repo. A package called tslint-microsoft-contrib has a peerDependency on typescript that isn't getting fulfilled when two version of typescript are requested.

@pgonzal

iclanton picture iclanton on 1 Sep 2018

We do see a warning for this issue during install, so https://github.com/pnpm/pnpm/issues/1126 would be very helpful in this case.

Here's the warning

 WARN  [email protected] requires a peer of typescript@^2.1.0 || ^3.0.0 but none was installed.
iclanton picture iclanton on 1 Sep 2018

[email protected]

Another, maybe different, case: I have multiple versions of prop-types installed while I believe I should only be getting one.

25862 $ pnpm ls prop-types
[email protected] /path
├─┬ @yaska-eu/[email protected]
│ └── [email protected]
├─┬ [email protected]
│ └── [email protected]
├── [email protected]
├─┬ [email protected]
│ └── [email protected]
├─┬ [email protected]
│ └── [email protected]
├─┬ [email protected]
│ └── [email protected]
├─┬ [email protected]
│ ├── [email protected]
│ └─┬ [email protected]
│   ├── [email protected]
│   └─┬ [email protected]
│     └── [email protected]
├─┬ [email protected]
│ └─┬ [email protected]
│   └── [email protected]
[...]

$ grep \"prop-types\":  node_modules/*/package.json
node_modules/eslint-plugin-react/package.json:    "prop-types": "^15.6.2",
node_modules/react-apollo/package.json:    "prop-types": "^15.6.0",
node_modules/react-autosize-textarea/package.json:    "prop-types": "^15.5.6"
node_modules/react-big-calendar/package.json:    "prop-types": "^15.6.2",
node_modules/react-daterange-picker/package.json:    "prop-types": "^15.6.2",
node_modules/react-dom/package.json:    "prop-types": "^15.6.2",
node_modules/react-dropzone/package.json:    "prop-types": "^15.7.2"
node_modules/react-google-login/package.json:    "prop-types": "^15.6.0"
node_modules/react-headroom/package.json:    "prop-types": "^15.5.8",
node_modules/react-helmet/package.json:    "prop-types": "^15.5.4"
node_modules/react-hot-loader/package.json:    "prop-types": "^15.6.1",
node_modules/react-images/package.json:    "prop-types": "^15.6.0",
node_modules/react-imported-component/package.json:    "prop-types": "15.6.2",
node_modules/react-inspector/package.json:    "prop-types": "^15.6.1"
node_modules/react-lazyload/package.json:    "prop-types": "^15.5.6",
node_modules/react-measure/package.json:    "prop-types": "^15.6.2",
node_modules/react-numeric-input/package.json:    "prop-types": "^15.5.8",
node_modules/react-redux/package.json:    "prop-types": "^15.7.2",
node_modules/react-router-dom/package.json:    "prop-types": "^15.6.2",
node_modules/react-router/package.json:    "prop-types": "^15.6.2",
node_modules/react-select/package.json:    "prop-types": "^15.5.8",
node_modules/react-share/package.json:    "prop-types": "^15.5.8"
node_modules/react-sortable-hoc/package.json:    "prop-types": "^15.5.7"
node_modules/react-stripe-checkout/package.json:    "prop-types": "^15.5.8",
node_modules/react-test-renderer/package.json:    "prop-types": "^15.6.2",
node_modules/react-text-mask/package.json:    "prop-types": "^15.5.6"
node_modules/react-tooltip/package.json:    "prop-types": "^15.6.0"
node_modules/react-transition-group/package.json:    "prop-types": "^15.5.6",
node_modules/react/package.json:    "prop-types": "^15.6.2",
node_modules/redbox-react/package.json:    "prop-types": "^15.5.4",
node_modules/redux-form/package.json:    "prop-types": "^15.6.1",
node_modules/searchtabular/package.json:    "prop-types": "^15.6.0",
node_modules/slate-react/package.json:    "prop-types": "^15.5.8",
node_modules/sortabular/package.json:    "prop-types": "^15.6.0",
node_modules/styled-components/package.json:    "prop-types": "^15.5.4",
node_modules/treetabular/package.json:    "prop-types": "^15.6.0",

As you can see, all deps have the version as ^ so I would expect all to get the latest proptypes v15.7.2. Am I wrong?

wmertens picture wmertens on 12 Mar 2019

In this related Yarn discussion, we are proposing that every @types package should specify its dependencies as peerDependencies:

https://github.com/yarnpkg/yarn/issues/6695#issuecomment-470346353

I wonder if its the same root problem.

Are you able to work around it using pnpmfile.js? (In particular it would be cool if someone could cook up a pnpmfile.js that generically converts everything to peers and see if the idea works.)

That said, I also tend to agree with the above assertions that pnpm could choose the versions correctly during installation. An algorithm seems to exist that would do the right thing, and nobody in our group cares if it makes installation somewhat slower, since installs are now a relatively small contributor to our build times.

octogonz picture octogonz on 12 Mar 2019

@zkochan what's your current thinking regarding this issue? Seems like people are still encountering it regularly.

octogonz picture octogonz on 12 Mar 2019

@octogonz Isn't that just

pnpmfile.js

const readPackage = (pkg, context) => {
    if (/@types/.test(pkg.name)) {
        pkg.peerDependencies = {...pkg.peerDependencies, ...pkg.dependencies}
        pkg.dependencies = {}
        context.log(
            `replacing ${pkg.name}'s deps with peerDeps`
        )
    }
    return pkg
}

module.exports = {hooks: {readPackage}}

Maybe it can keep all the dependencies in module scope and then when the main package is visited (how to know this 🤔) put all the peerDeps into its deps.

Still, that's just the same as doing a flat install.

wmertens picture wmertens on 12 Mar 2019

Still, that's just the same as doing a flat install.

Not exactly, because intermediary libraries are able to satisfy peer dependencies.

For example, you can have:

app/package.json

{
  "name": "app",
  "version": "1.2.3",
  "dependencies": {
    "platform": "1.2.3"
  }
}

and then:

platform/package.json

{
  "name": "platform",
  "version": "1.2.3",
  "dependencies": {
    "@types/react": "1.2.3",
    "@types/prop-types": "1.2.3"
  }
}

and then:

react/package.json

{
  "name": "@types/react",
  "version": "1.2.3",
  "peerDependencies": {
    "@types/prop-types": "*"
  }
}

In this (somewhat contrived) example:

  • platform uses @types/react which needs @types/prop-types
  • platform locks its version of @types/react, and in doing so also takes responsibility for choosing the right version for @types/prop-types
  • app uses platform but does not need to get involved with @types at all, because the app doesn't use React directly

If app also relies on another library that uses a different version of React (side-by-side), then that library can specify its own choice for @types/prop-types version, without interfering with platform's installation.

You can sort of accomplish the same thing without peer dependencies, except that the NPM specification is fairly vague about how a package manager should solve the *. It's a gambling crapshoot where you usually get lucky with NPM, sometimes unlucky with PNPM, and always lose with Yarn. Whereas using peerDependencies forces the package manager to look at the consuming package to find the answer, thus eliminating ambiguity (and making Yarn usable).

octogonz picture octogonz on 12 Mar 2019

@zkochan what's your current thinking regarding this issue? Seems like people are still encountering it regularly.

I agree with Yarn's maintainer completely in that thread. I don't think there's anything wrong with how pnpm or Yarn currently resolve the dependencies. If peerDependencies solve this issue, then peerDependencies should be used. If it is possible to come up with some other declarative way of solving this issue, I'd support that as well.

If someone thinks pnpm should support an alternative resolution strategy that sacrifices speed for less duplication, I don't have objections. We can have it but only as opt-in.

zkochan picture zkochan on 13 Mar 2019

@zkochan I think that pnpm currently does resolution wrong. The Yarn maintainer wrote:

It's just that, at a basic level, Yarn will only ever resolve one range to one version, regardless of the position of the package in the dependency tree (this is different from npm which might resolve the same range to different versions depending on the package location on the dependency tree, for the better or worse). This mechanism is a cornerstone of our architecture, and unlocks various core optimizations.

But as I show in my example above, there are no conflicting ranges for prop-types, they can all resolve to the latest 15.7.2 which would be the single version that Yarn talks about, but pnpm instead resolves to both 15.6.2 and 15.7.2.

I think that behavior is incorrect.

wmertens picture wmertens on 13 Mar 2019

ok, if this is so important, I'll work on a flag

zkochan picture zkochan on 15 Mar 2019
❤1

Fewer dependencies mean fewer tarballs have to be downloaded, so even though deduplication slows down the resolutions step, the overall installation time might stay the same (or become faster). But we won't know for sure until we measure it.

(that might explain why pnpm is currently slower than yarn with lockfile)

zkochan picture zkochan on 23 Mar 2019

Have you considered a way to parallelize the installation algorithm to divide across multiple NodeJs processes? At my work the network bandwidth is ridiculously fast, but the pnpm install downloads still run somewhat sluggishly, and sometimes it seems like the CPU might be pegged at 1/8 of 100%, i.e. maxing out one CPU core. I didn't do any rigorous measurements though -- this was just an informal impression.

octogonz picture octogonz on 23 Mar 2019

PR is ready https://github.com/pnpm/pnpm/pull/1739

@octogonz no, I did not try that

zkochan picture zkochan on 24 Mar 2019

Also, I created a poll to see what is more important for pnpm users (speed or deduplication)

https://twitter.com/pnpmjs/status/1109800824744656898

zkochan picture zkochan on 24 Mar 2019

The poll is a little biased in its wording heheh. It sounds like it's asking whether people prefer speed versus saving disk space ("deduplication"). Whereas the tradeoff here is really speed versus compatibility -- "Do you want to be able to use two different versions of an @types package like you can with NPM? Or are you willing to maintain complicated fixups in your pnpmfile.js so that installation can be a little faster?"

(My own vote would depend -- If I was on a small team, I'd opt to wait to enable the flag until we hit an actual compatibility problem. Whereas on a big team, I'd prefer reliability and compatibility, so it's one less maintenance headache we have to deal with.)

octogonz picture octogonz on 24 Mar 2019

Well in my case, there's no 5% speedup of anything that would make me consider having my app break in subtle ways, often only noticeable in production…

wmertens picture wmertens on 24 Mar 2019

OK, if this will solve issues, let's make separate voting in a few weeks.

zkochan picture zkochan on 24 Mar 2019

@zkochan thanks a ton for implementing this! I'll give it a try as soon as it's published.

octogonz picture octogonz on 25 Mar 2019

:ship: 3.1.0-0

config name: resolution-strategy. Possible values: fast (the default), fewer-dependencies

zkochan picture zkochan on 25 Mar 2019
❤1

Looks good!

I was able to reproduce @nickpape-msft 's issue using his example repro above. (The behavior has changed slightly since @types/react-dom now specifies its dependency as "@types/react": "^15" instead of "@types/react": "*", so the side-by-side versions are both from React 15.x. But the same effective problem is still there.)

Then I installed using --resolution-strategy fewer-dependencies, and confirmed that with this option, only one version of @types/react gets installed. Hurray!

We will need to try this out in some production monorepos. But if it works as expected, I'm thinking we will change Rush to enable --resolution-strategy fewer-dependencies by default when PNPM is the package manager. I expect that it will eliminate a lot of confusion for people.

octogonz picture octogonz on 25 Mar 2019
👍1

Deduplication actually won the poll by about 5 votes. So I am OK making this the default behavior but only if I can make it more deterministic. The current implementation may result in different lockfiles, the result is a bit random. But making it more deterministic will make it a bit slower again. We'll see. I am not releasing 3.1.0 for now

zkochan picture zkochan on 25 Mar 2019

Dang, I can't use 3.1.0-0 because shrinkwrap.yaml got renamed. I guess we need to update Rush to support that first heheh.

octogonz picture octogonz on 29 Mar 2019
Was this page helpful?
0 / 5 - 0 ratings

Related issues

waylandc picture waylandc  ·  31Comments
zkochan picture zkochan  ·  34Comments
ghost picture ghost  ·  54Comments
zkochan picture zkochan  ·  33Comments
seoker picture seoker  ·  31Comments