fix: calculate omit in diff (#8566)

Discovered while investigating #8535

`npm install --omit` output doesn't show any removed packages.

This PR moves the omit calc into the diff calc so omits are handled like
other resolution logic.

Improvements:
* we see removals in CLI output
* _createSparseTree no longer creates dirs that will only be cleaned
later
* no duplicate filterSet calculation, omit is calculated right after
filters

I removed the trashList check on reifying node. Code coverage complained
that this branch wasn't hit in any tests so I assume it should always be
empty.

I think this uncovered one bug with workspaces in the test `workspaces >
reify workspaces omit dev dependencies > workspaces only`. From my
understanding of the
[docs](https://docs.npmjs.com/cli/v11/commands/npm-install#include-workspace-root),
only workspace `a` should be touched, `root` and workspace `b` should
still have their packages. I updated the test.

Co-authored-by: Liam Mitchell <[email protected]>

Author: liamcmitchell

workspaces/arborist/lib/arborist/reify.js

@@ -42,7 +42,6 @@ const { defaultLockfileVersion } = Shrinkwrap
 const _retireShallowNodes = Symbol.for('retireShallowNodes')
 const _loadBundlesAndUpdateTrees = Symbol.for('loadBundlesAndUpdateTrees')
 const _submitQuickAudit = Symbol('submitQuickAudit')
-const _addOmitsToTrashList = Symbol('addOmitsToTrashList')
 const _unpackNewModules = Symbol.for('unpackNewModules')
 const _build = Symbol.for('build')
 
@@ -85,6 +84,7 @@ module.exports = cls => class Reifier extends cls {
   #dryRun
   #nmValidated = new Set()
   #omit
+  #omitted
   #retiredPaths = {}
   #retiredUnchanged = {}
   #savePrefix
@@ -109,6 +109,7 @@ module.exports = cls => class Reifier extends cls {
     }
 
     this.#omit = new Set(options.omit)
+    this.#omitted = new Set()
 
     // start tracker block
     this.addTracker('reify')
@@ -141,6 +142,10 @@ 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 up any trash that is still in the tree
     for (const path of this[_trashList]) {
       const loc = relpath(this.idealTree.realpath, path)
@@ -315,7 +320,6 @@ module.exports = cls => class Reifier extends cls {
       ]],
       [_rollbackCreateSparseTree, [
         _createSparseTree,
-        _addOmitsToTrashList,
         _loadShrinkwrapsAndUpdateTrees,
         _loadBundlesAndUpdateTrees,
         _submitQuickAudit,
@@ -470,6 +474,8 @@ module.exports = cls => class Reifier extends cls {
     // find all the nodes that need to change between the actual
     // and ideal trees.
     this.diff = Diff.calculate({
+      omit: this.#omit,
+      omitted: this.#omitted,
       shrinkwrapInflated: this.#shrinkwrapInflated,
       filterNodes,
       actual: this.actualTree,
@@ -554,37 +560,6 @@ 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.#omit.size) {
-      return
-    }
-
-    const timeEnd = time.start('reify:trashOmits')
-    for (const node of this.idealTree.inventory.values()) {
-      const { top } = node
-
-      // if the top is not the root or workspace then we do not want to omit it
-      if (!top.isProjectRoot && !top.isWorkspace) {
-        continue
-      }
-
-      // if a diff filter has been created, then we do not omit the node if the
-      // top node is not in that set
-      if (this.diff?.filterSet?.size && !this.diff.filterSet.has(top)) {
-        continue
-      }
-
-      // omit node if the dep type matches any omit flags that were set
-      if (node.shouldOmit(this.#omit)) {
-        this[_addNodeToTrashList](node)
-      }
-    }
-
-    timeEnd()
-  }
-
   [_createSparseTree] () {
     const timeEnd = time.start('reify:createSparse')
     // if we call this fn again, we look for the previous list
@@ -683,23 +658,17 @@ module.exports = cls => class Reifier extends cls {
       // reload the diff and sparse tree because the ideal tree changed
       .then(() => this[_diffTrees]())
       .then(() => this[_createSparseTree]())
-      .then(() => this[_addOmitsToTrashList]())
       .then(() => this[_loadShrinkwrapsAndUpdateTrees]())
       .then(timeEnd)
   }
 
   // create a symlink for Links, extract for Nodes
   // return the node object, since we usually want that
   // handle optional dep failures here
-  // If node is in trash list, skip it
   // If reifying fails, and the node is optional, add it and its optionalSet
   // to the trash list
   // Always return the node.
   [_reifyNode] (node) {
-    if (this[_trashList].has(node.path)) {
-      return node
-    }
-
     const timeEnd = time.start(`reifyNode:${node.location}`)
     this.addTracker('reify', node.name, node.location)
 

workspaces/arborist/lib/diff.js

@@ -11,7 +11,9 @@ const { existsSync } = require('node:fs')
 const ssri = require('ssri')
 
 class Diff {
-  constructor ({ actual, ideal, filterSet, shrinkwrapInflated }) {
+  constructor ({ actual, ideal, filterSet, shrinkwrapInflated, omit, omitted }) {
+    this.omit = omit
+    this.omitted = omitted
     this.filterSet = filterSet
     this.shrinkwrapInflated = shrinkwrapInflated
     this.children = []
@@ -36,6 +38,8 @@ class Diff {
     ideal,
     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
@@ -94,18 +98,28 @@ class Diff {
     }
 
     return depth({
-      tree: new Diff({ actual, ideal, filterSet, shrinkwrapInflated }),
+      tree: new Diff({ actual, ideal, filterSet, shrinkwrapInflated, omit, omitted }),
       getChildren,
       leave,
     })
   }
 }
 
-const getAction = ({ actual, ideal }) => {
+const getAction = ({ actual, ideal, omit, omitted }) => {
   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
@@ -184,6 +198,8 @@ const getChildren = diff => {
     removed,
     filterSet,
     shrinkwrapInflated,
+    omit,
+    omitted,
   } = diff
 
   // Note: we DON'T diff fsChildren themselves, because they are either
@@ -214,6 +230,8 @@ const getChildren = diff => {
       removed,
       filterSet,
       shrinkwrapInflated,
+      omit,
+      omitted,
     })
   }
 
@@ -232,20 +250,22 @@ const diffNode = ({
   removed,
   filterSet,
   shrinkwrapInflated,
+  omit,
+  omitted,
 }) => {
   if (filterSet.size && !(filterSet.has(ideal) || filterSet.has(actual))) {
     return
   }
 
-  const action = getAction({ actual, ideal })
+  const action = getAction({ actual, ideal, omit, omitted })
 
   // 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 }))
+    children.push(new Diff({ actual, ideal, filterSet, shrinkwrapInflated, omit, omitted }))
   } else {
     unchanged.push(ideal)
     // !*! Weird dirty hack warning !*!
@@ -285,6 +305,8 @@ const diffNode = ({
       removed,
       filterSet,
       shrinkwrapInflated,
+      omit,
+      omitted,
     }))
   }
 }

workspaces/arborist/lib/node.js

@@ -490,6 +490,18 @@ class Node {
   }
 
   shouldOmit (omitSet) {
+    if (!omitSet.size) {
+      return false
+    }
+
+    const { top } = this
+
+    // if the top is not the root or workspace then we do not want to omit it
+    if (!top.isProjectRoot && !top.isWorkspace) {
+      return false
+    }
+
+    // omit node if the dep type matches any omit flags that were set
     return (
       this.peer && omitSet.has('peer') ||
       this.dev && omitSet.has('dev') ||

workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs

@@ -17365,7 +17365,6 @@ Array [
   "reify:retireShallow",
   "reify:save",
   "reify:trash",
-  "reify:trashOmits",
   "reify:unpack",
   "reify:unretire",
   "reifyNode:node_modules/@isaacs/testing-peer-deps-b",

workspaces/arborist/test/arborist/reify.js

@@ -394,6 +394,21 @@ t.test('dev, optional, devOptional flags and omissions', t => {
   }))
 })
 
+t.test('omit reports no diff on second run', async t => {
+  const path = fixture(t, 'testing-dev-optional-flags')
+  createRegistry(t, true)
+  const arb = newArb({ path })
+  await arb.reify({ omit: ['dev'] })
+  t.equal(arb.actualTree.children.get('once'), undefined, 'no once in tree')
+  t.ok(arb.diff.children.length, 'first reify has changes')
+  await arb.reify({ omit: ['dev'] })
+  t.equal(arb.actualTree.children.get('once'), undefined, 'no once in tree')
+  t.notOk(arb.diff.children.length, 'second reify has no changes')
+  await arb.reify({})
+  t.ok(arb.actualTree.children.get('once'), 'once in tree')
+  t.ok(arb.diff.children.length, 'removing omit has changes')
+})
+
 t.test('omits when both dev and optional flags are set', t => {
   const path = 'testing-dev-optional-flags-2'
   const omits = [['dev'], ['optional']]
@@ -1329,7 +1344,7 @@ t.test('workspaces', async t => {
     await t.test('workspaces only', async t => {
       createRegistry(t, false)
       const { root, a, b } = await runCase(t, { workspaces: ['a'] })
-      t.equal(root.exists(), false, 'root')
+      t.equal(root.exists(), true, 'root')
       t.equal(a.exists(), false, 'a')
       t.equal(b.exists(), true, 'b')
     })