fix extraneous deps on load-actual

lib/arborist/load-actual.js

@@ -22,6 +22,7 @@ const _loadFSTree = Symbol('loadFSTree')
 const _loadFSChildren = Symbol('loadFSChildren')
 const _findMissingEdges = Symbol('findMissingEdges')
 const _findFSParents = Symbol('findFSParents')
+const _resetDepFlags = Symbol('resetDepFlags')
 
 const _actualTreeLoaded = Symbol('actualTreeLoaded')
 const _rpcache = Symbol.for('realpathCache')
@@ -74,6 +75,19 @@ module.exports = cls => class ActualLoader extends cls {
     this[_topNodes] = new Set()
   }
 
+  [_resetDepFlags] (tree, root) {
+    // reset all deps to extraneous prior to recalc
+    if (!root) {
+      for (const node of tree.inventory.values())
+        node.extraneous = true
+    }
+
+    // only reset root flags if we're not re-rooting,
+    // otherwise leave as-is
+    calcDepFlags(tree, !root)
+    return tree
+  }
+
   // public method
   async loadActual (options = {}) {
     // allow the user to set options on the ctor as well.
@@ -88,6 +102,7 @@ module.exports = cls => class ActualLoader extends cls {
     return this.actualTree ? this.actualTree
       : this[_actualTreePromise] ? this[_actualTreePromise]
       : this[_actualTreePromise] = this[_loadActual](options)
+        .then(tree => this[_resetDepFlags](tree, options.root))
         .then(tree => this.actualTree = treeCheck(tree))
   }
 
@@ -152,8 +167,7 @@ module.exports = cls => class ActualLoader extends cls {
       root: this[_actualTree],
     })
     await this[_loadWorkspaces](this[_actualTree])
-    if (this[_actualTree].workspaces && this[_actualTree].workspaces.size)
-      calcDepFlags(this[_actualTree], !root)
+
     this[_transplant](root)
     return this[_actualTree]
   }
@@ -178,8 +192,6 @@ module.exports = cls => class ActualLoader extends cls {
         dependencies[name] = dependencies[name] || '*'
       actualRoot.package = { ...actualRoot.package, dependencies }
     }
-    // only reset root flags if we're not re-rooting, otherwise leave as-is
-    calcDepFlags(this[_actualTree], !root)
     return this[_actualTree]
   }
 

lib/calc-dep-flags.js

