fix: refactor node.ideallyInert to node.inert (#8602)

Discovered while investigating
#8535 (comment)

Similar to #8566, relates to
#8184

Moves `inert` (uninstallable optional) calculation into `buildIdealTree`
so it can be used in diff.

Also removes most of #8184 in favor of a
[simpler
fix](https://github.com/npm/cli/pull/8602/files#diff-02626074e1a4a170693607e4a3a69dfc08ee52067734717833b22cf162923e07R354),
see PR comments for the journey.

Improvements:

* we don't see uninstallable packages as "installed" in CLI output
* `createSparseTree` no longer creates dirs that will only be cleaned
later

For the example in the linked issue, it changes output from `added 156
packages` to `added 12 packages` and combined with
#8537 it changes to `added 6 packages`,
the expected result.

Author: liamcmitchell

package-lock.json

@@ -5249,6 +5249,21 @@
       "dev": true,
       "license": "ISC"
     },
+    "node_modules/fsevents": {
+      "version": "2.3.3",
+      "resolved": "https://registry.npmjs.org/fsevents/-/fsevents-2.3.3.tgz",
+      "integrity": "sha512-5xoDfX+fL7faATnagmWPpbFtwh/R77WmMMqqHGS65C3vvB0YHrgF+B1YmZ3441tMj5n63k0212XNoJwzlhffQw==",
+      "dev": true,
+      "hasInstallScript": true,
+      "license": "MIT",
+      "optional": true,
+      "os": [
+        "darwin"
+      ],
+      "engines": {
+        "node": "^8.16.0 || ^10.6.0 || >=11.0.0"
+      }
+    },
     "node_modules/function-bind": {
       "version": "1.1.2",
       "dev": true,

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

@@ -192,7 +192,7 @@ module.exports = cls => class IdealTreeBuilder extends cls {
   }
 
   async #checkEngineAndPlatform () {
-    const { engineStrict, npmVersion, nodeVersion, omit = [] } = this.options
+    const { engineStrict, npmVersion, nodeVersion, omit = [], cpu, os, libc } = this.options
     const omitSet = new Set(omit)
 
     for (const node of this.idealTree.inventory.values()) {
@@ -214,6 +214,19 @@ module.exports = cls => class IdealTreeBuilder extends cls {
         }
         checkPlatform(node.package, this.options.force)
       }
+      if (node.optional && !node.inert) {
+        // Mark any optional packages we can't install as inert.
+        // We ignore the --force and --engine-strict flags.
+        try {
+          checkEngine(node.package, npmVersion, nodeVersion, false)
+          checkPlatform(node.package, false, { cpu, os, libc })
+        } catch (error) {
+          const set = optionalSet(node)
+          for (const node of set) {
+            node.inert = true
+          }
+        }
+      }
     }
   }
 
