npm 6.8.0-next.0 regression in maximally flat install


(Daniel Stockman) #1

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

(Kat Marchán) #2

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.


(Daniel Stockman) #3

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


(Tobias Koppers) #4

@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?


(Tobias Koppers) #5

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:

  • babel-jest has a peer dependency to @babel/core
  • jest-config has a dependency on babel-jest and on @babel/core
  • While installing we don’t know in which order babel-jest and @babel/core are installed
  • When deciding if babel-jest should be placed in jest-config or hoisted to root, we don’t know if jest-config will contain @babel/core or not.
  • If jest-config would contain @babel/core it would be wrong to hoist babel-jest to the root (as it would no longer have access to the same instance of @babel/core as promised by the peer dependencies).
  • Because we don’t know the location of @babel/core we choose to not hoist it to root, which is a safe choice here.
  • Haven’t checked but I assume the same happens for the other dependencies.

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


(Tobias Koppers) #6

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.