fix(arborist): omit failed optional dependencies from installed deps (#8184)

… but keep them 'in the tree'

This PR was authored by @zkat 

Fixes: #4828
Fixes: #7961
Replaces: #8127
Replaces: #8077

When optional dependencies fail, we don't want to install them, for
whatever reason. The way this "uninstallation" is done right now is by
simply removing the dependencies from the ideal tree during reification.

Unfortunately, this means that what gets saved to the "hidden lockfile"
is the edited ideal tree as well, and when npm runs next, it'll use that
when regenerating the "real" package-lock.

This PR tags failed optional deps such that they're still added to the
"trash list" (and thus have their directories cleaned up by the
reifier), but prevents Arborist from removing them altogether from the
ideal tree (which is usually done by setting their parent to `null`,
making them unreachable, and letting them get GC'd).

PS: It's Friday, this is what I managed to get done together. I'm making
this a draft PR for now so folks can look at it, but I want to make sure
both reifiers work well, fix some messaging issues, and clean stuff up
(I'm pretty sure I'm guarding `ideallyInert` in more places than I need
to, because I was trying to find the right spot). Still, feel free to
talk about the approach. I'll get back to this on Monday.

PPS: also hi

Co-authored-by: Kat Marchán <[email protected]>

Author: owlstronaut

workspaces/arborist/lib/arborist/build-ideal-tree.js

@@ -809,7 +809,8 @@ This is a one-time fix-up, please be patient...
     const crackOpen = this.#complete &&
       node !== this.idealTree &&
       node.resolved &&
-      (hasBundle || hasShrinkwrap)
+      (hasBundle || hasShrinkwrap) &&
+      !node.ideallyInert
     if (crackOpen) {
       const Arborist = this.constructor
       const opt = { ...this.options }
@@ -1527,7 +1528,7 @@ This is a one-time fix-up, please be patient...
 
       const set = optionalSet(node)
       for (const node of set) {
-        node.parent = null
+        node.ideallyInert = true
       }
     }
   }
@@ -1548,6 +1549,7 @@ This is a one-time fix-up, please be patient...
           node.parent !== null
           && !node.isProjectRoot
           && !excludeNodes.has(node)
+          && !node.ideallyInert
         ) {
           this[_addNodeToTrashList](node)
         }

workspaces/arborist/lib/arborist/isolated-reifier.js

@@ -81,7 +81,7 @@ module.exports = cls => class IsolatedReifier extends cls {
         }
         queue.push(e.to)
       })
-      if (!next.isProjectRoot && !next.isWorkspace) {
+      if (!next.isProjectRoot && !next.isWorkspace && !next.ideallyInert) {
         root.external.push(await this.externalProxyMemo(next))
       }
     }