@@ -324,7 +337,7 @@ module.exports = cls => class IdealTreeBuilder extends cls {
       })
 
       .then(tree => {
-        // search the virtual tree for invalid edges, if any are found add their source to
+        // search the virtual tree for missing/invalid edges, if any are found add their source to
         // the depsQueue so that we'll fix it later
         depth({
           tree,
@@ -338,7 +351,7 @@ module.exports = cls => class IdealTreeBuilder extends cls {
           filter: node => node,
           visit: node => {
             for (const edge of node.edgesOut.values()) {
-              if (!edge.valid) {
+              if (!edge.to || !edge.valid) {
                 this.#depsQueue.push(node)
                 break // no need to continue the loop after the first hit
               }
@@ -811,7 +824,7 @@ This is a one-time fix-up, please be patient...
       node !== this.idealTree &&
       node.resolved &&
       (hasBundle || hasShrinkwrap) &&
-      !node.ideallyInert
+      !node.inert
     if (crackOpen) {
       const Arborist = this.constructor
       const opt = { ...this.options }
@@ -1561,7 +1574,7 @@ This is a one-time fix-up, please be patient...
 
       const set = optionalSet(node)
       for (const node of set) {
-        node.ideallyInert = true
+        node.inert = true
       }
     }
   }
@@ -1582,7 +1595,7 @@ This is a one-time fix-up, please be patient...
           node.parent !== null
           && !node.isProjectRoot
           && !excludeNodes.has(node)
-          && !node.ideallyInert
+          && !node.inert
         ) {
           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 && !next.ideallyInert) {
+      if (!next.isProjectRoot && !next.isWorkspace && !next.inert) {
         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 && !n.ideallyInert).map(this.externalProxyMemo))
-    result.externalOptionalDependencies = await Promise.all(optionalDeps.filter(n => !n.ideallyInert).map(this.externalProxyMemo))
+    result.externalDependencies = await Promise.all(nonOptionalDeps.filter(n => !n.isWorkspace && !n.inert).map(this.externalProxyMemo))
+    result.externalOptionalDependencies = await Promise.all(optionalDeps.filter(n => !n.inert).map(this.externalProxyMemo))
     result.dependencies = [
       ...result.externalDependencies,
       ...result.localDependencies,

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

@@ -269,7 +269,6 @@ To fix:
       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

@@ -7,7 +7,6 @@ const pacote = require('pacote')
 const promiseAllRejectLate = require('promise-all-reject-late')
 const runScript = require('@npmcli/run-script')
 const { callLimit: promiseCallLimit } = require('promise-call-limit')
-const { checkEngine, checkPlatform } = require('npm-install-checks')
 const { depth: dfwalk } = require('treeverse')
 const { dirname, resolve, relative, join } = require('node:path')
 const { log, time } = require('proc-log')
@@ -74,7 +73,6 @@ module.exports = cls => class Reifier extends cls {
   #dryRun
   #nmValidated = new Set()
   #omit
-  #omitted
   #retiredPaths = {}
   #retiredUnchanged = {}
   #savePrefix
@@ -99,7 +97,6 @@ module.exports = cls => class Reifier extends cls {
     }
 
     this.#omit = new Set(options.omit)
-    this.#omitted = new Set()
 
     // start tracker block
     this.addTracker('reify')
@@ -132,15 +129,17 @@ module.exports = cls => class Reifier extends cls {
       this.idealTree = oldTree
     }
     await this[_saveIdealTree](options)
-    // clean omitted
-    for (const node of this.#omitted) {
-      node.parent = null
+    // clean inert
+    for (const node of this.idealTree.inventory.values()) {
+      if (node.inert) {
+        node.parent = null
+      }
     }
     // clean up any trash that is still in the tree
     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 && !node.ideallyInert) {
+      if (node && node.root === this.idealTree) {
         node.parent = null
       }
     }
@@ -228,18 +227,6 @@ 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
 
@@ -465,7 +452,6 @@ module.exports = cls => class Reifier extends cls {
     // and ideal trees.
     this.diff = Diff.calculate({
       omit: this.#omit,
-      omitted: this.#omitted,
       shrinkwrapInflated: this.#shrinkwrapInflated,
       filterNodes,
       actual: this.actualTree,
@@ -566,9 +552,6 @@ 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
@@ -662,18 +645,7 @@ module.exports = cls => class Reifier extends cls {
     const timeEnd = time.start(`reifyNode:${node.location}`)
     this.addTracker('reify', node.name, node.location)
 
-    const { npmVersion, nodeVersion, cpu, os, libc } = this.options
     const p = Promise.resolve().then(async () => {
-      // when we reify an optional node, check the engine and platform
-      // first. be sure to ignore the --force and --engine-strict flags,
-      // since we always want to skip any optional packages we can't install.
-      // these checks throwing will result in a rollback and removal
-      // of the mismatches
-      // eslint-disable-next-line promise/always-return
-      if (node.optional) {
-        checkEngine(node.package, npmVersion, nodeVersion, false)
-        checkPlatform(node.package, false, { cpu, os, libc })
-      }
       await this[_checkBins](node)
       await this.#extractOrLink(node)
       const { _id, deprecated } = node.package
@@ -707,10 +679,6 @@ 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)
 
@@ -791,9 +759,9 @@ module.exports = cls => class Reifier extends cls {
   [_handleOptionalFailure] (node, p) {
     return (node.optional ? p.catch(() => {
       const set = optionalSet(node)
-      for (node of set) {
+      for (const node of set) {
         log.verbose('reify', 'failed optional dependency', node.path)
-        node.ideallyInert = true
+        node.inert = true
         this[_addNodeToTrashList](node)
       }
     }) : p).then(() => node)
@@ -1165,9 +1133,6 @@ 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 })
@@ -1247,7 +1212,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.ideallyInert) || node.target.fsTop === tree
+      const linkedFromRoot = (node.parent === tree && !node.inert) || node.target.fsTop === tree
       if (node.isLink && linkedFromRoot) {
         nodes.push(node)
       }

workspaces/arborist/lib/diff.js

@@ -11,9 +11,8 @@ const { existsSync } = require('node:fs')
 const ssri = require('ssri')
 
 class Diff {
-  constructor ({ actual, ideal, filterSet, shrinkwrapInflated, omit, omitted }) {
+  constructor ({ actual, ideal, filterSet, shrinkwrapInflated, omit }) {
     this.omit = omit
-    this.omitted = omitted
     this.filterSet = filterSet
     this.shrinkwrapInflated = shrinkwrapInflated
     this.children = []
@@ -39,7 +38,6 @@ class Diff {
     filterNodes = [],
     shrinkwrapInflated = new Set(),
     omit = new Set(),
-    omitted = new Set(),
   }) {
     // if there's a filterNode, then:
     // - get the path from the root to the filterNode.  The root or
@@ -98,28 +96,18 @@ class Diff {
     }
 
     return depth({
-      tree: new Diff({ actual, ideal, filterSet, shrinkwrapInflated, omit, omitted }),
+      tree: new Diff({ actual, ideal, filterSet, shrinkwrapInflated, omit }),
       getChildren,
       leave,
     })
   }
 }
 
-const getAction = ({ actual, ideal, omit, omitted }) => {
+const getAction = ({ actual, ideal }) => {
   if (!ideal) {
     return 'REMOVE'
   }
 
-  if (ideal.shouldOmit?.(omit)) {
-    omitted.add(ideal)
-
-    if (actual) {
-      return 'REMOVE'
-    }
-
-    return null
-  }
-
   // bundled meta-deps are copied over to the ideal tree when we visit it,
   // so they'll appear to be missing here.  There's no need to handle them
   // in the diff, though, because they'll be replaced at reify time anyway
@@ -199,7 +187,6 @@ const getChildren = diff => {
     filterSet,
     shrinkwrapInflated,
     omit,
-    omitted,
   } = diff
 
   // Note: we DON'T diff fsChildren themselves, because they are either
@@ -231,7 +218,6 @@ const getChildren = diff => {
       filterSet,
       shrinkwrapInflated,
       omit,
-      omitted,
     })
   }
 
@@ -251,21 +237,32 @@ const diffNode = ({
   filterSet,
   shrinkwrapInflated,
   omit,
-  omitted,
 }) => {
   if (filterSet.size && !(filterSet.has(ideal) || filterSet.has(actual))) {
     return
   }
 
-  const action = getAction({ actual, ideal, omit, omitted })
+  if (ideal?.shouldOmit?.(omit)) {
+    ideal.inert = true
+  }
+
+  // Treat inert nodes as undefined for the purposes of diffing.
+  if (ideal?.inert) {
+    ideal = undefined
+  }
+  if (!actual && !ideal) {
+    return
+  }
+
+  const action = getAction({ actual, ideal })
 
   // if it's a match, then get its children
   // otherwise, this is the child diff node
   if (action || (!shrinkwrapInflated.has(ideal) && ideal.hasShrinkwrap)) {
     if (action === 'REMOVE') {
       removed.push(actual)
     }
-    children.push(new Diff({ actual, ideal, filterSet, shrinkwrapInflated, omit, omitted }))
+    children.push(new Diff({ actual, ideal, filterSet, shrinkwrapInflated, omit }))
   } else {
     unchanged.push(ideal)
     // !*! Weird dirty hack warning !*!
@@ -306,7 +303,6 @@ const diffNode = ({
       filterSet,
       shrinkwrapInflated,
       omit,
-      omitted,
     }))
   }
 }

workspaces/arborist/lib/node.js

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