diff --git a/docs/lib/content/configuring-npm/package-json.md b/docs/lib/content/configuring-npm/package-json.md index abb5e362c61e3..c077129719657 100644 --- a/docs/lib/content/configuring-npm/package-json.md +++ b/docs/lib/content/configuring-npm/package-json.md @@ -842,6 +842,16 @@ This is a map of package name to version or URL, just like the `dependencies` ob The difference is that build failures do not cause installation to fail. Running `npm install --omit=optional` will prevent these dependencies from being installed. +For example: + +```json +{ + "optionalDependencies": { + "@npm/foo": "^1.0.0" + } +} +``` + It is still your program's responsibility to handle the lack of the dependency. For example, something like this: diff --git a/workspaces/arborist/lib/arborist/build-ideal-tree.js b/workspaces/arborist/lib/arborist/build-ideal-tree.js index c9d9c308cbc9c..7fecd6759c041 100644 --- a/workspaces/arborist/lib/arborist/build-ideal-tree.js +++ b/workspaces/arborist/lib/arborist/build-ideal-tree.js @@ -1351,7 +1351,22 @@ This is a one-time fix-up, please be patient... // doesn't satisfy the edge. try to fetch a manifest and build a node from that. return this.#fetchManifest(spec, parent, edge) .then( - pkg => new Node({ name, pkg, parent, installLinks, legacyPeerDeps }), + pkg => { + // When a proxy/upstream registry returns an incomplete manifest + // (e.g. missing version field for platform-specific packages it + // hasn't cached), treat it as a load failure so that optional deps + // are properly pruned instead of written to the lockfile without + // version metadata. Only apply to registry specs — file: deps + // legitimately omit version. + if (!pkg.version && spec.registry) { + const error = Object.assign( + new Error(`incomplete manifest for ${name}, missing version`), + { code: 'EINCOMPLETEMANIFEST' } + ) + return this.#failureNode(name, parent, error, edge) + } + return new Node({ name, pkg, parent, installLinks, legacyPeerDeps }) + }, error => this.#failureNode(name, parent, error, edge) ) } diff --git a/workspaces/arborist/lib/link.js b/workspaces/arborist/lib/link.js index d200f3d8f8d78..3824dc81ffb3c 100644 --- a/workspaces/arborist/lib/link.js +++ b/workspaces/arborist/lib/link.js @@ -109,12 +109,24 @@ class Link extends Node { // so this is a no-op [_loadDeps] () {} - // When a Link receives overrides (via edgesIn), forward them to the target node which holds the actual edgesOut. - // Without this, overrides stop at the Link and never reach the target's dependency edges. + // When a Link receives overrides (via edgesIn), forward them to the target node which holds the actual edgesOut — but only when the OverrideSet has at least one rule that names a dep the target actually depends on. + // Without this scope, the link forwards a generic ancestor OverrideSet that has no real effect on the target's edges, but still flips the target to "has overrides", which changes downstream `canReplaceWith` / placement decisions and causes `npm ci` to re-resolve lockfile-pinned edges from the registry. + // See npm/cli#9357. recalculateOutEdgesOverrides () { - if (this.target) { - this.target.updateOverridesEdgeInAdded(this.overrides) + if (!this.target || !this.overrides) { + return + } + let hasMatchingRule = false + for (const rule of this.overrides.ruleset.values()) { + if (this.target.edgesOut.has(rule.name)) { + hasMatchingRule = true + break + } + } + if (!hasMatchingRule) { + return } + this.target.updateOverridesEdgeInAdded(this.overrides) } // links can't have children, only their targets can diff --git a/workspaces/arborist/lib/shrinkwrap.js b/workspaces/arborist/lib/shrinkwrap.js index 8cb1814661c6d..ce2c58457098d 100644 --- a/workspaces/arborist/lib/shrinkwrap.js +++ b/workspaces/arborist/lib/shrinkwrap.js @@ -909,10 +909,20 @@ class Shrinkwrap { if (node.extraneous && !/(^|\/)node_modules\//.test(loc) && loc !== 'node_modules') { continue } - this.data.packages[loc] = Shrinkwrap.metaFromNode( + const meta = Shrinkwrap.metaFromNode( node, this.path, this.resolveOptions) + // Skip inert nodes — these are optional deps that failed to load + // (e.g. 404 from a proxy registry that hasn't cached the package, + // or incomplete manifest missing version field). + // #pruneFailedOptional marks them inert so they won't be reified; + // writing them to the lockfile produces invalid entries like + // {"optional": true} that cause "Invalid Version:" errors. + if (node.inert && !node.package.version) { + continue + } + this.data.packages[loc] = meta } } else if (this.#awaitingUpdate.size > 0) { for (const loc of this.#awaitingUpdate.keys()) { diff --git a/workspaces/arborist/test/arborist/build-ideal-tree.js b/workspaces/arborist/test/arborist/build-ideal-tree.js index 143255a0ec9cd..d6705222ef73e 100644 --- a/workspaces/arborist/test/arborist/build-ideal-tree.js +++ b/workspaces/arborist/test/arborist/build-ideal-tree.js @@ -4816,3 +4816,70 @@ t.test('allow-directory=root soft-skips a transitive optional directory dependen t.ok(optChild, 'blocked optional transitive is recorded in the tree') t.equal(optChild.inert, true, 'blocked optional transitive is marked inert (will not be reified)') }) + +t.test('incomplete manifest from proxy registry prunes optional dep (#9342)', async t => { + // When a proxy/upstream registry returns an + // incomplete manifest for a platform-specific optional dep it hasn't + // cached, the version field is missing. Our fix in #nodeFromSpec + // treats this as EINCOMPLETEMANIFEST load failure so that + // #pruneFailedOptional() marks it inert instead of writing a broken + // lockfile entry like {"optional": true}. + const registry = createRegistry(t, false) + + // parent package with an optional dep + const esbuildPack = registry.packument({ + name: 'esbuild', + version: '0.27.7', + optionalDependencies: { + '@esbuild/aix-ppc64': '0.27.7', + }, + }) + const esbuildManifest = registry.manifest({ name: 'esbuild', packuments: [esbuildPack] }) + await registry.package({ manifest: esbuildManifest }) + + // simulate proxy registry returning incomplete manifest (no version field) + await registry.package({ + manifest: { + _id: '@esbuild/aix-ppc64', + _rev: '00-incomplete', + name: '@esbuild/aix-ppc64', + description: 'incomplete proxy manifest', + 'dist-tags': { latest: '0.27.7' }, + versions: { + '0.27.7': { + _id: '@esbuild/aix-ppc64@0.27.7', + name: '@esbuild/aix-ppc64', + // NO version field — this is the proxy registry bug + dependencies: {}, + dist: { + tarball: 'https://registry.npmjs.org/@esbuild/aix-ppc64/-/aix-ppc64-0.27.7.tgz', + }, + }, + }, + }, + }) + + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'test-incomplete-manifest', + version: '1.0.0', + devDependencies: { esbuild: '^0.27.0' }, + }), + }) + + const arb = newArb(path) + const tree = await arb.buildIdealTree() + + // esbuild itself should be in the tree + t.ok(tree.children.get('esbuild'), 'esbuild is installed') + t.equal(tree.children.get('esbuild').version, '0.27.7', 'esbuild has correct version') + + // @esbuild/aix-ppc64 should be marked inert (EINCOMPLETEMANIFEST → loadFailure) + // pruneFailedOptional marks it inert so it won't be written to lockfile + const aixNodes = [...tree.inventory.query('name', '@esbuild/aix-ppc64')] + const aixNode = aixNodes.find(n => n.root === tree) + t.ok(aixNode, 'incomplete optional dep node exists in tree') + t.equal(aixNode.inert, true, 'incomplete optional dep is marked inert') + t.equal(aixNode.errors[0].code, 'EINCOMPLETEMANIFEST', + 'node has EINCOMPLETEMANIFEST error') +}) diff --git a/workspaces/arborist/test/link.js b/workspaces/arborist/test/link.js index f9b7d8eb1d96d..472f33c2f4535 100644 --- a/workspaces/arborist/test/link.js +++ b/workspaces/arborist/test/link.js @@ -256,6 +256,51 @@ t.test('recalculateOutEdgesOverrides forwards overrides to target', t => { t.end() }) +t.test('recalculateOutEdgesOverrides does not forward when no rule matches a target dep — npm/cli#9357', t => { + // Regression: prior to the fix, Link.recalculateOutEdgesOverrides forwarded the link's full OverrideSet to the target unconditionally. + // For a target whose edges are NOT named in any override rule, that flipped target.overrides from undefined to the root's OverrideSet. + // Downstream, that "has overrides" state changed canPlaceDep's KEEP-vs-REPLACE decision and made `npm ci` re-resolve lockfile-pinned edges from the registry. + // After the fix, propagation is gated on at least one rule whose name matches an edge in target.edgesOut. + const root = new Node({ + path: '/path/to/root', + pkg: { + name: 'root', + dependencies: { foo: '1.0.0' }, + // override is for "bar", but the linked target only depends on "baz" + overrides: { bar: '2.0.0' }, + }, + loadOverrides: true, + }) + + const target = new Node({ + path: '/path/to/store/foo', + pkg: { + name: 'foo', + version: '1.0.0', + dependencies: { baz: '1.0.0' }, + }, + root, + }) + + // eslint-disable-next-line no-new + new Link({ + pkg: { name: 'foo', version: '1.0.0' }, + path: '/path/to/root/node_modules/foo', + realpath: '/path/to/store/foo', + target, + parent: root, + }) + + t.ok(root.overrides, 'root has overrides') + t.notOk(target.overrides, + 'target.overrides stays undefined when no rule matches a target dep') + const bazEdge = target.edgesOut.get('baz') + t.notOk(bazEdge.overrides, + 'unrelated edge keeps edge.overrides undefined') + t.equal(bazEdge.spec, '1.0.0', 'unrelated edge spec is unchanged') + t.end() +}) + t.test('link to root path gets root as target', t => { const root = new Node({ path: '/project/root', diff --git a/workspaces/arborist/test/shrinkwrap.js b/workspaces/arborist/test/shrinkwrap.js index bcdf50b0f87b0..79389a862caa0 100644 --- a/workspaces/arborist/test/shrinkwrap.js +++ b/workspaces/arborist/test/shrinkwrap.js @@ -863,6 +863,141 @@ t.test('load a hidden lockfile', async t => { t.equal(data.dependencies, undefined, 'deleted legacy metadata') }) +t.test('skip inert nodes in commit', async t => { + // When a proxy registry returns 404 or incomplete manifests for + // platform-specific optional deps, #pruneFailedOptional marks them + // inert. commit() must skip inert nodes — otherwise the lockfile + // gets entries like {"optional": true} without version/resolved/integrity + // that cause "Invalid Version:" errors on subsequent npm ci. + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'proxy-registry-repro', + version: '1.0.0', + devDependencies: { esbuild: '^0.27.0' }, + }), + }) + const meta = new Shrinkwrap({ path }) + meta.data = { + lockfileVersion: 3, + packages: {}, + } + + const root = new Node({ + pkg: { + name: 'proxy-registry-repro', + version: '1.0.0', + devDependencies: { esbuild: '^0.27.0' }, + }, + path, + realpath: path, + }) + + // esbuild with full metadata (valid) + const esbuild = new Node({ + pkg: { + name: 'esbuild', + version: '0.27.7', + optionalDependencies: { + '@esbuild/linux-x64': '0.27.7', + '@esbuild/aix-ppc64': '0.27.7', + }, + }, + name: 'esbuild', + parent: root, + }) + esbuild.dev = true + + // platform dep with full metadata (current platform — valid, NOT inert) + const validDep = new Node({ + pkg: { + name: '@esbuild/linux-x64', + version: '0.27.7', + os: ['linux'], + cpu: ['x64'], + }, + name: '@esbuild/linux-x64', + parent: root, + }) + validDep.optional = true + validDep.dev = true + + // platform dep marked inert (proxy registry 404'd or returned incomplete manifest) + // #pruneFailedOptional sets inert=true on these nodes + const brokenDep = new Node({ + pkg: { + name: '@esbuild/aix-ppc64', + // no version — proxy registry returned 404 or incomplete manifest + }, + name: '@esbuild/aix-ppc64', + parent: esbuild, + }) + brokenDep.optional = true + brokenDep.dev = true + brokenDep.extraneous = false + brokenDep.inert = true + + // file: optional dep WITHOUT version (legitimate — NOT inert, should be kept) + const fileDep = new Node({ + pkg: { + name: 'my-local-optional', + // no version — but this is a file: dep, so it's legitimate + }, + name: 'my-local-optional', + parent: root, + resolved: 'file:../my-local-optional', + }) + fileDep.optional = true + fileDep.extraneous = false + + // local optional dep WITHOUT version or resolved (NOT inert, should be kept) + const localDep = new Node({ + pkg: { + name: 'my-disk-optional', + // no version, no resolved — loaded from local node_modules + }, + name: 'my-disk-optional', + parent: root, + }) + localDep.optional = true + localDep.extraneous = false + + meta.tree = root + const committed = meta.commit() + + // The valid platform dep should be in the lockfile + const validLoc = 'node_modules/@esbuild/linux-x64' + t.ok( + committed.packages[validLoc], + 'valid optional dep is included' + ) + t.equal( + committed.packages[validLoc].version, + '0.27.7', + 'valid optional dep has version' + ) + + // The inert dep should NOT be in the lockfile + const brokenLoc = 'node_modules/esbuild/node_modules/@esbuild/aix-ppc64' + t.notOk( + committed.packages[brokenLoc], + 'inert optional dep is excluded from lockfile' + ) + + // The file: optional dep WITHOUT version SHOULD be kept (not inert) + const fileLoc = 'node_modules/my-local-optional' + t.ok( + committed.packages[fileLoc], + 'file: optional dep without version is preserved in lockfile' + ) + + // The local (resolved=null) optional dep WITHOUT version SHOULD be kept (not inert) + const localLoc = 'node_modules/my-disk-optional' + t.ok( + committed.packages[localLoc], + 'local optional dep without version is preserved in lockfile' + ) +}) + t.test('load a fresh hidden lockfile', async t => { const sw = await Shrinkwrap.reset({ path: hiddenLockfileFixture,