@@ -147,8 +147,8 @@ module.exports = cls => class IsolatedReifier extends cls {
     const nonOptionalDeps = edges.filter(e => !e.optional).map(e => e.to.target)
 
     result.localDependencies = await Promise.all(nonOptionalDeps.filter(n => n.isWorkspace).map(this.workspaceProxyMemo))
-    result.externalDependencies = await Promise.all(nonOptionalDeps.filter(n => !n.isWorkspace).map(this.externalProxyMemo))
-    result.externalOptionalDependencies = await Promise.all(optionalDeps.map(this.externalProxyMemo))
+    result.externalDependencies = await Promise.all(nonOptionalDeps.filter(n => !n.isWorkspace && !n.ideallyInert).map(this.externalProxyMemo))
+    result.externalOptionalDependencies = await Promise.all(optionalDeps.filter(n => !n.ideallyInert).map(this.externalProxyMemo))
     result.dependencies = [
       ...result.externalDependencies,
       ...result.localDependencies,

workspaces/arborist/lib/arborist/load-virtual.js

@@ -267,6 +267,7 @@ module.exports = cls => class VirtualLoader extends cls {
       integrity: sw.integrity,
       resolved: consistentResolve(sw.resolved, this.path, path),
       pkg: sw,
+      ideallyInert: sw.ideallyInert,
       hasShrinkwrap: sw.hasShrinkwrap,
       dev,
       optional,

workspaces/arborist/lib/arborist/reify.js

@@ -149,7 +149,7 @@ module.exports = cls => class Reifier extends cls {
     for (const path of this[_trashList]) {
       const loc = relpath(this.idealTree.realpath, path)
       const node = this.idealTree.inventory.get(loc)
-      if (node && node.root === this.idealTree) {
+      if (node && node.root === this.idealTree && !node.ideallyInert) {
         node.parent = null
       }
     }
@@ -237,6 +237,18 @@ module.exports = cls => class Reifier extends cls {
     this.idealTree.meta.hiddenLockfile = true
     this.idealTree.meta.lockfileVersion = defaultLockfileVersion
 
+    // Preserve inertness for failed stuff.
+    if (this.actualTree) {
+      for (const [loc, actual] of this.actualTree.inventory.entries()) {
+        if (actual.ideallyInert) {
+          const ideal = this.idealTree.inventory.get(loc)
+          if (ideal) {
+            ideal.ideallyInert = true
+          }
+        }
+      }
+    }
+
     this.actualTree = this.idealTree
     this.idealTree = null
 
@@ -599,6 +611,9 @@ module.exports = cls => class Reifier extends cls {
     // retire the same path at the same time.
     const dirsChecked = new Set()
     return promiseAllRejectLate(leaves.map(async node => {
+      if (node.ideallyInert) {
+        return
+      }
       for (const d of walkUp(node.path)) {
         if (d === node.top.path) {
           break
@@ -743,6 +758,10 @@ module.exports = cls => class Reifier extends cls {
   }
 
   async #extractOrLink (node) {
+    if (node.ideallyInert) {
+      return
+    }
+
     const nm = resolve(node.parent.path, 'node_modules')
     await this.#validateNodeModules(nm)
 
@@ -818,6 +837,7 @@ module.exports = cls => class Reifier extends cls {
       const set = optionalSet(node)
       for (node of set) {
         log.verbose('reify', 'failed optional dependency', node.path)
+        node.ideallyInert = true
         this[_addNodeToTrashList](node)
       }
     }) : p).then(() => node)
@@ -1152,6 +1172,9 @@ module.exports = cls => class Reifier extends cls {
 
       this.#retiredUnchanged[retireFolder] = []
       return promiseAllRejectLate(diff.unchanged.map(node => {
+        if (node.ideallyInert) {
+          return
+        }
         // no need to roll back links, since we'll just delete them anyway
         if (node.isLink) {
           return mkdir(dirname(node.path), { recursive: true, force: true })
@@ -1231,7 +1254,7 @@ module.exports = cls => class Reifier extends cls {
       // skip links that only live within node_modules as they are most
       // likely managed by packages we installed, we only want to rebuild
       // unchanged links we directly manage
-      const linkedFromRoot = node.parent === tree || node.target.fsTop === tree
+      const linkedFromRoot = (node.parent === tree && !node.ideallyInert) || node.target.fsTop === tree
       if (node.isLink && linkedFromRoot) {
         nodes.push(node)
       }

workspaces/arborist/lib/node.js

@@ -103,6 +103,7 @@ class Node {
       global = false,
       dummy = false,
       sourceReference = null,
+      ideallyInert = false,
     } = options
     // this object gives querySelectorAll somewhere to stash context about a node
     // while processing a query
@@ -197,6 +198,8 @@ class Node {
       this.extraneous = false
     }
 
+    this.ideallyInert = ideallyInert
+
     this.edgesIn = new Set()
     this.edgesOut = new CaseInsensitiveMap()
 

workspaces/arborist/lib/shrinkwrap.js

@@ -109,6 +109,7 @@ const nodeMetaKeys = [
   'inBundle',
   'hasShrinkwrap',
   'hasInstallScript',
+  'ideallyInert',
 ]
 
 const metaFieldFromPkg = (pkg, key) => {
@@ -135,6 +136,10 @@ const assertNoNewer = async (path, data, lockTime, dir, seen) => {
 
   const parent = isParent ? dir : resolve(dir, 'node_modules')
   const rel = relpath(path, dir)
+  const inert = data.packages[rel]?.ideallyInert
+  if (inert) {
+    return
+  }
   seen.add(rel)
   let entries
   if (dir === path) {
@@ -173,7 +178,7 @@ const assertNoNewer = async (path, data, lockTime, dir, seen) => {
 
   // assert that all the entries in the lockfile were seen
   for (const loc in data.packages) {
-    if (!seen.has(loc)) {
+    if (!seen.has(loc) && !data.packages[loc].ideallyInert) {
       throw new Error(`missing from node_modules: ${loc}`)
     }
   }
@@ -783,6 +788,10 @@ class Shrinkwrap {
       // ok, I did my best!  good luck!
     }
 
+    if (lock.ideallyInert) {
+      meta.ideallyInert = true
+    }
+
     if (lock.bundled) {
       meta.inBundle = true
     }
@@ -953,6 +962,12 @@ class Shrinkwrap {
       this.#buildLegacyLockfile(this.tree, this.data)
     }
 
+    if (!this.hiddenLockfile) {
+      for (const node of Object.values(this.data.packages)) {
+        delete node.ideallyInert
+      }
+    }
+
     // lf version 1 = dependencies only
     // lf version 2 = dependencies and packages
     // lf version 3 = packages only

workspaces/arborist/tap-snapshots/test/arborist/build-ideal-tree.js.test.cjs

@@ -77868,11 +77868,34 @@ ArboristNode {
 
 exports[`test/arborist/build-ideal-tree.js TAP optional dependency failures > optional-dep-enotarget 1`] = `
 ArboristNode {
+  "children": Map {
+    "tap" => ArboristNode {
+      "edgesIn": Set {
+        EdgeIn {
+          "error": "INVALID",
+          "from": "",
+          "name": "tap",
+          "spec": "9999.0000.9999",
+          "type": "optional",
+        },
+      },
+      "errors": Array [
+        Object {
+          "code": "ETARGET",
+        },
+      ],
+      "location": "node_modules/tap",
+      "name": "tap",
+      "optional": true,
+      "path": "{CWD}/test/fixtures/optional-dep-enotarget/node_modules/tap",
+    },
+  },
   "edgesOut": Map {
     "tap" => EdgeOut {
+      "error": "INVALID",
       "name": "tap",
       "spec": "9999.0000.9999",
-      "to": null,
+      "to": "node_modules/tap",
       "type": "optional",
     },
   },
@@ -77887,11 +77910,32 @@ ArboristNode {
 
 exports[`test/arborist/build-ideal-tree.js TAP optional dependency failures > optional-dep-missing 1`] = `
 ArboristNode {
+  "children": Map {
+    "@isaacs/this-does-not-exist-at-all" => ArboristNode {
+      "edgesIn": Set {
+        EdgeIn {
+          "from": "",
+          "name": "@isaacs/this-does-not-exist-at-all",
+          "spec": "*",
+          "type": "optional",
+        },
+      },
+      "errors": Array [
+        Object {
+          "code": "E404",
+        },
+      ],
+      "location": "node_modules/@isaacs/this-does-not-exist-at-all",
+      "name": "@isaacs/this-does-not-exist-at-all",
+      "optional": true,
+      "path": "{CWD}/test/fixtures/optional-dep-missing/node_modules/@isaacs/this-does-not-exist-at-all",
+    },
+  },
   "edgesOut": Map {
     "@isaacs/this-does-not-exist-at-all" => EdgeOut {
       "name": "@isaacs/this-does-not-exist-at-all",
       "spec": "*",
-      "to": null,
+      "to": "node_modules/@isaacs/this-does-not-exist-at-all",
       "type": "optional",
     },
   },
@@ -77906,11 +77950,60 @@ ArboristNode {
 
 exports[`test/arborist/build-ideal-tree.js TAP optional dependency failures > optional-metadep-enotarget 1`] = `
 ArboristNode {
+  "children": Map {
+    "@isaacs/prod-dep-enotarget" => ArboristNode {
+      "children": Map {
+        "tap" => ArboristNode {
+          "edgesIn": Set {
+            EdgeIn {
+              "error": "INVALID",
+              "from": "node_modules/@isaacs/prod-dep-enotarget",
+              "name": "tap",
+              "spec": "9999.0000.9999",
+              "type": "prod",
+            },
+          },
+          "errors": Array [
+            Object {
+              "code": "ETARGET",
+            },
+          ],
+          "location": "node_modules/@isaacs/prod-dep-enotarget/node_modules/tap",
+          "name": "tap",
+          "optional": true,
+          "path": "{CWD}/test/fixtures/optional-metadep-enotarget/node_modules/@isaacs/prod-dep-enotarget/node_modules/tap",
+        },
+      },
+      "edgesIn": Set {
+        EdgeIn {
+          "from": "",
+          "name": "@isaacs/prod-dep-enotarget",
+          "spec": "*",
+          "type": "optional",
+        },
+      },
+      "edgesOut": Map {
+        "tap" => EdgeOut {
+          "error": "INVALID",
+          "name": "tap",
+          "spec": "9999.0000.9999",
+          "to": "node_modules/@isaacs/prod-dep-enotarget/node_modules/tap",
+          "type": "prod",
+        },
+      },
+      "location": "node_modules/@isaacs/prod-dep-enotarget",
+      "name": "@isaacs/prod-dep-enotarget",
+      "optional": true,
+      "path": "{CWD}/test/fixtures/optional-metadep-enotarget/node_modules/@isaacs/prod-dep-enotarget",
+      "resolved": "https://registry.npmjs.org/@isaacs/prod-dep-enotarget/-/prod-dep-enotarget-1.0.0.tgz",
+      "version": "1.0.0",
+    },
+  },
   "edgesOut": Map {
     "@isaacs/prod-dep-enotarget" => EdgeOut {
       "name": "@isaacs/prod-dep-enotarget",
       "spec": "*",
-      "to": null,
+      "to": "node_modules/@isaacs/prod-dep-enotarget",
       "type": "optional",
     },
   },
@@ -77924,11 +78017,58 @@ ArboristNode {
 
 exports[`test/arborist/build-ideal-tree.js TAP optional dependency failures > optional-metadep-missing 1`] = `
 ArboristNode {
+  "children": Map {
+    "@isaacs/testing-prod-dep-metadata-missing" => ArboristNode {
+      "children": Map {
+        "@isaacs/this-does-not-exist-at-all" => ArboristNode {
+          "edgesIn": Set {
+            EdgeIn {
+              "from": "node_modules/@isaacs/testing-prod-dep-metadata-missing",
+              "name": "@isaacs/this-does-not-exist-at-all",
+              "spec": "*",
+              "type": "prod",
+            },
+          },
+          "errors": Array [
+            Object {
+              "code": "E404",
+            },
+          ],
+          "location": "node_modules/@isaacs/testing-prod-dep-metadata-missing/node_modules/@isaacs/this-does-not-exist-at-all",
+          "name": "@isaacs/this-does-not-exist-at-all",
+          "optional": true,
+          "path": "{CWD}/test/fixtures/optional-metadep-missing/node_modules/@isaacs/testing-prod-dep-metadata-missing/node_modules/@isaacs/this-does-not-exist-at-all",
+        },
+      },
+      "edgesIn": Set {
+        EdgeIn {
+          "from": "",
+          "name": "@isaacs/testing-prod-dep-metadata-missing",
+          "spec": "*",
+          "type": "optional",
+        },
+      },
+      "edgesOut": Map {
+        "@isaacs/this-does-not-exist-at-all" => EdgeOut {
+          "name": "@isaacs/this-does-not-exist-at-all",
+          "spec": "*",
+          "to": "node_modules/@isaacs/testing-prod-dep-metadata-missing/node_modules/@isaacs/this-does-not-exist-at-all",
+          "type": "prod",
+        },
+      },
+      "location": "node_modules/@isaacs/testing-prod-dep-metadata-missing",
+      "name": "@isaacs/testing-prod-dep-metadata-missing",
+      "optional": true,
+      "path": "{CWD}/test/fixtures/optional-metadep-missing/node_modules/@isaacs/testing-prod-dep-metadata-missing",
+      "resolved": "https://registry.npmjs.org/@isaacs/testing-prod-dep-metadata-missing/-/testing-prod-dep-metadata-missing-1.0.0.tgz",
+      "version": "1.0.0",
+    },
+  },
   "edgesOut": Map {
     "@isaacs/testing-prod-dep-metadata-missing" => EdgeOut {
       "name": "@isaacs/testing-prod-dep-metadata-missing",
       "spec": "*",
-      "to": null,
+      "to": "node_modules/@isaacs/testing-prod-dep-metadata-missing",
       "type": "optional",
     },
   },

workspaces/arborist/tap-snapshots/test/arborist/load-actual.js.test.cjs

@@ -7784,4 +7784,4 @@ ArboristNode {
   "name": "yarn-lock-mkdirp-file-dep",
   "path": "yarn-lock-mkdirp-file-dep",
 }
-`
+`