fix: use omit when checking ideal tree engine (#8372)

Also centralizes shouldOmit logic

Author: owlstronaut

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

@@ -192,9 +192,11 @@ module.exports = cls => class IdealTreeBuilder extends cls {
   }
 
   async #checkEngineAndPlatform () {
-    const { engineStrict, npmVersion, nodeVersion } = this.options
+    const { engineStrict, npmVersion, nodeVersion, omit = [] } = this.options
+    const omitSet = new Set(omit)
+
     for (const node of this.idealTree.inventory.values()) {
-      if (!node.optional) {
+      if (!node.optional && !node.shouldOmit(omitSet)) {
         try {
           // if devEngines is present in the root node we ignore the engines check
           if (!(node.isRoot && node.package.devEngines)) {

workspaces/arborist/lib/arborist/reify.js

@@ -84,9 +84,7 @@ module.exports = cls => class Reifier extends cls {
   #bundleUnpacked = new Set() // the nodes we unpack to read their bundles
   #dryRun
   #nmValidated = new Set()
-  #omitDev
-  #omitPeer
-  #omitOptional
+  #omit
   #retiredPaths = {}
   #retiredUnchanged = {}
   #savePrefix
@@ -110,10 +108,7 @@ module.exports = cls => class Reifier extends cls {
       throw er
     }
 
-    const omit = new Set(options.omit || [])
-    this.#omitDev = omit.has('dev')
-    this.#omitOptional = omit.has('optional')
-    this.#omitPeer = omit.has('peer')
+    this.#omit = new Set(options.omit)
 
     // start tracker block
     this.addTracker('reify')
@@ -562,12 +557,11 @@ module.exports = cls => class Reifier extends cls {
   // adding to the trash list will skip reifying, and delete them
   // if they are currently in the tree and otherwise untouched.
   [_addOmitsToTrashList] () {
-    if (!this.#omitDev && !this.#omitOptional && !this.#omitPeer) {
+    if (!this.#omit.size) {
       return
     }
 
     const timeEnd = time.start('reify:trashOmits')
-
     for (const node of this.idealTree.inventory.values()) {
       const { top } = node
 
@@ -583,12 +577,7 @@ module.exports = cls => class Reifier extends cls {
       }
 
       // omit node if the dep type matches any omit flags that were set
-      if (
-        node.peer && this.#omitPeer ||
-        node.dev && this.#omitDev ||
-        node.optional && this.#omitOptional ||
-        node.devOptional && this.#omitOptional && this.#omitDev
-      ) {
+      if (node.shouldOmit(this.#omit)) {
         this[_addNodeToTrashList](node)
       }
     }

workspaces/arborist/lib/audit-report.js

@@ -148,7 +148,7 @@ class AuditReport extends Map {
       if (!seen.has(k)) {
         const p = []
         for (const node of this.tree.inventory.query('packageName', name)) {
-          if (!shouldAudit(node, this[_omit], this.filterSet)) {
+          if (!this.shouldAudit(node)) {
             continue
           }
 
@@ -282,7 +282,7 @@ class AuditReport extends Map {
 
     const timeEnd = time.start('auditReport:getReport')
     try {
-      const body = prepareBulkData(this.tree, this[_omit], this.filterSet)
+      const body = this.prepareBulkData()
       log.silly('audit', 'bulk request', body)
 
       // no sense asking if we don't have anything to audit,
@@ -309,37 +309,33 @@ class AuditReport extends Map {
       timeEnd()
     }
   }
-}
 
-// return true if we should audit this one
-const shouldAudit = (node, omit, filterSet) =>
-  !node.version ? false
-  : node.isRoot ? false
-  : filterSet && filterSet.size !== 0 && !filterSet.has(node) ? false
-  : omit.size === 0 ? true
-  : !( // otherwise, just ensure we're not omitting this one
-    node.dev && omit.has('dev') ||
-    node.optional && omit.has('optional') ||
-    node.devOptional && omit.has('dev') && omit.has('optional') ||
-    node.peer && omit.has('peer')
-  )
-
-const prepareBulkData = (tree, omit, filterSet) => {
-  const payload = {}
-  for (const name of tree.inventory.query('packageName')) {
-    const set = new Set()
-    for (const node of tree.inventory.query('packageName', name)) {
-      if (!shouldAudit(node, omit, filterSet)) {
-        continue
-      }
+  // return true if we should audit this one
+  shouldAudit (node) {
+    return !node.version ? false
+      : node.isRoot ? false
+      : this.filterSet && this.filterSet.size !== 0 && !this.filterSet.has(node) ? false
+      : this[_omit].size === 0 ? true
+      : !node.shouldOmit(this[_omit])
+  }
 
-      set.add(node.version)
-    }
-    if (set.size) {
-      payload[name] = [...set]
+  prepareBulkData () {
+    const payload = {}
+    for (const name of this.tree.inventory.query('packageName')) {
+      const set = new Set()
+      for (const node of this.tree.inventory.query('packageName', name)) {
+        if (!this.shouldAudit(node)) {
+          continue
+        }
+
+        set.add(node.version)
+      }
+      if (set.size) {
+        payload[name] = [...set]
+      }
     }
+    return payload
   }
-  return payload
 }
 
 module.exports = AuditReport

workspaces/arborist/lib/node.js

@@ -489,6 +489,15 @@ class Node {
     return false
   }
 
+  shouldOmit (omitSet) {
+    return (
+      this.peer && omitSet.has('peer') ||
+      this.dev && omitSet.has('dev') ||
+      this.optional && omitSet.has('optional') ||
+      this.devOptional && omitSet.has('optional') && omitSet.has('dev')
+    )
+  }
+
   getBundler (path = []) {
     // made a cycle, definitely not bundled!
     if (path.includes(this)) {

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

@@ -4091,3 +4091,47 @@ t.test('should take devEngines in account', async t => {
   const tree = await buildIdeal(path)
   t.matchSnapshot(String(tree.meta))
 })
+
+t.test('engine checking respects omit flags', async t => {
+  const testFixture = resolve(fixtures, 'engine-omit-test')
+
+  t.test('fail on engine mismatch in devDependencies without omit=dev', async t => {
+    await t.rejects(buildIdeal(testFixture, {
+      nodeVersion: '12.18.4',
+      engineStrict: true,
+    }),
+    { code: 'EBADENGINE' },
+    'should fail with EBADENGINE when devDependencies have engine mismatch'
+    )
+  })
+
+  t.test('skip engine check for devDependencies with omit=dev', async t => {
+    // This should NOT throw an EBADENGINE error
+    await buildIdeal(testFixture, {
+      nodeVersion: '12.18.4',
+      engineStrict: true,
+      omit: ['dev'],
+    })
+    t.pass('should succeed when omitting dev dependencies with engine mismatches')
+  })
+
+  t.test('skip engine check for optionalDependencies with omit=optional', async t => {
+    const optionalFixture = resolve(fixtures, 'optional-engine-omit-test')
+    await buildIdeal(optionalFixture, {
+      nodeVersion: '12.18.4',
+      engineStrict: true,
+      omit: ['optional'],
+    })
+    t.pass('should succeed when omitting optional dependencies with engine mismatches')
+  })
+
+  t.test('skip engine check for peerDependencies with omit=peer', async t => {
+    const peerFixture = resolve(fixtures, 'peer-engine-omit-test')
+    await buildIdeal(peerFixture, {
+      nodeVersion: '12.18.4',
+      engineStrict: true,
+      omit: ['peer'],
+    })
+    t.pass('should succeed when omitting peer dependencies with engine mismatches')
+  })
+})

workspaces/arborist/test/fixtures/engine-omit-test/node_modules/strict-engine-dev/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "engine-omit-test",
+  "version": "1.0.0",
+  "devDependencies": {
+    "strict-engine-dev": "1.0.0"
+  }
+}

workspaces/arborist/test/fixtures/engine-omit-test/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "optional-engine-omit-test",
+  "version": "1.0.0",
+  "optionalDependencies": {
+    "strict-engine-optional": "1.0.0"
+  }
+}