npm Community Forum (Archive)

The npm community forum has been discontinued.

To discuss usage of npm, visit the GitHub Support Community.

npm 6.8.0-next.0 regression in maximally flat install

What I Wanted to Do

I wanted to upgrade to the latest version of Jest (v24), and I happened to be using npm@next (v6.8.0-next.0). Instead of a maximally flat node_modules, I ended up with (seemingly) random modules unnecessarily nested.

What Happened Instead

babel-jest and request-promise-native were incorrectly nested underneath their dependents, instead of being installed at the root of node_modules as npm v6.7.0 does.

Reproduction Steps

The git steps are just to make the diff extremely clear.

$ npm -v
6.7.0

$ mkdir -p /tmp/npm-6.8.0-dehoisting-repro
$ cd $!
$ git init .
$ echo 'node_modules' >> .gitignore
$ npm init -y
$ git add .
$ git commit -m 'init'

# initial install of jest with npm 6.7.0 with correct lockfile
$ npm i -D jest
$ git add .
$ git commit -m "install jest"

# babel-jest is in the right spot ("maximally flat")
$ npm ls -lp babel-jest
$PWD/node_modules/babel-jest:babel-jest@24.0.0:undefined

# install jest using npm 6.8.0-next.0
npx npm@6.8.0-next.0 install jest@latest

# babel-jest has moved
$ npm ls -lp babel-jest
$PWD/node_modules/jest-config/node_modules/babel-jest:babel-jest@24.0.0:undefined

$ git diff
# below

Resulting diff:

diff --git a/package-lock.json b/package-lock.json
index 3ee3e79..aaa1726 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -358,16 +358,6 @@
       "integrity": "sha512-ReZxvNHIOv88FlT7rxcXIIC0fPt4KZqZbOlivyWtXLt8ESx84zd3kMC6iK5jVeS2qt+g7ftS7ye4fi06X5rtRQ==",
       "dev": true
     },
-    "babel-jest": {
-      "version": "24.0.0",
-      "resolved": "https://registry.npmjs.org/babel-jest/-/babel-jest-24.0.0.tgz",
-      "integrity": "sha512-YGKRbZUjoRmNIAyG7x4wYxUyHvHPFpYXj6Mx1A5cslhaQOUgP/+LF3wtFgMuOQkIpjbVNBufmOnVY0QVwB5v9Q==",
-      "dev": true,
-      "requires": {
-        "babel-plugin-istanbul": "^5.1.0",
-        "babel-preset-jest": "^24.0.0"
-      }
-    },
     "babel-plugin-istanbul": {
       "version": "5.1.0",
       "resolved": "https://registry.npmjs.org/babel-plugin-istanbul/-/babel-plugin-istanbul-5.1.0.tgz",
@@ -2523,6 +2513,18 @@
         "pretty-format": "^24.0.0",
         "realpath-native": "^1.0.2",
         "uuid": "^3.3.2"
+      },
+      "dependencies": {
+        "babel-jest": {
+          "version": "24.0.0",
+          "resolved": "https://registry.npmjs.org/babel-jest/-/babel-jest-24.0.0.tgz",
+          "integrity": "sha512-YGKRbZUjoRmNIAyG7x4wYxUyHvHPFpYXj6Mx1A5cslhaQOUgP/+LF3wtFgMuOQkIpjbVNBufmOnVY0QVwB5v9Q==",
+          "dev": true,
+          "requires": {
+            "babel-plugin-istanbul": "^5.1.0",
+            "babel-preset-jest": "^24.0.0"
+          }
+        }
       }
     },
     "jest-diff": {
@@ -2886,6 +2888,28 @@
         "whatwg-url": "^6.4.1",
         "ws": "^5.2.0",
         "xml-name-validator": "^3.0.0"
+      },
+      "dependencies": {
+        "request-promise-core": {
+          "version": "1.1.1",
+          "resolved": "https://registry.npmjs.org/request-promise-core/-/request-promise-core-1.1.1.tgz",
+          "integrity": "sha1-Pu4AssWqgyOc+wTFcA2jb4HNCLY=",
+          "dev": true,
+          "requires": {
+            "lodash": "^4.13.1"
+          }
+        },
+        "request-promise-native": {
+          "version": "1.0.5",
+          "resolved": "https://registry.npmjs.org/request-promise-native/-/request-promise-native-1.0.5.tgz",
+          "integrity": "sha1-UoF3D2jgyXGeUWP9P6tIIhX0/aU=",
+          "dev": true,
+          "requires": {
+            "request-promise-core": "1.1.1",
+            "stealthy-require": "^1.1.0",
+            "tough-cookie": ">=2.3.3"
+          }
+        }
       }
     },
     "jsesc": {
@@ -3763,26 +3787,6 @@
         }
       }
     },
