Skip to content

Commit

Permalink
fix extraneous deps on load-actual
Browse files Browse the repository at this point in the history 
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: npm/cli#2554

PR-URL: npm#289
Credit: @ruyadorno
Close: npm#289
Reviewed-by: @isaacs
  • Loading branch information
@ruyadorno @isaacs
ruyadorno authored and isaacs committed Jun 16, 2021
1 parent 590fa18 commit 271dd5f
Show file tree
Hide file tree
Showing 5 changed files with 360 additions and 6 deletions.
20 changes: 16 additions & 4 deletions lib/arborist/load-actual.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -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.
Expand All @@ -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))
}

Expand Down Expand Up @@ -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]
}
Expand All @@ -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]
}

Expand Down
16 changes: 14 additions & 2 deletions lib/calc-dep-flags.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}) => {
Expand Down Expand Up @@ -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) => {
Expand Down
210 changes: 210 additions & 0 deletions tap-snapshots/test/calc-dep-flags.js.test.cjs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
`
32 changes: 32 additions & 0 deletions test/arborist/load-actual.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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')
})
Loading

0 comments on commit 271dd5f

Please sign in to comment.