Fix #10727 - Consider devDependencies when deciding whether to hoist a package

[schmod]

Fixes several issues that were causing devDependencies to block certain transitive dependencies from being installed, or leave incompatible versions in place.

deps#earliestInstallable now considers devDependencies that are being installed when determining where to place a package, and will hoist or resolve conflicts where necessary, so that the devDependencies and dependencies can coexist peacefully.

Previously, if we requested:

pkg@1.0.0 
├── depA@1.0.0 (dev dependency)
├─┬ libA@1.0.0
│ ├── depA@2.0.0

We would actually install:

pkg@1.0.0 
├── depA@1.0.0 (dev dependency)
├── libA@1.0.0

depA@2.0.0 would first be placed in the root, as earliestInstallable did not consider pkg's devDependencies when looking for potential conflicts. Laster, we would process the devDependencies, and depA@1.0.0 would overwrite depA@2.0.0's position in the tree. [Overwriting nodes like this seems somewhat bug-prone, although I'm not sure if there are any good options for detecting when it happens so we can print a warning...]

---

This fix uncovered a second condition (observed in the shrinkwrap-shared-dev-dependency test-case), where devDependencies are not properly considered when computing the minimal number of packages that need to be installed. If a package is included as a devDependency and is also transitive dependency of a regular dependency, that package would be installed twice, leaving us with a tree that looks something like this:

pkg@1.0.0 
├── depA@2.0.0 (dev dependency)
├─┬ libA@1.0.0
│ ├── depA@2.0.0 (dependency of libA. redundant)

This patch alters npm install's behavior so that the transitive dependencies of our dev + prod dependencies are computed together. Previously, we calculated the two trees separately, and production dependencies had no knowledge of any devDependencies that were being installed in the same session.

---

The symptoms described in #10727 could be worked around by running npm install twice. I honestly can't figure out why this workaround actually worked, and consider the behavior to be somewhat alarming, as it suggests there may be inconsistent behavior elsewhere.

---

Fixes #10727 #11062 #12654 #10277 #11766 #11043

[schmod]

@iarna: While working on this issue, this caveat bothered me somewhat, as it makes earliestInstallable highly dependent on the behavior of findRequirement (which is particularly troublesome, given that earliestInstallable is exported and used elsewhere)

It may be worth considering somehow merging the two methods, or adding some sort of safeguard mechanism so that we can verify that findRequirement indeed did its job.

You seem to know this code better than anyone else -- I'd be interested in hearing your thoughts about how we might be able to make this more robust without hurting performance.

I don't think we need to change this for the sake of solving the issues addressed in my PR, but it seems like this would be a good spot to insert a sanity-check for future cleanup/refactoring work...

[iarna]

Ok, so let's back up and talk about what earliestInstallable is supposed to do:

You give it a tree, the node in the tree with this requirement and the package.json data for the package and it tells you the location closest to the root of the tree that you can install the package in without producing conflicts. If it can't find any such location then it just returns the requiring node.

I think the problem here is that this comment is a bit off base. We do make one assumption:

• You already know that you need to install this module, you just don't know where yet.

Ordinarily this code is pointless given the existence of _phantomChildren– but _phantomChildren isn't populated in two circumstances:

1. If you requested the installation of just one (or a few) named modules, eg npm install thingy.
2. If you asked for --only=dev on a folder without prod deps installed.
3. Or relatedly, if you did an ordinary install and we still have a separate dev deps stage.

So having written this out it seems clear to me that it should be doing a version match like findRequirement does to see if the version we have in our hands is compatible with the spec in dependencies (and devDependencies) and if it isn't returning null.

I think that would look something like:

var deps = tree.package.dependencies || {}
if (deps[pkg.name] && !doesPkgVersionMatch(deps[pkg.name], pkg)) return null
var devDeps = tree.package.devDependencies || {}
if (devDeps[pkg.name] && !doesPkgVersionMatch(devDeps[pkg.name], pkg)) return null

With a doesPkgVersionMatch being split from doesChildVersionMatch like:

function doesChildVersionMatch (child, requested, requestor) {
  // we always consider deps provided by a shrinkwrap as "correct" or else
  // we'll subvert them if they're intentionally "invalid"
  if (child.parent === requestor && child.fromShrinkwrap) return true
  return doesPkgVersionMatch(requested, child.package)
}

function doesPkgVersionMatch (requested, pkg) {
  // ranges of * ALWAYS count as a match, because when downloading we allow
  // prereleases to match * if there are ONLY prereleases
  if (requested.spec === '*') return true

  var pkgReq = pkg._requested
  if (pkgReq) {
    if (pkgReq.rawSpec === requested.rawSpec) return true
    if (pkgReq.type === requested.type && pkgReq.spec === requested.spec) return true
  }
  if (!registryTypes[requested.type]) return requested.rawSpec === pkg._from
  return semver.satisfies(child.package.version, requested.spec)
}

[iarna]

With an inverted scenario from what I discussed in https://github.com/npm/npm/pull/12811/files#r64309610: ```

pkg@1.0.0 
├── DEV: depA@^1.2.0
├─┬ libA@1.0.0
│ ├── depA@~1.2.0

A transitive prod dep can change what dev dep version gets installed. This would mean that:

npm install --only=dev && npm install --only=prod

Would give you:

pkg@1.0.0 
├── depA@1.4.5
├─┬ libA@1.0.0
│ ├── depA@1.2.7

But:

npm install --only=prod && npm install --only=dev

would give you:

pkg@1.0.0 
├── depA@1.2.7
├── libA@1.0.0

But I'm a lot more comfortable with that lack of symmetry than the other kind.

[schmod]

So having written this out it seems clear to me that it should be doing a version match like findRequirement does to see if the version we have in our hands is compatible with the spec in dependencies (and devDependencies) and if it isn't returning null.

Yes, that behavior seems like it helped the original bug to slip through:

My prod deps would throw lodash@0.9.2 into my root node_modules folder. During loadDevDeps, a copy of lodash@2.0.0 would silently take its place, because earliestInstallable wasn't able to realize that the versions were incompatible.

As you mentioned, earliestInstallable makes a strong assumption that we really need to install this module somewhere, and by falling back to the root node_modules folder, guarantees that we do so at all costs. If earliestInstallable crawls all the way up to the root, and finds a conflicting version of the same package already there, it assumes that we really need the package, and overwrites the existing one.

Unfortunately, earliestInstallable has no way of knowing that the versions conflicted, so it had no way of raising a warning. I don't think we would have a good way of recovering from this situation, but silent failure seems bad if we can avoid it.

So having written this out it seems clear to me that it should be doing a version match like findRequirement does to see if the version we have in our hands is compatible with the spec in dependencies (and devDependencies) and if it isn't returning null.

I have vague concerns that this still won't be able to handle conflicts at the root level. Returning null presents an ambiguity between "This belongs at the root level" and "I don't know where to put this"

[iarna]

We should get warnings about conflicts at the root level due to invalidness checking, although really I'd like conflicts to be warned separately from invalid installed modules.

[schmod]

On @iarna's advice, I've restructured this so loadDeps and loadDevDeps are once again discrete stages.

npm install now attempts to install a valid tree of prod + dev dependencies that most closely resembles the prod-only tree (possibly at the expense of reduced efficiency). We do this by installing the production dependencies, and subsequently install the devDeps on top of the resulting tree as an additive operation.

devDependencies (and their transitive deps) will attempt to re-use packages installed by the prod deps, but not the other way around.

There is one unavoidable exception to this rule (and the original impetus for this PR): Transitive production dependencies need to be hoisted out of the root package if the root package lists the same package as a devDependency.

If the devDependency and transitive production dependency happen to have the same version, we'll re-use the same module, but won't allow this to influence which versions get installed. I think that this offers the best tradeoff between predictability and efficiency.

---

One caveat:

This hoisting behavior only takes place if both prod AND dev dependencies are being installed simultaneously.

npm install is therefore NOT guaranteed to be equivalent to npm install --only=prod && npm install --only=dev, as the second command can potentially overwrite root-level packages installed by the first.

There are two possible fixes for this issue (if we even want to consider it an issue):

• Simple but inefficient: Always assume we might install our root-level devDeps, and always hoist transitive deps that conflict with devDeps at the root level. Easy fix. Predictable. Yields inefficient production-only trees.
• More complicated: If npm install overwrites a root-level package, it should determine if the overwritten package needs to reinstalled at a higher level in the tree to meet other dependencies. Should guarantee that a npm install command won't leave with any packages in node_modules with unmet dependencies. Could be computationally-intensive and produce huge trees if npm prune isn't run periodically. Might contradict user expectations of what --only does.

IMO, this is something we can tackle separately in the future.

[schmod]

Also, the failing test appears to be unrelated to this PR (as the same test is failing on other builds)

[zkat]

I think this might be ready for another round of review?

[mourner]

Any updates on this?

[iarna]

npm install is therefore NOT guaranteed to be equivalent to npm install --only=prod && npm install --only=dev, as the second command can potentially overwrite root-level packages installed by the first.

True, but the second run wouldn't leave a broken tree. npm is smart enough to hoist the transitive deps if it replaces one in a separate invocation. Which is to say, your "more complicated" scenario is what npm does today. This is what loadExtraneous.andResolveDeps is doing.

[iarna]

The top of the tree should have isTop: true set

[iarna]

(Setting this allows the assertions to fail without this patch. Without setting this, it crashes before the assertions can fail.)

[iarna]

Also needs isTop

[iarna]

This can't work because tree.package.devDependencies[pkg.name]) is a specifier, not a version. This needs to be a semver check.

[iarna]

It's not actually as simple as a semver check as not all specifiers are semver. This is what doesChildVersionMatch but that takes a package node as an argument, which makes this check awkward.

[iarna]

We also have --also and --only. This should probably be:

var alwaysIncludeDev = npm.config.get('dev') || /^dev(elopment)$/.test(npm.config.get('also'))
var onlyIncludeProduction = npm.config.get('production') || /^prod(uction)$/.test(npm.config.get('only'))
if (!tree.parent && (alwaysIncludeDev || !onlyIncludeProduction)) {

But to be honest, I'm not sure we should even gate this? Why not do this kind of hoisting even if you ask for --only=prod? It'd ensure that you got the same tree with npm install --only=prod && npm install --only=dev as you would with npm install

[iarna]

With some slight tweaking to the tests, this is how I'd approach this:

  var devDeps = tree.package.devDependencies || {}
  if (tree.isTop && devDeps[pkg.name]) {
    var requested = npa(pkg.name + '@' + devDeps[pkg.name])
    if (!doesChildVersionMatch({package:pkg}, requested, tree)) {
      return null
    }
  }

[iarna]

If you change this to ^1.0.0 then the test will fail where it should succeed.

[iarna]

My version of this PR: release-next...iarna/pr/12811 (squash commits before merging)

[zkat]

@schmod thanks a ton for this patch. Sorry about the delay in getting it merged! This PR was included in npm@4.0.5 :)