-    "request-promise-core": {
-      "version": "1.1.1",
-      "resolved": "https://registry.npmjs.org/request-promise-core/-/request-promise-core-1.1.1.tgz",
-      "integrity": "sha1-Pu4AssWqgyOc+wTFcA2jb4HNCLY=",
-      "dev": true,
-      "requires": {
-        "lodash": "^4.13.1"
-      }
-    },
-    "request-promise-native": {
-      "version": "1.0.5",
-      "resolved": "https://registry.npmjs.org/request-promise-native/-/request-promise-native-1.0.5.tgz",
-      "integrity": "sha1-UoF3D2jgyXGeUWP9P6tIIhX0/aU=",
-      "dev": true,
-      "requires": {
-        "request-promise-core": "1.1.1",
-        "stealthy-require": "^1.1.0",
-        "tough-cookie": ">=2.3.3"
-      }
-    },
     "require-directory": {
       "version": "2.1.1",
       "resolved": "https://registry.npmjs.org/require-directory/-/require-directory-2.1.1.tgz",

Details

Without definitive evidence, this feels like it’s related to packages with peerDependencies are incorrectly hoisted, because that’s the only change since 6.7.0 in the install logic.

Searching the entire node_modules tree, the only other reference to babel-jest in a package.json file other than its primary dependent jest-config is in the devDependencies of cross-spawn.

request-promise-native is not present in any other package.json other than its primary dependent jsdom.

None of the de-hoisted modules is considered a peer of any other package, as far as I can tell.

Platform Info

$ npm --versions
{ 'npm-6.8.0-dehoisting-repro': '1.0.0',
  npm: '6.7.0',
  ares: '1.15.0',
  cldr: '33.1',
  http_parser: '2.8.0',
  icu: '62.1',
  modules: '64',
  napi: '3',
  nghttp2: '1.34.0',
  node: '10.15.1',
  openssl: '1.1.0j',
  tz: '2018e',
  unicode: '11.0',
  uv: '1.23.2',
  v8: '6.8.275.32-node.12',
  zlib: '1.2.11' }
$ node -p process.platform
darwin


After discussing with @iarna, I think this means we’ll need to revert @sokra’s patch. @iarna’s assessment that the required changes to be able to safely support peerDeps are much bigger are probably right, though there may be a separate patch where we do certain restructuring of the tree any time we hit peerDeps that might not mean “rewrite the tree resolver”.

I’m really really glad you found this! Thanks for using the prerelease! We’ll be releasing a new prerelease of 6.8.0 with those changes reverted (unfortunately). In the end, if we’re going to break one party or another, we’d rather things break the way they always have than randomly break a new group of people just to fix another.


Happy to provide useful feedback when I’m able to do so. :)


@evocateur I see the difference and also that the tree is not maximally flat, but it doesn’t look like the tree is “broken”.

Is there are chance to try fixing this issue, instead of reverting the bugfix?


The issue seem to be related to the trade-off I noted here: https://github.com/npm/cli/blob/3c22d1a35878f73c0af8ea5968b962a85a1a9b84/lib/install/deps.js#L807-L812

Here is what happens:

To be honest I didn’t even know that npm promises a maximally flat install while writing the bugfix.


One possible fix could be to make sure to run the peer dependency before the package with the peer dependency. Then we can run the peer dependency check on the real tree instead of the meta information.

In lib/deps.js loadDeps could be changed to first load meta data, sort dependencies and then install the packages into the tree.

loadDevDeps could be changed to loadDevAndNormalDeps and do the same.

In lib/install.js loadAllDepsIntoIdealTree could be changed to call only loadDevAndNormalDeps.