diff --git a/lib/arborist/load-actual.js b/lib/arborist/load-actual.js index d9e7fb46d..9fca7d642 100644 --- a/lib/arborist/load-actual.js +++ b/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] } diff --git a/lib/calc-dep-flags.js b/lib/calc-dep-flags.js index d6ae266db..21d8ddcf7 100644 --- a/lib/calc-dep-flags.js +++ b/lib/calc-dep-flags.js @@ -22,6 +22,11 @@ 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) { @@ -29,8 +34,7 @@ const calcDepFlagsStep = (node) => { 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) => { diff --git a/lib/node.js b/lib/node.js index 1683c7049..c21bc46cf 100644 --- a/lib/node.js +++ b/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 diff --git a/tap-snapshots/test/calc-dep-flags.js.test.cjs b/tap-snapshots/test/calc-dep-flags.js.test.cjs index 42833504f..1e7d0b743 100644 --- a/tap-snapshots/test/calc-dep-flags.js.test.cjs +++ b/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, +} +` diff --git a/test/arborist/load-actual.js b/test/arborist/load-actual.js index eebf4c063..2104bb3e4 100644 --- a/test/arborist/load-actual.js +++ b/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') +}) diff --git a/test/calc-dep-flags.js b/test/calc-dep-flags.js index ccbd74f2b..01c103a56 100644 --- a/test/calc-dep-flags.js +++ b/test/calc-dep-flags.js @@ -1,3 +1,4 @@ +const { resolve } = require('path') const t = require('tap') const calcDepFlags = require('../lib/calc-dep-flags.js') const Node = require('../lib/node.js') @@ -176,3 +177,90 @@ t.test('no reset', async t => { t.equal(root.extraneous, false, 'root.extraneous') t.equal(foo.extraneous, false, 'foo.extraneous') }) + +t.test('set parents to not extraneous when visiting', t => { + const root = new Node({ + path: '/some/path', + realpath: '/some/path', + pkg: { + dependencies: { + baz: 'file:node_modules/asdf/node_modules/baz', + foo: 'file:bar/foo', + }, + }, + }) + const bar = new Node({ + root, + path: resolve(root.path, 'bar'), + }) + const foo = new Node({ + root, + path: resolve(bar.path, 'foo'), + pkg: { name: 'foo', version: '1.2.3' }, + }) + const asdf = new Node({ + parent: root, + pkg: { name: 'asdf', version: '1.2.3' }, + }) + const baz = new Node({ + parent: asdf, + pkg: { name: 'baz', version: '1.2.3' }, + }) + const fooLink = new Link({ + name: 'foo', + target: foo, + parent: root, + realpath: foo.path, + }) + const bazLink = new Link({ + name: 'baz', + target: baz, + parent: root, + realpath: baz.path, + }) + + t.matchSnapshot(printTree(root), 'before') + calcDepFlags(root, true) + t.matchSnapshot(printTree(root), 'after') + + t.equal(root.extraneous, false, 'root') + t.equal(asdf.extraneous, false, 'asdf') + t.equal(bar.extraneous, false, 'bar') + t.equal(baz.extraneous, false, 'baz') + t.equal(foo.extraneous, false, 'foo') + t.equal(fooLink.extraneous, false, 'fooLink') + t.equal(bazLink.extraneous, false, 'bazLink') + + t.equal(root.dev, false, 'root not dev') + t.equal(asdf.dev, false, 'asdf not dev') + t.equal(bar.dev, false, 'bar not dev') + t.equal(baz.dev, false, 'baz not dev') + t.equal(foo.dev, false, 'foo not dev') + t.equal(fooLink.dev, false, 'fooLink not dev') + t.equal(bazLink.dev, false, 'bazLink not dev') + + t.equal(root.optional, false, 'root not optional') + t.equal(asdf.optional, false, 'asdf not optional') + t.equal(bar.optional, false, 'bar not optional') + t.equal(baz.optional, false, 'baz not optional') + t.equal(foo.optional, false, 'foo not optional') + t.equal(fooLink.optional, false, 'foolink not optional') + t.equal(bazLink.optional, false, 'bazlink not optional') + + t.equal(root.peer, false, 'root not peer') + t.equal(asdf.peer, false, 'asdf not peer') + t.equal(bar.peer, false, 'bar not peer') + t.equal(baz.peer, false, 'baz not peer') + t.equal(foo.peer, false, 'foo not peer') + t.equal(fooLink.peer, false, 'foolink not peer') + t.equal(bazLink.peer, false, 'bazlink not peer') + + t.equal(root.devOptional, false, 'root not devOptional') + t.equal(asdf.devOptional, false, 'asdf not devOptional') + t.equal(bar.devOptional, false, 'bar not devOptional') + t.equal(baz.devOptional, false, 'baz not devOptional') + t.equal(foo.devOptional, false, 'foo not devOptional') + t.equal(fooLink.devOptional, false, 'foolink not devOptional') + t.equal(bazLink.devOptional, false, 'bazlink not devOptional') + t.end() +}) diff --git a/test/node.js b/test/node.js index f9ec9cc08..cb6944936 100644 --- a/test/node.js +++ b/test/node.js @@ -2508,3 +2508,14 @@ t.test('packageName getter', t => { t.equal(node.packageName, 'foo') t.end() }) + +t.test('node at / should not have fsParent', t => { + const root = new Node({ path: '/some/path' }) + const link = new Link({ + parent: root, + name: 'link', + realpath: '/', + }) + t.equal(link.target.fsParent, null) + t.end() +})