fix: include ideally inert in diff

Author: liamcmitchell

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.ideallyInert) {
+        // Mark any optional packages we can't install as ideally 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.ideallyInert = true
+          }
+        }
+      }
     }
   }
 

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')
@@ -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
 
@@ -566,9 +553,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 +646,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 +680,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,7 +760,7 @@ 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
         this[_addNodeToTrashList](node)
@@ -1165,9 +1134,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 })

workspaces/arborist/lib/diff.js

@@ -257,6 +257,17 @@ const diffNode = ({
     return
   }
 
+  // Treat inert nodes as undefined for the purposes of diffing.
+  if (actual?.ideallyInert) {
+    actual = undefined
+  }
+  if (ideal?.ideallyInert) {
+    ideal = undefined
+  }
+  if (!actual && !ideal) {
+    return
+  }
+
   const action = getAction({ actual, ideal, omit, omitted })
 
   // if it's a match, then get its children

workspaces/arborist/test/arborist/reify.js

@@ -496,6 +496,14 @@ t.test('do not install optional deps with mismatched platform specifications', a
   await t.resolveMatchSnapshot(printReified(fixture(t, 'optional-platform-specification')))
 })
 
+t.test('do not report failed optional deps as installed', async t => {
+  createRegistry(t, true)
+  const path = fixture(t, 'optional-platform-specification')
+  const arb = newArb({ path })
+  await arb.reify()
+  t.equal(arb.diff.children.length, 0, 'no changes, nothing installed')
+})
+
 t.test('still do not install optional deps with mismatched platform specifications even when forced', async t => {
   createRegistry(t, true)
   await t.resolveMatchSnapshot(printReified(fixture(t, 'optional-platform-specification'), { force: true }))