feat(patch): exclude package.json from the diff and warn when an edit only touches it

Author: manzoorwanijk

lib/commands/patch.js

@@ -212,18 +212,25 @@ class Patch extends BaseCommand {
 
     // extract a clean baseline to diff against
     const base = await mkdtemp(join(tmpdir(), 'npm-patch-base-'))
-    let diff
+    let diff, packageJsonChanged
     try {
       await pacote.extract(selectorKey(name, version), base, this.npm.flatOptions)
-      diff = await diffDirs(base, editDir)
+      ;({ diff, packageJsonChanged } = await diffDirs(base, editDir))
     } finally {
       await rm(base, { recursive: true, force: true })
     }
 
     if (!diff) {
-      log.warn('patch', `no changes detected in ${editDir}; nothing to commit`)
+      // package.json is excluded from patches, so an edit limited to it captures nothing
+      const reason = packageJsonChanged
+        ? `only package.json changed in ${editDir}, which is not patchable; nothing to commit`
+        : `no changes detected in ${editDir}; nothing to commit`
+      log.warn('patch', reason)
       return
     }
+    if (packageJsonChanged) {
+      log.warn('patch', 'changes to package.json are not included in patches and were ignored')
+    }
 
     const patchesDir = this.npm.config.get('patches-dir')
     const absPatch = resolve(this.#root, patchFilePath(patchesDir, name, version))

lib/utils/patch-diff.js

@@ -35,8 +35,9 @@ const readMaybe = async file => {
   }
 }
 
-// Diff originalDir against editedDir, returning a unified diff string.
+// Diff originalDir against editedDir, returning { diff, packageJsonChanged }.
 // Added files use `--- /dev/null`, deleted files use `+++ /dev/null`.
+// The root package.json is excluded: Arborist resolves the pre-patch manifest, so a patched manifest would apply to disk without being honored.
 const diffDirs = async (originalDir, editedDir) => {
   const [origFiles, editFiles] = await Promise.all([
     listFiles(originalDir),
@@ -45,6 +46,7 @@ const diffDirs = async (originalDir, editedDir) => {
   const all = [...new Set([...origFiles, ...editFiles])].sort()
 
   let result = ''
+  let packageJsonChanged = false
   for (const file of all) {
     const native = file.split('/').join(sep)
     const [a, b] = await Promise.all([
@@ -55,6 +57,12 @@ const diffDirs = async (originalDir, editedDir) => {
       continue
     }
 
+    // the root package.json is never patchable; flag the change so commit can warn
+    if (file === 'package.json') {
+      packageJsonChanged = true
+      continue
+    }
+
     let patch = createTwoFilesPatch(
       `a/${file}`, `b/${file}`, a || '', b || '', '', ''
     ).replace('===================================================================\n', '')
@@ -68,7 +76,7 @@ const diffDirs = async (originalDir, editedDir) => {
     }
     result += patch
   }
-  return result
+  return { diff: result, packageJsonChanged }
 }
 
 module.exports = { diffDirs }

test/lib/commands/patch.js

@@ -467,6 +467,64 @@ t.test('commit: no changes logs a warning and does not write a patch', async t =
   t.notOk(pkg.patchedDependencies, 'no patchedDependencies added when nothing changed')
 })
 
+t.test('commit: only package.json changed warns and writes no patch', async t => {
+  const { npm, logs, registry } = await loadMockNpm(t, {
+    config: { 'ignore-scripts': true, audit: false },
+    strictRegistryNock: false,
+    prefixDir: basePrefix(),
+  })
+  await setupDep(npm, registry)
+  await npm.exec('install', [])
+
+  await npm.exec('patch', ['add', DEP_NAME])
+  const editDir = path.join(npm.prefix, 'clean-edit')
+  await pacote.extract(`${DEP_NAME}@${DEP_VERSION}`, editDir, npm.flatOptions)
+
+  // edit only package.json, which is excluded from patches
+  const pkgPath = path.join(editDir, 'package.json')
+  const edited = readJson(pkgPath)
+  edited.description = 'edited'
+  fs.writeFileSync(pkgPath, JSON.stringify(edited))
+
+  await npm.exec('patch', ['commit', editDir])
+  t.notOk(
+    fs.existsSync(path.join(npm.prefix, 'patches', `${DEP_NAME}@${DEP_VERSION}.patch`)),
+    'no patch file written when only package.json changed'
+  )
+  t.match(logs.warn.join('\n'), /only package.json changed/, 'warns package.json is not patchable')
+})
+
+t.test('commit: package.json change alongside code is dropped with a warning', async t => {
+  const { npm, logs, registry } = await loadMockNpm(t, {
+    config: { 'ignore-scripts': true, audit: false },
+    strictRegistryNock: false,
+    prefixDir: basePrefix(),
+  })
+  await setupDep(npm, registry)
+  await npm.exec('install', [])
+
+  await npm.exec('patch', ['add', DEP_NAME])
+  const editDir = path.join(npm.prefix, 'clean-edit')
+  await pacote.extract(`${DEP_NAME}@${DEP_VERSION}`, editDir, npm.flatOptions)
+
+  // edit both package.json and a real file
+  const pkgPath = path.join(editDir, 'package.json')
+  const edited = readJson(pkgPath)
+  edited.description = 'edited'
+  fs.writeFileSync(pkgPath, JSON.stringify(edited))
+  fs.writeFileSync(path.join(editDir, 'index.js'), 'module.exports = () => "patched"\n')
+
+  await npm.exec('patch', ['commit', editDir])
+  const patchPath = path.join(npm.prefix, 'patches', `${DEP_NAME}@${DEP_VERSION}.patch`)
+  t.ok(fs.existsSync(patchPath), 'patch written for the code change')
+  t.notMatch(fs.readFileSync(patchPath, 'utf8'), 'package.json', 'patch excludes package.json')
+  t.match(
+    logs.warn.join('\n'),
+    /changes to package.json are not included/,
+    'warns the package.json edit was ignored'
+  )
+})
+
 t.test('rm: no pkg arg rejects with EUSAGE', async t => {
   const { npm } = await loadMockNpm(t, {
     config: { 'ignore-scripts': true, audit: false },

test/lib/utils/patch-diff.js

@@ -12,7 +12,7 @@ t.test('modified file produces a unified diff', async t => {
     orig: { 'index.js': 'hello\n' },
     edit: { 'index.js': 'world\n' },
   })
-  const diff = await diffDirs(resolve(dir, 'orig'), resolve(dir, 'edit'))
+  const { diff } = await diffDirs(resolve(dir, 'orig'), resolve(dir, 'edit'))
   t.match(diff, '--- a/index.js', 'has old header')
   t.match(diff, '+++ b/index.js', 'has new header')
   t.match(diff, '-hello', 'removes old line')
@@ -25,7 +25,7 @@ t.test('added file uses --- /dev/null', async t => {
     orig: { 'keep.js': 'same\n' },
     edit: { 'keep.js': 'same\n', 'added.js': 'brand new\n' },
   })
-  const diff = await diffDirs(resolve(dir, 'orig'), resolve(dir, 'edit'))
+  const { diff } = await diffDirs(resolve(dir, 'orig'), resolve(dir, 'edit'))
   t.match(diff, '--- /dev/null', 'old side is /dev/null')
   t.match(diff, '+++ b/added.js', 'new side names the added file')
   t.match(diff, '+brand new', 'includes added content')
@@ -37,7 +37,7 @@ t.test('deleted file uses +++ /dev/null', async t => {
     orig: { 'gone.js': 'remove me\n' },
     edit: {},
   })
-  const diff = await diffDirs(resolve(dir, 'orig'), resolve(dir, 'edit'))
+  const { diff } = await diffDirs(resolve(dir, 'orig'), resolve(dir, 'edit'))
   t.match(diff, '--- a/gone.js', 'old side names the deleted file')
   t.match(diff, '+++ /dev/null', 'new side is /dev/null')
   t.match(diff, '-remove me', 'includes removed content')
@@ -48,7 +48,7 @@ t.test('nested file path is posix-separated in the diff', async t => {
     orig: { lib: { deep: { 'x.js': 'a\n' } } },
     edit: { lib: { deep: { 'x.js': 'b\n' } } },
   })
-  const diff = await diffDirs(resolve(dir, 'orig'), resolve(dir, 'edit'))
+  const { diff } = await diffDirs(resolve(dir, 'orig'), resolve(dir, 'edit'))
   t.match(diff, '--- a/lib/deep/x.js', 'old header uses posix separators')
   t.match(diff, '+++ b/lib/deep/x.js', 'new header uses posix separators')
 })
@@ -58,7 +58,7 @@ t.test('identical files produce no diff', async t => {
     orig: { 'a.js': 'x\n', sub: { 'b.js': 'y\n' } },
     edit: { 'a.js': 'x\n', sub: { 'b.js': 'y\n' } },
   })
-  const diff = await diffDirs(resolve(dir, 'orig'), resolve(dir, 'edit'))
+  const { diff } = await diffDirs(resolve(dir, 'orig'), resolve(dir, 'edit'))
   t.equal(diff, '', 'empty diff for identical trees')
 })
 
@@ -75,20 +75,50 @@ t.test('node_modules and .git are ignored', async t => {
       '.git': { HEAD: 'ref: refs/heads/other\n' },
     },
   })
-  const diff = await diffDirs(resolve(dir, 'orig'), resolve(dir, 'edit'))
+  const { diff } = await diffDirs(resolve(dir, 'orig'), resolve(dir, 'edit'))
   t.match(diff, 'index.js', 'top-level change is captured')
   t.notMatch(diff, 'node_modules', 'node_modules contents are excluded')
   t.notMatch(diff, 'HEAD', '.git contents are excluded')
 })
 
+t.test('root package.json is excluded and flagged, nested is kept', async t => {
+  const dir = t.testdir({
+    orig: {
+      'package.json': '{ "version": "1.0.0" }\n',
+      'index.js': 'a\n',
+      sub: { 'package.json': '{ "private": true }\n' },
+    },
+    edit: {
+      'package.json': '{ "version": "2.0.0" }\n',
+      'index.js': 'b\n',
+      sub: { 'package.json': '{ "private": false }\n' },
+    },
+  })
+  const { diff, packageJsonChanged } = await diffDirs(resolve(dir, 'orig'), resolve(dir, 'edit'))
+  t.equal(packageJsonChanged, true, 'root package.json change is flagged')
+  t.notMatch(diff, 'a/package.json\t', 'root package.json is not in the diff')
+  t.match(diff, 'a/sub/package.json', 'nested package.json is still diffed')
+  t.match(diff, 'a/index.js', 'other files are still diffed')
+})
+
+t.test('packageJsonChanged is false when only other files change', async t => {
+  const dir = t.testdir({
+    orig: { 'package.json': '{ "version": "1.0.0" }\n', 'index.js': 'a\n' },
+    edit: { 'package.json': '{ "version": "1.0.0" }\n', 'index.js': 'b\n' },
+  })
+  const { diff, packageJsonChanged } = await diffDirs(resolve(dir, 'orig'), resolve(dir, 'edit'))
+  t.equal(packageJsonChanged, false, 'unchanged package.json is not flagged')
+  t.match(diff, 'a/index.js', 'the real change is captured')
+})
+
 t.test('non-file entries like symlinks are skipped', async t => {
   const dir = t.testdir({
     orig: { 'real.js': 'a\n' },
     edit: { 'real.js': 'b\n' },
   })
   // A symlink is neither a directory nor a regular file so it is ignored.
   symlinkSync(resolve(dir, 'orig', 'real.js'), resolve(dir, 'edit', 'link.js'))
-  const diff = await diffDirs(resolve(dir, 'orig'), resolve(dir, 'edit'))
+  const { diff } = await diffDirs(resolve(dir, 'orig'), resolve(dir, 'edit'))
   t.match(diff, 'real.js', 'regular file is diffed')
   t.notMatch(diff, 'link.js', 'symlink entry is skipped')
 })
@@ -107,7 +137,7 @@ t.test('round-trip: applying the diff reproduces the edited tree', async t => {
     },
   })
   const orig = resolve(dir, 'orig')
-  const diff = await diffDirs(orig, resolve(dir, 'edit'))
+  const { diff } = await diffDirs(orig, resolve(dir, 'edit'))
 
   // Apply the diff back onto a copy of the original and check the result.
   await applyPatchToDir({ patch: diff, cwd: orig })