From b4a2a4ec4fef381b103a29f6379a281a8a88b222 Mon Sep 17 00:00:00 2001 From: Ruy Adorno Date: Thu, 10 Jun 2021 17:27:58 -0400 Subject: [PATCH] fix extraneous deps on load-actual When loading an actual tree sometimes extraneous deps were not being properly marked as so, since they were not really marked as extraneous at the moment of reify, so in order to be strictly correct we need to recalculate deps at load-actual in order to make sure extraneous deps are properly marked as such. Related to: https://github.com/npm/cli/issues/2554 --- lib/arborist/load-actual.js | 20 +- lib/calc-dep-flags.js | 16 +- tap-snapshots/test/calc-dep-flags.js.test.cjs | 210 ++++++++++++++++++ test/arborist/load-actual.js | 32 +++ test/calc-dep-flags.js | 88 ++++++++ 5 files changed, 360 insertions(+), 6 deletions(-) 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/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() +})