fix(arborist): omit failed optional dependencies from installed deps, but keep them 'in the tree'

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).

Author: zkat

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

@@ -100,6 +100,9 @@ module.exports = cls => class IsolatedReifier extends cls {
   }
 
   async externalProxy (result, node) {
+    if (node.ideallyInert) {
+      return
+    }
     await this.assignCommonProperties(node, result)
     if (node.hasShrinkwrap) {
       const dir = join(

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

@@ -150,7 +150,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
       }
     }
@@ -238,6 +238,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
 
@@ -600,6 +612,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
@@ -744,6 +759,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)
 
@@ -819,6 +838,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)
@@ -1151,6 +1171,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 })
@@ -1230,7 +1253,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,11 +136,15 @@ 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) {
     entries = [{ name: 'node_modules', isDirectory: () => true }]
-  } else {
+  } else if (!inert) {
     const { mtime: dirTime } = await stat(dir)
     if (dirTime > lockTime) {
       throw new Error(`out of date, updated: ${rel}`)
@@ -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