diff --git a/workspaces/arborist/lib/arborist/build-ideal-tree.js b/workspaces/arborist/lib/arborist/build-ideal-tree.js index b8b00698c8a59..121b20246731b 100644 --- a/workspaces/arborist/lib/arborist/build-ideal-tree.js +++ b/workspaces/arborist/lib/arborist/build-ideal-tree.js @@ -97,6 +97,7 @@ module.exports = cls => class IdealTreeBuilder extends cls { #loadFailures = new Set() #manifests = new Map() #mutateTree = false + #requestedTreeMutation = false // a map of each module in a peer set to the thing that depended on // that set of peers in the first place. Use a WeakMap so that we // don't hold onto references for nodes that are garbage collected. @@ -261,12 +262,13 @@ module.exports = cls => class IdealTreeBuilder extends cls { // set if we add anything, but also set here if we know we'll make // changes and thus have to maybe prune later. - this.#mutateTree = !!( + this.#requestedTreeMutation = !!( options.add || options.rm || update.all || update.names.length ) + this.#mutateTree = this.#requestedTreeMutation } // load the initial tree, either the virtualTree from a shrinkwrap, @@ -344,7 +346,9 @@ module.exports = cls => class IdealTreeBuilder extends cls { filter: node => node, visit: node => { for (const edge of node.edgesOut.values()) { - const skipPeerOptional = edge.type === 'peerOptional' && this.options.save === false + const skipPeerOptional = edge.type === 'peerOptional' && + this.options.save === false && + !this.#requestedTreeMutation if (!skipPeerOptional && (!edge.to || !edge.valid)) { this.#depsQueue.push(node) break // no need to continue the loop after the first hit @@ -1047,9 +1051,16 @@ This is a one-time fix-up, please be patient... } const { from, valid, peerConflicted } = edgeIn if (!peerConflicted && !valid) { - if (this.#depsSeen.has(from) && this.options.save) { - // Re-queue already-processed nodes when a newly placed dep creates an invalid edge during npm install (save=true). - // This handles the case where a peerOptional dep was valid (missing) when the node was first processed, but becomes invalid when the dep is later placed by another path with a version that doesn't satisfy the peer spec. + if (this.#depsSeen.has(from) && + (this.options.save || + (this.options.save === false && this.#requestedTreeMutation))) { + // Re-queue already-processed nodes when a newly placed dep + // creates an invalid edge during npm install or another + // lockfile-mutating operation. This handles the case where a + // peerOptional dep was valid (missing) when the node was + // first processed, but becomes invalid when the dep is later + // placed by another path with a version that doesn't satisfy + // the peer spec. // See npm/cli#8726. this.#depsSeen.delete(from) this.#depsQueue.push(from) @@ -1260,11 +1271,16 @@ This is a one-time fix-up, please be patient... continue } - // If the edge has an error, there's a problem, unless it's peerOptional and we're not saving (e.g. npm ci), in which case we trust the lockfile and skip re-resolution. - // When saving (npm install), peerOptional invalid edges ARE treated as problems so the lockfile gets fixed. + // If the edge has an error, there's a problem, unless it's peerOptional + // and we're not saving or otherwise mutating (e.g. npm ci), in which + // case we trust the lockfile and skip re-resolution. When saving (npm + // install) or updating, peerOptional invalid edges ARE treated as + // problems so the lockfile gets fixed. // See npm/cli#8726. if (!edge.valid) { - if (edge.type !== 'peerOptional' || this.options.save !== false) { + if (edge.type !== 'peerOptional' || + this.options.save !== false || + this.#requestedTreeMutation) { problems.push(edge) } continue diff --git a/workspaces/arborist/test/arborist/build-ideal-tree.js b/workspaces/arborist/test/arborist/build-ideal-tree.js index f399bea2864d6..9bad026633823 100644 --- a/workspaces/arborist/test/arborist/build-ideal-tree.js +++ b/workspaces/arborist/test/arborist/build-ideal-tree.js @@ -4631,14 +4631,53 @@ t.test('re-queue already-seen nodes when placed dep invalidates peerOptional (sa t.ok(tree.children.get('shared'), 'shared is in the tree') }) -t.test('skip invalid peerOptional edges in problemEdges when save=false (#8726)', async t => { - // With save=false (npm ci behavior), invalid peerOptional edges should NOT be treated as problems. - // We use update.names to force alpha into #problemEdges while shared@1.1.0 (invalid for alpha's peerOptional spec of 1.0.0) is already in the tree from the lockfile. +t.test('re-queue already-seen nodes when save=false update invalidates peerOptional', async t => { const registry = createRegistry(t, false) - const utilPacks = registry.packuments(['1.0.0', '1.0.1'], 'util') - const utilManifest = registry.manifest({ name: 'util', packuments: utilPacks }) - await registry.package({ manifest: utilManifest }) + const alphaPack = registry.packument({ + name: 'alpha', + version: '1.0.0', + peerDependencies: { shared: '1.0.0' }, + peerDependenciesMeta: { shared: { optional: true } }, + }) + const alphaManifest = registry.manifest({ name: 'alpha', packuments: [alphaPack] }) + await registry.package({ manifest: alphaManifest }) + + const betaPack = registry.packument({ + name: 'beta', + version: '1.0.0', + dependencies: { shared: '^1.0.0' }, + }) + const betaManifest = registry.manifest({ name: 'beta', packuments: [betaPack] }) + await registry.package({ manifest: betaManifest }) + + const sharedPacks = registry.packuments(['1.0.0', '1.1.0'], 'shared') + const sharedManifest = registry.manifest({ name: 'shared', packuments: sharedPacks }) + await registry.package({ manifest: sharedManifest, times: 2 }) + + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'test-8726-save-false-update', + version: '1.0.0', + dependencies: { + alpha: '1.0.0', + beta: '1.0.0', + }, + }), + }) + + const arb = newArb(path, { save: false }) + const tree = await arb.buildIdealTree({ update: true }) + + t.ok(tree.children.get('alpha'), 'alpha is in the tree') + t.ok(tree.children.get('beta'), 'beta is in the tree') + t.ok(tree.children.get('shared'), 'shared is in the tree') +}) + +t.test('skip invalid peerOptional edges when save=false and not mutating (#8726)', async t => { + // With save=false and no tree mutation (npm ci behavior), invalid peerOptional + // edges should NOT be treated as problems. The locked tree is trusted as-is. + createRegistry(t, false) const path = t.testdir({ 'package.json': JSON.stringify({ @@ -4663,7 +4702,6 @@ t.test('skip invalid peerOptional edges in problemEdges when save=false (#8726)' 'node_modules/alpha': { version: '1.0.0', resolved: 'https://registry.npmjs.org/alpha/-/alpha-1.0.0.tgz', - dependencies: { util: '^1.0.0' }, peerDependencies: { shared: '1.0.0' }, peerDependenciesMeta: { shared: { optional: true } }, }, @@ -4676,22 +4714,178 @@ t.test('skip invalid peerOptional edges in problemEdges when save=false (#8726)' version: '1.1.0', resolved: 'https://registry.npmjs.org/shared/-/shared-1.1.0.tgz', }, - 'node_modules/util': { - version: '1.0.0', - resolved: 'https://registry.npmjs.org/util/-/util-1.0.0.tgz', - }, }, }), }) const arb = newArb(path, { save: false }) - const tree = await arb.buildIdealTree({ update: { names: ['util'] } }) + const tree = await arb.buildIdealTree() t.ok(tree.children.get('alpha'), 'alpha is in the tree') t.ok(tree.children.get('beta'), 'beta is in the tree') t.equal(tree.children.get('shared').version, '1.1.0', 'shared stays at 1.1.0 - peerOptional mismatch is not treated as a problem') - t.ok(tree.children.get('util'), 'util is in the tree') +}) + +t.test('save=false non-mutating build ignores invalid peerOptional problem edges', async t => { + const registry = createRegistry(t, false) + + const gammaPack = registry.packument({ + name: 'gamma', + version: '1.0.0', + }) + const gammaManifest = registry.manifest({ name: 'gamma', packuments: [gammaPack] }) + await registry.package({ manifest: gammaManifest }) + + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'test-8726-save-false-non-mutating-problem-edges', + version: '1.0.0', + dependencies: { + alpha: '1.0.0', + }, + }), + 'package-lock.json': JSON.stringify({ + name: 'test-8726-save-false-non-mutating-problem-edges', + version: '1.0.0', + lockfileVersion: 3, + requires: true, + packages: { + '': { + name: 'test-8726-save-false-non-mutating-problem-edges', + version: '1.0.0', + dependencies: { alpha: '1.0.0' }, + }, + 'node_modules/alpha': { + version: '1.0.0', + resolved: 'https://registry.npmjs.org/alpha/-/alpha-1.0.0.tgz', + dependencies: { gamma: '1.0.0' }, + peerDependencies: { shared: '1.0.0' }, + peerDependenciesMeta: { shared: { optional: true } }, + }, + 'node_modules/shared': { + version: '1.1.0', + resolved: 'https://registry.npmjs.org/shared/-/shared-1.1.0.tgz', + }, + }, + }), + }) + + const arb = newArb(path, { save: false }) + const tree = await arb.buildIdealTree() + + t.ok(tree.children.get('gamma'), 'missing non-peer dependency is resolved') +}) + +t.test('update does not leave invalid peerOptional edges when save=false', async t => { + // npm update defaults to save=false because it should not rewrite package.json + // ranges. It still mutates and writes the lockfile, so it cannot use the + // npm-ci behavior of trusting invalid peerOptional edges in the locked tree. + const registry = createRegistry(t, false) + + const alphaPacks = [ + registry.packument({ name: 'alpha', version: '1.0.0' }), + registry.packument({ + name: 'alpha', + version: '1.1.0', + peerDependencies: { shared: '^1.0.0' }, + peerDependenciesMeta: { shared: { optional: true } }, + }), + ] + const alphaManifest = registry.manifest({ name: 'alpha', packuments: alphaPacks }) + await registry.package({ manifest: alphaManifest }) + + const sharedPacks = registry.packuments(['1.0.0', '2.0.0', '2.0.1'], 'shared') + const sharedManifest = registry.manifest({ name: 'shared', packuments: sharedPacks }) + await registry.package({ manifest: sharedManifest, times: 2 }) + + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'test-peer-optional-update', + version: '1.0.0', + dependencies: { + alpha: '^1.0.0', + shared: '^2.0.0', + }, + }), + 'package-lock.json': JSON.stringify({ + name: 'test-peer-optional-update', + version: '1.0.0', + lockfileVersion: 3, + requires: true, + packages: { + '': { + name: 'test-peer-optional-update', + version: '1.0.0', + dependencies: { alpha: '^1.0.0', shared: '^2.0.0' }, + }, + 'node_modules/alpha': { + version: '1.0.0', + resolved: 'https://registry.npmjs.org/alpha/-/alpha-1.0.0.tgz', + }, + 'node_modules/shared': { + version: '2.0.0', + resolved: 'https://registry.npmjs.org/shared/-/shared-2.0.0.tgz', + }, + }, + }), + }) + + const arb = newArb(path, { save: false }) + await t.rejects(arb.buildIdealTree({ update: true }), { + code: 'ERESOLVE', + }, 'lockfile-mutating update should reject instead of keeping an invalid peerOptional edge') +}) + +t.test('add does not leave invalid peerOptional edges when save=false', async t => { + // npm install --no-save also mutates and writes the lockfile without + // changing package.json. It should not use npm-ci behavior either. + const registry = createRegistry(t, false) + + const alphaPack = registry.packument({ + name: 'alpha', + version: '1.0.0', + peerDependencies: { shared: '^1.0.0' }, + peerDependenciesMeta: { shared: { optional: true } }, + }) + const alphaManifest = registry.manifest({ name: 'alpha', packuments: [alphaPack] }) + await registry.package({ manifest: alphaManifest }) + + const sharedPacks = registry.packuments(['1.0.0', '2.0.0'], 'shared') + const sharedManifest = registry.manifest({ name: 'shared', packuments: sharedPacks }) + await registry.package({ manifest: sharedManifest, times: 2 }) + + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'test-peer-optional-add', + version: '1.0.0', + dependencies: { + shared: '^2.0.0', + }, + }), + 'package-lock.json': JSON.stringify({ + name: 'test-peer-optional-add', + version: '1.0.0', + lockfileVersion: 3, + requires: true, + packages: { + '': { + name: 'test-peer-optional-add', + version: '1.0.0', + dependencies: { shared: '^2.0.0' }, + }, + 'node_modules/shared': { + version: '2.0.0', + resolved: 'https://registry.npmjs.org/shared/-/shared-2.0.0.tgz', + }, + }, + }), + }) + + const arb = newArb(path, { save: false }) + await t.rejects(arb.buildIdealTree({ add: ['alpha@1.0.0'] }), { + code: 'ERESOLVE', + }, 'lockfile-mutating add should reject instead of keeping an invalid peerOptional edge') }) t.test('peerOptional prefers existing tree node over registry fetch (#9249)', async t => {