From c7fe2258cf1d50593ae65c4c518d5309627182d1 Mon Sep 17 00:00:00 2001 From: Manzoor Wani Date: Fri, 19 Jun 2026 00:57:43 +0530 Subject: [PATCH] fix(arborist): drop orphaned patch entry on uninstall instead of EPATCHUNUSED --- .../arborist/lib/arborist/build-ideal-tree.js | 1 + workspaces/arborist/lib/arborist/reify.js | 3 + .../arborist/lib/patched-dependencies.js | 26 ++++- .../arborist/test/arborist/reify-patch.js | 23 ++++ .../test/patched-dependencies-resolve.js | 108 ++++++++++++++++++ 5 files changed, 156 insertions(+), 5 deletions(-) diff --git a/workspaces/arborist/lib/arborist/build-ideal-tree.js b/workspaces/arborist/lib/arborist/build-ideal-tree.js index 58e803b844cc1..f3c6d7ec846d7 100644 --- a/workspaces/arborist/lib/arborist/build-ideal-tree.js +++ b/workspaces/arborist/lib/arborist/build-ideal-tree.js @@ -187,6 +187,7 @@ module.exports = cls => class IdealTreeBuilder extends cls { await resolvePatchedDependencies(this.idealTree, { path: this.path, allowUnusedPatches: this.options.allowUnusedPatches, + rm: options.rm || [], }) this.#warnWorkspacePackageExtensions() } finally { diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index 0183324ea0f3b..cfb1fa5096fa7 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -1824,6 +1824,8 @@ module.exports = cls => class Reifier extends cls { // field so defaulting this to an empty array would add that field to // every package.json file. bundleDependencies, + // resolvePatchedDependencies drops entries orphaned by uninstall; persist that removal + patchedDependencies, } = tree.package pkgJson.update({ @@ -1832,6 +1834,7 @@ module.exports = cls => class Reifier extends cls { optionalDependencies, peerDependencies, bundleDependencies, + patchedDependencies, }) await pkgJson.save() } diff --git a/workspaces/arborist/lib/patched-dependencies.js b/workspaces/arborist/lib/patched-dependencies.js index c814974b5471e..411406f0df71a 100644 --- a/workspaces/arborist/lib/patched-dependencies.js +++ b/workspaces/arborist/lib/patched-dependencies.js @@ -3,6 +3,7 @@ // Enforces the failure modes (workspace-member entry, missing file, unused patch, non-registry target, ambiguous selectors) as hard errors. const semver = require('semver') const npa = require('npm-package-arg') +const { log } = require('proc-log') const { resolve, relative, isAbsolute } = require('node:path') const { readFile } = require('node:fs/promises') const { patchIntegrity } = require('./patch.js') @@ -61,7 +62,7 @@ const matchSelector = (selectors, node) => { return matches.find(s => s.spec === null) || null } -const resolvePatchedDependencies = async (tree, { path, allowUnusedPatches }) => { +const resolvePatchedDependencies = async (tree, { path, allowUnusedPatches, rm }) => { const patchedDependencies = tree.package?.patchedDependencies || {} const selectors = Object.entries(patchedDependencies) .map(([key, patchPath]) => ({ ...parseSelector(key), key, patchPath })) @@ -135,15 +136,30 @@ const resolvePatchedDependencies = async (tree, { path, allowUnusedPatches }) => usedKeys.add(selector.key) } - if (selectors.length && !allowUnusedPatches) { + if (selectors.length) { const unused = selectors.filter(s => !usedKeys.has(s.key)) - if (unused.length) { + + // an explicit `npm uninstall ` orphans that package's patch entry, so drop it instead of failing + const removed = new Set(rm) + const dropped = unused.filter(s => removed.has(s.name)) + if (dropped.length) { + const patched = { ...tree.package.patchedDependencies } + for (const s of dropped) { + delete patched[s.key] + log.notice('patch', `Removing patch entry "${s.key}" for uninstalled ${s.name}; left ${s.patchPath} in place.`) + } + // undefined drops the key entirely when reify writes package.json + tree.package.patchedDependencies = Object.keys(patched).length ? patched : undefined + } + + const stillUnused = unused.filter(s => !removed.has(s.name)) + if (stillUnused.length && !allowUnusedPatches) { throw err( `The following patches were registered but matched no installed ` + - `package:\n${unused.map(s => ` ${s.key} -> ${s.patchPath}`).join('\n')}\n` + + `package:\n${stillUnused.map(s => ` ${s.key} -> ${s.patchPath}`).join('\n')}\n` + `Use --allow-unused-patches to install anyway.`, 'EPATCHUNUSED', - { unused: unused.map(s => s.key) } + { unused: stillUnused.map(s => s.key) } ) } } diff --git a/workspaces/arborist/test/arborist/reify-patch.js b/workspaces/arborist/test/arborist/reify-patch.js index b3b0db980eb75..aa56669b12e27 100644 --- a/workspaces/arborist/test/arborist/reify-patch.js +++ b/workspaces/arborist/test/arborist/reify-patch.js @@ -102,6 +102,29 @@ t.test('registry dep with patch is applied and recorded in lockfile', async t => t.match(pkgEntry.patched.integrity, /^sha512-/, 'patched.integrity is an SSRI') }) +t.test('uninstalling a patched package drops its entry from package.json', async t => { + const registry = createRegistry(t) + // one install, then an uninstall that resolves from the lockfile so no extra fetches + await mockPackage(t, registry) + + const patch = filePatch('index.js', ORIGINAL, PATCHED) + const path = makeProject(t, { + patch, + patchedDependencies: { [`${PKG_NAME}@${PKG_VERSION}`]: `patches/${PKG_NAME}@${PKG_VERSION}.patch` }, + }) + + await newArb({ path }).reify() + t.equal(fs.readFileSync(installedFile(path), 'utf8'), PATCHED, 'package installed and patched') + + // npm uninstall : removes the dep but leaves the now-orphaned patch entry + await newArb({ path }).reify({ rm: [PKG_NAME] }) + + const rootPkg = JSON.parse(fs.readFileSync(resolve(path, 'package.json'), 'utf8')) + t.notOk(rootPkg.dependencies?.[PKG_NAME], 'dependency removed from package.json') + t.notOk(rootPkg.patchedDependencies, 'orphaned patch entry dropped from package.json') + t.notOk(fs.existsSync(resolve(path, 'node_modules', PKG_NAME)), 'package removed from node_modules') +}) + t.test('patch is re-applied on a patch-change reify even with ignoreScripts', async t => { const registry = createRegistry(t) // two reifys: the second re-extracts the node due to the patch change. diff --git a/workspaces/arborist/test/patched-dependencies-resolve.js b/workspaces/arborist/test/patched-dependencies-resolve.js index c2cd830dff1ef..d213b3cf87019 100644 --- a/workspaces/arborist/test/patched-dependencies-resolve.js +++ b/workspaces/arborist/test/patched-dependencies-resolve.js @@ -239,6 +239,114 @@ t.test('EPATCHUNUSED when a registered patch matches no node', async t => { await t.rejects(buildIdeal(path), { code: 'EPATCHUNUSED', unused: ['ghost@1.0.0'] }) }) +t.test('uninstalling a patched package drops its orphaned patch entry', async t => { + // npm uninstall removes the node but leaves the patchedDependencies entry, which would otherwise be EPATCHUNUSED + const path = t.testdir({ + 'fix.patch': PATCH, + 'package.json': JSON.stringify({ + name: 'root', + version: '1.0.0', + dependencies: { dep: '^1.0.0' }, + patchedDependencies: { 'dep@1.0.0': 'fix.patch' }, + }), + 'package-lock.json': JSON.stringify({ + name: 'root', + version: '1.0.0', + lockfileVersion: 3, + requires: true, + packages: { + '': { name: 'root', version: '1.0.0', dependencies: { dep: '^1.0.0' } }, + 'node_modules/dep': lockEntry('dep', '1.0.0'), + }, + }), + node_modules: { + dep: { 'package.json': JSON.stringify({ name: 'dep', version: '1.0.0' }) }, + }, + }) + + const tree = await buildIdeal(path, { rm: ['dep'] }) + t.notOk(tree.inventory.query('name', 'dep').size, 'dep node is removed') + t.notOk(tree.package.patchedDependencies, 'the orphaned patch entry is dropped') +}) + +t.test('uninstall keeps patch entries for packages that remain', async t => { + // removing one package must not drop another package's still-used patch + const path = t.testdir({ + 'a.patch': PATCH, + 'b.patch': PATCH, + 'package.json': JSON.stringify({ + name: 'root', + version: '1.0.0', + dependencies: { a: '^1.0.0', b: '^1.0.0' }, + patchedDependencies: { 'a@1.0.0': 'a.patch', 'b@1.0.0': 'b.patch' }, + }), + 'package-lock.json': JSON.stringify({ + name: 'root', + version: '1.0.0', + lockfileVersion: 3, + requires: true, + packages: { + '': { name: 'root', version: '1.0.0', dependencies: { a: '^1.0.0', b: '^1.0.0' } }, + 'node_modules/a': lockEntry('a', '1.0.0'), + 'node_modules/b': lockEntry('b', '1.0.0'), + }, + }), + node_modules: { + a: { 'package.json': JSON.stringify({ name: 'a', version: '1.0.0' }) }, + b: { 'package.json': JSON.stringify({ name: 'b', version: '1.0.0' }) }, + }, + }) + + const tree = await buildIdeal(path, { rm: ['a'] }) + t.notOk(tree.inventory.query('name', 'a').size, 'a node is removed') + t.same(tree.package.patchedDependencies, { 'b@1.0.0': 'b.patch' }, 'b patch entry survives') + const b = tree.inventory.query('name', 'b').values().next().value + t.ok(b.patched, 'b is still patched') +}) + +t.test('uninstall does not drop a patch when the package survives as a transitive dep', async t => { + // dep is a direct dep being removed but stays in the tree under parent, so its patch is still used + const path = t.testdir({ + 'fix.patch': PATCH, + 'package.json': JSON.stringify({ + name: 'root', + version: '1.0.0', + dependencies: { dep: '^1.0.0', parent: '^1.0.0' }, + patchedDependencies: { 'dep@1.0.0': 'fix.patch' }, + }), + 'package-lock.json': JSON.stringify({ + name: 'root', + version: '1.0.0', + lockfileVersion: 3, + requires: true, + packages: { + '': { name: 'root', version: '1.0.0', dependencies: { dep: '^1.0.0', parent: '^1.0.0' } }, + 'node_modules/dep': lockEntry('dep', '1.0.0'), + 'node_modules/parent': { + ...lockEntry('parent', '1.0.0'), + dependencies: { dep: '^1.0.0' }, + }, + }, + }), + node_modules: { + dep: { 'package.json': JSON.stringify({ name: 'dep', version: '1.0.0' }) }, + parent: { + 'package.json': JSON.stringify({ + name: 'parent', + version: '1.0.0', + dependencies: { dep: '^1.0.0' }, + }), + }, + }, + }) + + const tree = await buildIdeal(path, { rm: ['dep'] }) + const dep = tree.inventory.query('name', 'dep').values().next().value + t.ok(dep, 'dep node survives as a transitive dependency') + t.ok(dep.patched, 'dep is still patched') + t.same(tree.package.patchedDependencies, { 'dep@1.0.0': 'fix.patch' }, 'patch entry is kept') +}) + t.test('allowUnusedPatches:true suppresses EPATCHUNUSED', async t => { const path = t.testdir({ 'fix.patch': PATCH,