@@ -22,15 +22,19 @@ const calcDepFlagsStep = (node) => {
   // Since we're only walking through deps that are not already flagged
   // as non-dev/non-optional, it's typically a very shallow traversal
   node.extraneous = false
+  resetParents(node, 'extraneous')
+  resetParents(node, 'dev')
+  resetParents(node, 'peer')
+  resetParents(node, 'devOptional')
+  resetParents(node, 'optional')
 
   // for links, map their hierarchy appropriately
   if (node.target) {
     node.target.dev = node.dev
     node.target.optional = node.optional
     node.target.devOptional = node.devOptional
     node.target.peer = node.peer
-    node.target.extraneous = false
-    node = node.target
+    return calcDepFlagsStep(node.target)
   }
 
   node.edgesOut.forEach(({peer, optional, dev, to}) => {
@@ -71,6 +75,14 @@ const calcDepFlagsStep = (node) => {
   return node
 }
 
+const resetParents = (node, flag) => {
+  if (node[flag])
+    return
+
+  for (let p = node; p && (p === node || p[flag]); p = p.resolveParent)
+    p[flag] = false
+}
+
 // typically a short walk, since it only traverses deps that
 // have the flag set.
 const unsetFlag = (node, flag) => {

lib/node.js

@@ -547,6 +547,8 @@ class Node {
 
       // try to find our parent/fsParent in the new root inventory
       for (const p of walkUp(dirname(this.path))) {
+        if (p === this.path)
+          continue
         const ploc = relpath(root.realpath, p)
         const parent = root.inventory.get(ploc)
         if (parent) {
@@ -783,7 +785,13 @@ class Node {
   }
 
   get fsParent () {
-    return this[_fsParent]
+    const parent = this[_fsParent]
+    /* istanbul ignore next - should be impossible */
+    debug(() => {
+      if (parent === this)
+        throw new Error('node set to its own fsParent')
+    })
+    return parent
   }
 
   set fsParent (fsParent) {
@@ -1009,7 +1017,13 @@ class Node {
   }
 
   get parent () {
-    return this[_parent]
+    const parent = this[_parent]
+    /* istanbul ignore next - should be impossible */
+    debug(() => {
+      if (parent === this)
+        throw new Error('node set to its own parent')
+    })
+    return parent
   }
 
   // This setter keeps everything in order when we move a node from

tap-snapshots/test/calc-dep-flags.js.test.cjs

@@ -421,3 +421,213 @@ ArboristNode {
   "peer": true,
 }
 `
+
+exports[`test/calc-dep-flags.js TAP set parents to not extraneous when visiting > after 1`] = `
+ArboristNode {
+  "children": Map {
+    "asdf" => ArboristNode {
+      "children": Map {
+        "baz" => ArboristNode {
+          "location": "node_modules/asdf/node_modules/baz",
+          "name": "baz",
+          "path": "/some/path/node_modules/asdf/node_modules/baz",
+          "version": "1.2.3",
+        },
+      },
+      "location": "node_modules/asdf",
+      "name": "asdf",
+      "path": "/some/path/node_modules/asdf",
+      "version": "1.2.3",
+    },
+    "baz" => ArboristLink {
+      "edgesIn": Set {
+        EdgeIn {
+          "from": "",
+          "name": "baz",
+          "spec": "file:node_modules/asdf/node_modules/baz",
+          "type": "prod",
+        },
+      },
+      "location": "node_modules/baz",
+      "name": "baz",
+      "path": "/some/path/node_modules/baz",
+      "realpath": "/some/path/node_modules/asdf/node_modules/baz",
+      "resolved": "file:asdf/node_modules/baz",
+      "target": ArboristNode {
+        "location": "node_modules/asdf/node_modules/baz",
+      },
+      "version": "1.2.3",
+    },
+    "foo" => ArboristLink {
+      "edgesIn": Set {
+        EdgeIn {
+          "from": "",
+          "name": "foo",
+          "spec": "file:bar/foo",
+          "type": "prod",
+        },
+      },
+      "location": "node_modules/foo",
+      "name": "foo",
+      "path": "/some/path/node_modules/foo",
+      "realpath": "/some/path/bar/foo",
+      "resolved": "file:../bar/foo",
+      "target": ArboristNode {
+        "location": "bar/foo",
+      },
+      "version": "1.2.3",
+    },
+  },
+  "edgesOut": Map {
+    "baz" => EdgeOut {
+      "name": "baz",
+      "spec": "file:node_modules/asdf/node_modules/baz",
+      "to": "node_modules/baz",
+      "type": "prod",
+    },
+    "foo" => EdgeOut {
+      "name": "foo",
+      "spec": "file:bar/foo",
+      "to": "node_modules/foo",
+      "type": "prod",
+    },
+  },
+  "fsChildren": Set {
+    ArboristNode {
+      "fsChildren": Set {
+        ArboristNode {
+          "location": "bar/foo",
+          "name": "foo",
+          "path": "/some/path/bar/foo",
+          "version": "1.2.3",
+        },
+      },
+      "location": "bar",
+      "name": "bar",
+      "path": "/some/path/bar",
+    },
+  },
+  "location": "",
+  "name": "path",
+  "path": "/some/path",
+}
+`
+
+exports[`test/calc-dep-flags.js TAP set parents to not extraneous when visiting > before 1`] = `
+ArboristNode {
+  "children": Map {
+    "asdf" => ArboristNode {
+      "children": Map {
+        "baz" => ArboristNode {
+          "dev": true,
+          "extraneous": true,
+          "location": "node_modules/asdf/node_modules/baz",
+          "name": "baz",
+          "optional": true,
+          "path": "/some/path/node_modules/asdf/node_modules/baz",
+          "peer": true,
+          "version": "1.2.3",
+        },
+      },
+      "dev": true,
+      "extraneous": true,
+      "location": "node_modules/asdf",
+      "name": "asdf",
+      "optional": true,
+      "path": "/some/path/node_modules/asdf",
+      "peer": true,
+      "version": "1.2.3",
+    },
+    "baz" => ArboristLink {
+      "dev": true,
+      "edgesIn": Set {
+        EdgeIn {
+          "from": "",
+          "name": "baz",
+          "spec": "file:node_modules/asdf/node_modules/baz",
+          "type": "prod",
+        },
+      },
+      "extraneous": true,
+      "location": "node_modules/baz",
+      "name": "baz",
+      "optional": true,
+      "path": "/some/path/node_modules/baz",
+      "peer": true,
+      "realpath": "/some/path/node_modules/asdf/node_modules/baz",
+      "resolved": "file:asdf/node_modules/baz",
+      "target": ArboristNode {
+        "location": "node_modules/asdf/node_modules/baz",
+      },
+      "version": "1.2.3",
+    },
+    "foo" => ArboristLink {
+      "dev": true,
+      "edgesIn": Set {
+        EdgeIn {
+          "from": "",
+          "name": "foo",
+          "spec": "file:bar/foo",
+          "type": "prod",
+        },
+      },
+      "extraneous": true,
+      "location": "node_modules/foo",
+      "name": "foo",
+      "optional": true,
+      "path": "/some/path/node_modules/foo",
+      "peer": true,
+      "realpath": "/some/path/bar/foo",
+      "resolved": "file:../bar/foo",
+      "target": ArboristNode {
+        "location": "bar/foo",
+      },
+      "version": "1.2.3",
+    },
+  },
+  "dev": true,
+  "edgesOut": Map {
+    "baz" => EdgeOut {
+      "name": "baz",
+      "spec": "file:node_modules/asdf/node_modules/baz",
+      "to": "node_modules/baz",
+      "type": "prod",
+    },
+    "foo" => EdgeOut {
+      "name": "foo",
+      "spec": "file:bar/foo",
+      "to": "node_modules/foo",
+      "type": "prod",
+    },
+  },
+  "extraneous": true,
+  "fsChildren": Set {
+    ArboristNode {
+      "dev": true,
+      "extraneous": true,
+      "fsChildren": Set {
+        ArboristNode {
+          "dev": true,
+          "extraneous": true,
+          "location": "bar/foo",
+          "name": "foo",
+          "optional": true,
+          "path": "/some/path/bar/foo",
+          "peer": true,
+          "version": "1.2.3",
+        },
+      },
+      "location": "bar",
+      "name": "bar",
+      "optional": true,
+      "path": "/some/path/bar",
+      "peer": true,
+    },
+  },
+  "location": "",
+  "name": "path",
+  "optional": true,
+  "path": "/some/path",
+  "peer": true,
+}
+`

test/arborist/load-actual.js

@@ -358,3 +358,35 @@ t.test('load workspaces when loading from hidding lockfile', async t => {
   t.equal(aTarget.version, '1.2.3', 'updated a target version')
   t.matchSnapshot(tree, 'actual tree')
 })
+
+t.test('recalc dep flags for virtual load actual', async t => {
+  const path = t.testdir({
+    node_modules: {
+      abbrev: {
+        'package.json': JSON.stringify({
+          name: 'abbrev',
+          version: '1.1.1',
+        }),
+      },
+      '.package-lock.json': JSON.stringify({
+        lockfileVersion: 2,
+        requires: true,
+        packages: {
+          'node_modules/abbrev': {
+            version: '1.1.1',
+            resolved: 'https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz',
+            integrity: 'sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q==',
+          },
+        },
+      }),
+    },
+    'package.json': JSON.stringify({}),
+  })
+
+  const hidden = resolve(path, 'node_modules/.package-lock.json')
+  const then = Date.now() + 10000
+  fs.utimesSync(hidden, new Date(then), new Date(then))
+  const tree = await loadActual(path)
+  const abbrev = tree.children.get('abbrev')
+  t.equal(abbrev.extraneous, true, 'abbrev is extraneous')
+})