fix: make omit flags work properly with workspaces (#6549)

Co-authored-by: Luke Karrys <luke@lukekarrys.com>

Author: Rayyan98

workspaces/arborist/lib/arborist/reify.js

@@ -483,17 +483,29 @@ module.exports = cls => class Reifier extends cls {
 
     process.emit('time', 'reify:trashOmits')
 
-    const filter = node =>
-      node.top.isProjectRoot &&
-        (
-          node.peer && this[_omitPeer] ||
-          node.dev && this[_omitDev] ||
-          node.optional && this[_omitOptional] ||
-          node.devOptional && this[_omitOptional] && this[_omitDev]
-        )
-
-    for (const node of this.idealTree.inventory.filter(filter)) {
-      this[_addNodeToTrashList](node)
+    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.peer && this[_omitPeer] ||
+        node.dev && this[_omitDev] ||
+        node.optional && this[_omitOptional] ||
+        node.devOptional && this[_omitOptional] && this[_omitDev]
+      ) {
+        this[_addNodeToTrashList](node)
+      }
     }
 
     process.emit('timeEnd', 'reify:trashOmits')

workspaces/arborist/test/arborist/reify.js

@@ -1187,6 +1187,55 @@ t.test('workspaces', t => {
   t.test('reify simple-workspaces', t =>
     t.resolveMatchSnapshot(printReified(fixture(t, 'workspaces-simple')), 'should reify simple workspaces'))
 
+  t.test('reify workspaces omit dev dependencies', async t => {
+    const runCase = async (t, opts) => {
+      const path = fixture(t, 'workspaces-conflicting-dev-deps')
+      const rootAjv = resolve(path, 'node_modules/ajv/package.json')
+      const ajvOfPkgA = resolve(path, 'a/node_modules/ajv/package.json')
+      const ajvOfPkgB = resolve(path, 'b/node_modules/ajv/package.json')
+
+      t.equal(fs.existsSync(rootAjv), true, 'root ajv exists')
+      t.equal(fs.existsSync(ajvOfPkgA), true, 'ajv under package a node_modules exists')
+      t.equal(fs.existsSync(ajvOfPkgB), true, 'ajv under package a node_modules exists')
+
+      await reify(path, { omit: ['dev'], ...opts })
+
+      return {
+        root: { exists: () => fs.existsSync(rootAjv) },
+        a: { exists: () => fs.existsSync(ajvOfPkgA) },
+        b: { exists: () => fs.existsSync(ajvOfPkgB) },
+      }
+    }
+
+    await t.test('default', async t => {
+      const { root, a, b } = await runCase(t)
+      t.equal(root.exists(), false, 'root')
+      t.equal(a.exists(), false, 'a')
+      t.equal(b.exists(), false, 'b')
+    })
+
+    await t.test('workspaces only', async t => {
+      const { root, a, b } = await runCase(t, { workspaces: ['a'] })
+      t.equal(root.exists(), false, 'root')
+      t.equal(a.exists(), false, 'a')
+      t.equal(b.exists(), true, 'b')
+    })
+
+    await t.test('workspaces + root', async t => {
+      const { root, a, b } = await runCase(t, { workspaces: ['a'], includeWorkspaceRoot: true })
+      t.equal(root.exists(), false, 'root')
+      t.equal(a.exists(), false, 'a')
+      t.equal(b.exists(), true, 'b')
+    })
+
+    await t.test('disable workspaces', async t => {
+      const { root, a, b } = await runCase(t, { workspacesEnabled: false })
+      t.equal(root.exists(), false, 'root')
+      t.equal(a.exists(), true, 'a')
+      t.equal(b.exists(), true, 'b')
+    })
+  })
+
   t.test('reify workspaces lockfile', async t => {
     const path = fixture(t, 'workspaces-simple')
     await reify(path)

workspaces/arborist/test/fixtures/reify-cases/workspaces-conflicting-dev-deps.js

@@ -0,0 +1,58 @@
+// generated from test/fixtures/workspaces-simple
+module.exports = t => {
+  const path = t.testdir({
+  "node_modules": {
+    ajv: {
+      'package.json': JSON.stringify({
+        name: 'ajv',
+        version: '6.10.2',
+      }),
+    }
+  },
+  "a": {
+    "package.json": JSON.stringify({
+      "name": "a",
+      "version": "1.0.0",
+      "devDependencies": {
+        "ajv": "4.11.2"
+      }
+    }),
+    "node_modules": {
+      ajv: {
+        'package.json': JSON.stringify({
+          name: 'ajv',
+          version: '4.11.2',
+        }),
+      }  
+    },
+  },
+  "b": {
+    "package.json": JSON.stringify({
+      "name": "b",
+      "version": "1.0.0",
+      "devDependencies": {
+        "ajv": "5.11.2"
+      }
+    }),
+    "node_modules": {
+      ajv: {
+        'package.json': JSON.stringify({
+          name: 'ajv',
+          version: '5.11.2',
+        }),
+      }  
+    },
+  },
+  "package.json": JSON.stringify({
+    "name": "workspace-conflicting-dev-deps",
+    "devDependencies": {
+      "ajv": "6.10.2"
+    },
+    "workspaces": [
+      "a",
+      "b"
+    ]
+  })
+})
+  return path
+}