Merge pull request #350 from npm/fritzy/better-workspace-filter

fix: ws no longer filter root deps

lib/arborist/index.js

@@ -29,6 +29,7 @@
 const { resolve } = require('path')
 const { homedir } = require('os')
 const procLog = require('proc-log')
+const { depth } = require('treeverse')
 const { saveTypeMap } = require('../add-rm-pkg-deps.js')
 
 const mixins = [
@@ -88,6 +89,9 @@ class Arborist extends Base {
     process.emit('timeEnd', 'arborist:ctor')
   }
 
+  // TODO: We should change these to static functions instead
+  //   of methods for the next major version
+
   // returns an array of the actual nodes for all the workspaces
   workspaceNodes (tree, workspaces) {
     return getWorkspaceNodes(tree, workspaces, this.log)
@@ -103,15 +107,15 @@ class Arborist extends Base {
         }
       }
     }
-    const set = new Set(wsNodes)
+    const wsDepSet = new Set(wsNodes)
     const extraneous = new Set()
-    for (const node of set) {
+    for (const node of wsDepSet) {
       for (const edge of node.edgesOut.values()) {
         const dep = edge.to
         if (dep) {
-          set.add(dep)
+          wsDepSet.add(dep)
           if (dep.isLink) {
-            set.add(dep.target)
+            wsDepSet.add(dep.target)
           }
         }
       }
@@ -122,28 +126,36 @@ class Arborist extends Base {
       }
     }
     for (const extra of extraneous) {
-      set.add(extra)
+      wsDepSet.add(extra)
     }
 
-    return set
+    return wsDepSet
   }
 
+  // returns a set of root dependencies, excluding depdencies that are
+  // exclusively workspace dependencies
   excludeWorkspacesDependencySet (tree) {
-    const set = new Set()
-    for (const edge of tree.edgesOut.values()) {
-      if (edge.type !== 'workspace' && edge.to) {
-        set.add(edge.to)
-      }
-    }
-    for (const node of set) {
-      for (const edge of node.edgesOut.values()) {
-        if (edge.to) {
-          set.add(edge.to)
+    const rootDepSet = new Set()
+    depth({
+      tree,
+      visit: node => {
+        for (const { to } of node.edgesOut.values()) {
+          if (!to || to.isWorkspace) {
+            continue
+          }
+          for (const edgeIn of to.edgesIn.values()) {
+            if (edgeIn.from.isRoot || rootDepSet.has(edgeIn.from)) {
+              rootDepSet.add(to)
+            }
+          }
         }
-      }
-    }
-
-    return set
+        return node
+      },
+      filter: node => node,
+      getChildren: (node, tree) =>
+        [...tree.edgesOut.values()].map(edge => edge.to),
+    })
+    return rootDepSet
   }
 }
 

tap-snapshots/test/arborist/pruner.js.test.cjs

@@ -180,6 +180,26 @@ ArboristNode {
       "path": "{CWD}/test/arborist/tap-testdir-pruner-prune-workspaces/node_modules/b",
       "version": "1.2.3",
     },
+    "derp" => ArboristNode {
+      "edgesIn": Set {
+        EdgeIn {
+          "from": "",
+          "name": "derp",
+          "spec": "*",
+          "type": "prod",
+        },
+        EdgeIn {
+          "from": "node_modules/once",
+          "name": "derp",
+          "spec": "*",
+          "type": "prod",
+        },
+      },
+      "location": "node_modules/derp",
+      "name": "derp",
+      "path": "{CWD}/test/arborist/tap-testdir-pruner-prune-workspaces/node_modules/derp",
+      "version": "90.2.11",
+    },
     "once" => ArboristNode {
       "edgesIn": Set {
         EdgeIn {
@@ -190,6 +210,12 @@ ArboristNode {
         },
       },
       "edgesOut": Map {
+        "derp" => EdgeOut {
+          "name": "derp",
+          "spec": "*",
+          "to": "node_modules/derp",
+          "type": "prod",
+        },
         "wrappy" => EdgeOut {
           "name": "wrappy",
           "spec": "*",
@@ -246,6 +272,12 @@ ArboristNode {
       "to": "node_modules/b",
       "type": "workspace",
     },
+    "derp" => EdgeOut {
+      "name": "derp",
+      "spec": "*",
+      "to": "node_modules/derp",
+      "type": "prod",
+    },
     "qs" => EdgeOut {
       "name": "qs",
       "spec": "*",

test/arborist/index.js

@@ -131,6 +131,7 @@ t.test('excludeSet includes nonworkspace metadeps', async t => {
       workspaces: ['pkgs/*'],
       dependencies: {
         foo: '',
+        fritzy: '',
       },
     },
     children: [
@@ -215,9 +216,11 @@ t.test('excludeSet includes nonworkspace metadeps', async t => {
   const arb = new Arborist()
   const filter = arb.excludeWorkspacesDependencySet(tree)
 
-  t.equal(filter.size, 2)
+  t.equal(filter.size, 3)
   t.equal(filter.has(tree.children.get('foo')), true)
   t.equal(filter.has(tree.children.get('bar')), true)
+  t.equal(filter.has(tree.children.get('baz')), false)
+  t.equal(filter.has(tree.children.get('fritzy')), true)
 })
 
 t.test('lockfileVersion config validation', async t => {

test/arborist/pruner.js

@@ -124,6 +124,7 @@ t.test('prune workspaces', async t => {
       main: 'index.js',
       dependencies: {
         qs: '',
+        derp: '',
       },
       scripts: {
         test: 'echo "Error: no test specified" && exit 1',
@@ -171,6 +172,7 @@ t.test('prune workspaces', async t => {
           version: '1.2.3',
           dependencies: {
             wrappy: '',
+            derp: '',
           },
         }),
       },
@@ -186,6 +188,12 @@ t.test('prune workspaces', async t => {
           version: '1.2.3',
         }),
       },
+      derp: {
+        'package.json': JSON.stringify({
+          name: 'derp',
+          version: '90.2.11',
+        }),
+      },
     },
   })
   const tree = await pruneTree(path, { workspacesEnabled: false })
@@ -194,5 +202,6 @@ t.test('prune workspaces', async t => {
   t.notOk(fs.existsSync(join(path, 'node_modules', 'wrappy')), 'wrappy was pruned from tree')
   t.notOk(fs.existsSync(join(path, 'node_modules', 'a')), 'a was pruned from tree')
   t.notOk(fs.existsSync(join(path, 'node_modules', 'b')), 'b was pruned from tree')
+  t.ok(fs.existsSync(join(path, 'node_modules', 'derp')), 'derp was not pruned from tree')
   t.matchSnapshot(printTree(tree))
 })