From ce38200437e9ed527df973794909b2699909bc9b Mon Sep 17 00:00:00 2001 From: nlf Date: Thu, 29 Apr 2021 12:40:09 -0700 Subject: [PATCH] @npmcli/arborist@2.4.1 --- .../arborist/lib/arborist/build-ideal-tree.js | 200 ++++++++++++++---- node_modules/@npmcli/arborist/lib/node.js | 38 ++++ .../@npmcli/arborist/lib/printable.js | 9 + node_modules/@npmcli/arborist/package.json | 2 +- package-lock.json | 14 +- package.json | 2 +- 6 files changed, 213 insertions(+), 52 deletions(-) diff --git a/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js b/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js index 293691563ee95..7ee8dae35be1b 100644 --- a/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js +++ b/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js @@ -47,6 +47,7 @@ const _flagsSuspect = Symbol.for('flagsSuspect') const _workspaces = Symbol.for('workspaces') const _prune = Symbol('prune') const _preferDedupe = Symbol('preferDedupe') +const _pruneDedupable = Symbol('pruneDedupable') const _legacyBundling = Symbol('legacyBundling') const _parseSettings = Symbol('parseSettings') const _initTree = Symbol('initTree') @@ -1342,6 +1343,21 @@ This is a one-time fix-up, please be patient... // this is an overridden peer dep this[_warnPeerConflict](edge) } + + // if we get a KEEP in a update scenario, then we MAY have something + // already duplicating this unnecessarily! For example: + // ``` + // root + // +-- x (dep: y@1.x) + // | +-- y@1.0.0 + // +-- y@1.1.0 + // ``` + // Now say we do `reify({update:['y']})`, and the latest version is + // 1.1.0, which we already have in the root. We'll try to place y@1.1.0 + // first in x, then in the root, ending with KEEP, because we already + // have it. In that case, we ought to REMOVE the nm/x/nm/y node, because + // it is an unnecessary duplicate. + this[_pruneDedupable](target, true) return [] } @@ -1397,8 +1413,8 @@ This is a one-time fix-up, please be patient... // MAY end up putting a better/identical node further up the tree in // a way that causes an unnecessary duplication. If so, remove the // now-unnecessary node. - if (edge.valid && edge.to.parent !== target && newDep.canReplace(edge.to)) - edge.to.parent = null + if (edge.valid && edge.to && edge.to !== newDep) + this[_pruneDedupable](edge.to, false) // visit any dependents who are upset by this change // if it's an angry overridden peer edge, however, make sure we @@ -1414,30 +1430,8 @@ This is a one-time fix-up, please be patient... // prune anything deeper in the tree that can be replaced by this if (this.idealTree) { for (const node of this.idealTree.inventory.query('name', newDep.name)) { - if (node !== newDep && - node.isDescendantOf(target) && - !node.inShrinkwrap && - !node.inBundle && - node.canReplaceWith(newDep)) { - // don't prune if the dupe is necessary! - // root (a, d) - // +-- a (b, c2) - // | +-- b (c2) <-- place c2 for b, lands at root - // +-- d (e) - // +-- e (c1, d) - // +-- c1 - // +-- f (c2) - // +-- c2 <-- pruning this would be bad - - const mask = node.parent !== target && - node.parent && - node.parent.parent && - node.parent.parent !== target && - node.parent.parent.resolve(newDep.name) - - if (!mask || mask === newDep || node.canReplaceWith(mask)) - node.parent = null - } + if (node.isDescendantOf(target)) + this[_pruneDedupable](node, false) } } @@ -1470,6 +1464,21 @@ This is a one-time fix-up, please be patient... return placed } + // prune all the nodes in a branch of the tree that can be safely removed + // This is only the most basic duplication detection; it finds if there + // is another satisfying node further up the tree, and if so, dedupes. + // Even in legacyBundling mode, we do this amount of deduplication. + [_pruneDedupable] (node, descend = true) { + if (node.canDedupe(this[_preferDedupe])) { + node.root = null + return + } + if (descend) { + for (const child of node.children.values()) + this[_pruneDedupable](child) + } + } + [_pruneForReplacement] (node, oldDeps) { // gather up all the invalid edgesOut, and any now-extraneous // deps that the new node doesn't depend on but the old one did. @@ -1622,32 +1631,137 @@ This is a one-time fix-up, please be patient... // placed here as well. the virtualRoot already has the appropriate // overrides applied. if (peerEntryEdge) { - const peerSet = getPeerSet(current) - OUTER: for (const p of peerSet) { - // if any have a non-peer dep from the target, or a peer dep if - // the target is root, then cannot safely replace and dupe deeper. + const currentPeerSet = getPeerSet(current) + + // We are effectively replacing currentPeerSet with newPeerSet + // If there are any non-peer deps coming into the currentPeerSet, + // which are currently valid, and are from the target, then that + // means that we have to ensure that they're not going to be made + // invalid by putting the newPeerSet in place. + // If the edge comes from somewhere deeper than the target, then + // that's fine, because we'll create an invalid edge, detect it, + // and duplicate the node further into the tree. + // loop through the currentPeerSet checking for valid edges on + // the members of the peer set which will be made invalid. + const targetEdges = new Set() + for (const p of currentPeerSet) { for (const edge of p.edgesIn) { - if (peerSet.has(edge.from)) + // edge from within the peerSet, ignore + if (currentPeerSet.has(edge.from)) + continue + // only care about valid edges from target. + // edges from elsewhere can dupe if offended, invalid edges + // are already being fixed or will be later. + if (edge.from !== target || !edge.valid) continue + targetEdges.add(edge) + } + } - // only respect valid edges, however, since we're likely trying - // to fix the very one that's currently broken! If the virtual - // root's replacement is ok, and doesn't have any invalid edges - // indicating that it was an overridden peer, then ignore the - // conflict and continue. If it WAS an override, then we need - // to get the conflict here so that we can decide whether to - // accept the current dep node, clobber it, or fail the install. - if (edge.from === target && edge.valid) { - const rep = dep.parent.children.get(edge.name) - const override = rep && ([...rep.edgesIn].some(e => !e.valid)) - if (!rep || !rep.satisfies(edge) || override) { + for (const edge of targetEdges) { + // see if we intend to replace this one anyway + const rep = dep.parent.children.get(edge.name) + const current = edge.to + if (!rep) { + // this isn't one we're replacing. but it WAS included in the + // peerSet for some reason, so make sure that it's still + // ok with the replacements in the new peerSet + for (const curEdge of current.edgesOut.values()) { + const newRepDep = dep.parent.children.get(curEdge.name) + if (curEdge.valid && newRepDep && !newRepDep.satisfies(curEdge)) { canReplace = false - break OUTER + break + } + } + continue + } + + // was this replacement already an override of some sort? + const override = [...rep.edgesIn].some(e => !e.valid) + // if we have a rep, and it's ok to put in this location, and + // it's not already part of an override in the peerSet, then + // we can continue with it. + if (rep.satisfies(edge) && !override) + continue + // Otherwise, we cannot replace. + canReplace = false + break + } + // if we're going to be replacing the peerSet, we have to remove + // and re-resolve any members of the old peerSet that are not + // present in the new one, and which will have invalid edges. + // We know that they're not depended upon by the target, or else + // they would have caused a conflict, so they'll get landed deeper + // in the tree, if possible. + if (canReplace) { + let needNesting = false + OUTER: for (const node of currentPeerSet) { + const rep = dep.parent.children.get(node.name) + // has a replacement, already addressed above + if (rep) + continue + + // ok, it has been placed here to dedupe, see if it needs to go + // back deeper within the tree. + for (const edge of node.edgesOut.values()) { + const repDep = dep.parent.children.get(edge.name) + // not in new peerSet, maybe fine. + if (!repDep) + continue + + // new thing will be fine, no worries + if (repDep.satisfies(edge)) + continue + + // uhoh, we'll have to nest them. + needNesting = true + break OUTER + } + } + + // to nest, just delete everything without a target dep + // that's in the current peerSet, and add their dependants + // to the _depsQueue for evaluation. Some of these MAY end + // up in the same location again, and that's fine. + if (needNesting) { + // avoid mutating the tree while we're examining it + const dependants = new Set() + const reresolve = new Set() + OUTER: for (const node of currentPeerSet) { + const rep = dep.parent.children.get(node.name) + if (rep) + continue + // create a separate set for each one, so we can skip any + // that might somehow have an incoming target edge + const deps = new Set() + for (const edge of node.edgesIn) { + // a target dep, skip this dep entirely, already addressed + // ignoring for coverage, because it really ought to be + // impossible, but I can't prove it yet, so this is here + // for safety. + /* istanbul ignore if - should be impossible */ + if (edge.from === target) + continue OUTER + // ignore this edge, it'll either be replaced or re-resolved + if (currentPeerSet.has(edge.from)) + continue + // ok, we care about this one. + deps.add(edge.from) } + reresolve.add(node) + for (const d of deps) + dependants.add(d) } + for (const dependant of dependants) { + this[_depsQueue].push(dependant) + this[_depsSeen].delete(dependant) + } + for (const node of reresolve) + node.root = null } } } + if (canReplace) { const ret = this[_canPlacePeers](dep, target, edge, REPLACE, peerEntryEdge, peerPath, isSource) /* istanbul ignore else - extremely rare that the peer set would diff --git a/node_modules/@npmcli/arborist/lib/node.js b/node_modules/@npmcli/arborist/lib/node.js index 197804e0ce0e3..a54f76afcdf3b 100644 --- a/node_modules/@npmcli/arborist/lib/node.js +++ b/node_modules/@npmcli/arborist/lib/node.js @@ -28,6 +28,7 @@ // where we need to quickly find all instances of a given package name within a // tree. +const semver = require('semver') const nameFromFolder = require('@npmcli/name-from-folder') const Edge = require('./edge.js') const Inventory = require('./inventory.js') @@ -885,6 +886,43 @@ class Node { return node.canReplaceWith(this) } + // return true if it's safe to remove this node, because anything that + // is depending on it would be fine with the thing that they would resolve + // to if it was removed, or nothing is depending on it in the first place. + canDedupe (preferDedupe = false) { + // not allowed to mess with shrinkwraps or bundles + if (this.inDepBundle || this.inShrinkwrap) + return false + + // it's a top level pkg, or a dep of one + if (!this.parent || !this.parent.parent) + return false + + // no one wants it, remove it + if (this.edgesIn.size === 0) + return true + + const other = this.parent.parent.resolve(this.name) + + // nothing else, need this one + if (!other) + return false + + // if it's the same thing, then always fine to remove + if (other.matches(this)) + return true + + // if the other thing can't replace this, then skip it + if (!other.canReplace(this)) + return false + + // if we prefer dedupe, or if the version is greater/equal, take the other + if (preferDedupe || semver.gte(other.version, this.version)) + return true + + return false + } + satisfies (requested) { if (requested instanceof Edge) return this.name === requested.name && requested.satisfiedBy(this) diff --git a/node_modules/@npmcli/arborist/lib/printable.js b/node_modules/@npmcli/arborist/lib/printable.js index 169984fcf17ea..79f46a9e93c4a 100644 --- a/node_modules/@npmcli/arborist/lib/printable.js +++ b/node_modules/@npmcli/arborist/lib/printable.js @@ -29,6 +29,15 @@ class ArboristNode { this.peer = true if (tree.inBundle) this.bundled = true + if (tree.inDepBundle) + this.bundler = tree.getBundler().location + const bd = tree.package && tree.package.bundleDependencies + if (bd && bd.length) + this.bundleDependencies = bd + if (tree.inShrinkwrap) + this.inShrinkwrap = true + else if (tree.hasShrinkwrap) + this.hasShrinkwrap = true if (tree.error) this.error = treeError(tree.error) if (tree.errors && tree.errors.length) diff --git a/node_modules/@npmcli/arborist/package.json b/node_modules/@npmcli/arborist/package.json index 66a92f0d52d34..e7ac932e08d8f 100644 --- a/node_modules/@npmcli/arborist/package.json +++ b/node_modules/@npmcli/arborist/package.json @@ -1,6 +1,6 @@ { "name": "@npmcli/arborist", - "version": "2.4.0", + "version": "2.4.1", "description": "Manage node_modules trees", "dependencies": { "@npmcli/installed-package-contents": "^1.0.7", diff --git a/package-lock.json b/package-lock.json index fc2aefb93de95..bded82f20879e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -78,7 +78,7 @@ ], "license": "Artistic-2.0", "dependencies": { - "@npmcli/arborist": "^2.4.0", + "@npmcli/arborist": "^2.4.1", "@npmcli/ci-detect": "^1.2.0", "@npmcli/config": "^2.2.0", "@npmcli/run-script": "^1.8.5", @@ -712,9 +712,9 @@ } }, "node_modules/@npmcli/arborist": { - "version": "2.4.0", - "resolved": "https://registry.npmjs.org/@npmcli/arborist/-/arborist-2.4.0.tgz", - "integrity": "sha512-rCoRrUSmXdBDBBgL/O0oehIR53ey99Pds8dId7gztARZmx6/NBoeiUOu9RnvXSe15XZLc3JSz9sHPcbQ9NQ53Q==", + "version": "2.4.1", + "resolved": "https://registry.npmjs.org/@npmcli/arborist/-/arborist-2.4.1.tgz", + "integrity": "sha512-LivXfK+LjtvzFjnwK6E41Pkw1C8+MYrgdXinzqpDc8MDYp7gMT0nvGvnpQd47OV2GhLRyBkbUSEcLk6P1d1s0g==", "inBundle": true, "dependencies": { "@npmcli/installed-package-contents": "^1.0.7", @@ -10826,9 +10826,9 @@ "dev": true }, "@npmcli/arborist": { - "version": "2.4.0", - "resolved": "https://registry.npmjs.org/@npmcli/arborist/-/arborist-2.4.0.tgz", - "integrity": "sha512-rCoRrUSmXdBDBBgL/O0oehIR53ey99Pds8dId7gztARZmx6/NBoeiUOu9RnvXSe15XZLc3JSz9sHPcbQ9NQ53Q==", + "version": "2.4.1", + "resolved": "https://registry.npmjs.org/@npmcli/arborist/-/arborist-2.4.1.tgz", + "integrity": "sha512-LivXfK+LjtvzFjnwK6E41Pkw1C8+MYrgdXinzqpDc8MDYp7gMT0nvGvnpQd47OV2GhLRyBkbUSEcLk6P1d1s0g==", "requires": { "@npmcli/installed-package-contents": "^1.0.7", "@npmcli/map-workspaces": "^1.0.2", diff --git a/package.json b/package.json index 04987c36e0b2f..975f89e30a20e 100644 --- a/package.json +++ b/package.json @@ -42,7 +42,7 @@ "./package.json": "./package.json" }, "dependencies": { - "@npmcli/arborist": "^2.4.0", + "@npmcli/arborist": "^2.4.1", "@npmcli/ci-detect": "^1.2.0", "@npmcli/config": "^2.2.0", "@npmcli/run-script": "^1.8.5",