feat(plugin-commands-patching): apply existed patch file when re-patch (#5906)

close #5632

Co-authored-by: Zoltan Kochan <z@kochan.io>

.changeset/clean-swans-listen.md

@@ -0,0 +1,5 @@
+---
+"@pnpm/plugin-commands-patching": minor
+---
+
+apply existing patch file when re-patch [#5632](https://github.com/pnpm/pnpm/issues/5632)

.changeset/eleven-fireants-warn.md

@@ -0,0 +1,5 @@
+---
+"@pnpm/parse-wanted-dependency": minor
+---
+
+Export `ParseWantedDependencyResult`.

.changeset/flat-eagles-tease.md

@@ -0,0 +1,5 @@
+---
+"pnpm": minor
+---
+
+When patching a dependency that is already patched, the existing patch is applied to the dependency, so that the new edit are applied on top of the existing ones. To ignore the existing patches, run the patch command with the `--ignore-existing` option [#5632](https://github.com/pnpm/pnpm/issues/5632).

.changeset/fuzzy-birds-joke.md

@@ -0,0 +1,5 @@
+---
+"@pnpm/patching.apply-patch": major
+---
+
+Initial release.

exec/build-modules/package.json

@@ -36,16 +36,15 @@
   "dependencies": {
     "@pnpm/calc-dep-state": "workspace:*",
     "@pnpm/core-loggers": "workspace:*",
-    "@pnpm/error": "workspace:*",
     "@pnpm/fs.hard-link-dir": "workspace:*",
     "@pnpm/graph-sequencer": "1.0.0",
     "@pnpm/lifecycle": "workspace:*",
     "@pnpm/link-bins": "workspace:*",
+    "@pnpm/patching.apply-patch": "workspace:*",
     "@pnpm/read-package-json": "workspace:*",
     "@pnpm/store-controller-types": "workspace:*",
     "@pnpm/types": "workspace:*",
     "p-defer": "^3.0.0",
-    "patch-package": "^6.5.1",
     "ramda": "npm:@pnpm/ramda@0.28.1",
     "run-groups": "^3.0.1"
   },

exec/build-modules/src/index.ts

@@ -1,16 +1,15 @@
 import path from 'path'
 import { calcDepState, DepsStateCache } from '@pnpm/calc-dep-state'
 import { skippedOptionalDependencyLogger } from '@pnpm/core-loggers'
-import { PnpmError } from '@pnpm/error'
 import { runPostinstallHooks } from '@pnpm/lifecycle'
 import { linkBins, linkBinsOfPackages } from '@pnpm/link-bins'
 import { logger } from '@pnpm/logger'
 import { hardLinkDir } from '@pnpm/fs.hard-link-dir'
 import { readPackageJsonFromDir, safeReadPackageJsonFromDir } from '@pnpm/read-package-json'
 import { StoreController } from '@pnpm/store-controller-types'
+import { applyPatchToDir } from '@pnpm/patching.apply-patch'
 import { DependencyManifest } from '@pnpm/types'
 import pDefer, { DeferredPromise } from 'p-defer'
-import { applyPatch } from 'patch-package/dist/applyPatches'
 import pickBy from 'ramda/src/pickBy'
 import runGroups from 'run-groups'
 import { buildSequence, DependenciesGraph, DependenciesGraphNode } from './buildSequence'
@@ -106,7 +105,7 @@ async function buildDependency (
     await linkBinsOfDependencies(depNode, depGraph, opts)
     const isPatched = depNode.patchFile?.path != null
     if (isPatched) {
-      applyPatchToDep(depNode.dir, depNode.patchFile!.path)
+      applyPatchToDir({ patchedDir: depNode.dir, patchFilePath: depNode.patchFile!.path })
     }
     const hasSideEffects = !opts.ignoreScripts && await runPostinstallHooks({
       depPath,
@@ -179,21 +178,6 @@ async function buildDependency (
   }
 }
 
-function applyPatchToDep (patchDir: string, patchFilePath: string) {
-  // Ideally, we would just run "patch" or "git apply".
-  // However, "patch" is not available on Windows and "git apply" is hard to execute on a subdirectory of an existing repository
-  const cwd = process.cwd()
-  process.chdir(patchDir)
-  const success = applyPatch({
-    patchFilePath,
-    patchDir,
-  })
-  process.chdir(cwd)
-  if (!success) {
-    throw new PnpmError('PATCH_FAILED', `Could not apply patch ${patchFilePath} to ${patchDir}`)
-  }
-}
-
 export async function linkBinsOfDependencies (
   depNode: DependenciesGraphNode,
   depGraph: DependenciesGraph,

exec/build-modules/tsconfig.json

@@ -19,10 +19,10 @@
       "path": "../../packages/core-loggers"
     },
     {
-      "path": "../../packages/error"
+      "path": "../../packages/types"
     },
     {
-      "path": "../../packages/types"
+      "path": "../../patching/apply-patch"
     },
     {
       "path": "../../pkg-manager/link-bins"

packages/parse-wanted-dependency/src/index.ts

@@ -1,13 +1,18 @@
 import validateNpmPackageName from 'validate-npm-package-name'
 
-interface ParsedWantedDependency {
+export interface ParsedWantedDependency {
   alias: string
   pref: string
 }
 
-export function parseWantedDependency (
-  rawWantedDependency: string
-): Partial<ParsedWantedDependency> & (Omit<ParsedWantedDependency, 'pref'> | Omit<ParsedWantedDependency, 'alias'> | ParsedWantedDependency) {
+export type ParseWantedDependencyResult = Partial<ParsedWantedDependency> &
+(
+  Omit<ParsedWantedDependency, 'pref'>
+  | Omit<ParsedWantedDependency, 'alias'>
+  | ParsedWantedDependency
+)
+
+export function parseWantedDependency (rawWantedDependency: string): ParseWantedDependencyResult {
   const versionDelimiter = rawWantedDependency.indexOf('@', 1) // starting from 1 to skip the @ that marks scope
   if (versionDelimiter !== -1) {
     const alias = rawWantedDependency.slice(0, versionDelimiter)

packages/plugin-commands-patching/package.json

@@ -42,6 +42,7 @@
     "@pnpm/cli-utils": "workspace:*",
     "@pnpm/config": "workspace:*",
     "@pnpm/error": "workspace:*",
+    "@pnpm/patching.apply-patch": "workspace:*",
     "@pnpm/parse-wanted-dependency": "workspace:*",
     "@pnpm/pick-registry-for-package": "workspace:*",
     "@pnpm/plugin-commands-installation": "workspace:*",

packages/plugin-commands-patching/src/patch.ts

@@ -1,4 +1,6 @@
 import fs from 'fs'
+import path from 'path'
+import { applyPatchToDir } from '@pnpm/patching.apply-patch'
 import { docsUrl } from '@pnpm/cli-utils'
 import { Config, types as allTypes } from '@pnpm/config'
 import { LogBase } from '@pnpm/logger'
@@ -9,14 +11,14 @@ import pick from 'ramda/src/pick'
 import renderHelp from 'render-help'
 import tempy from 'tempy'
 import { PnpmError } from '@pnpm/error'
-import { writePackage } from './writePackage'
+import { writePackage, ParseWantedDependencyResult } from './writePackage'
 
 export function rcOptionsTypes () {
   return pick([], allTypes)
 }
 
 export function cliOptionsTypes () {
-  return { ...rcOptionsTypes(), 'edit-dir': String }
+  return { ...rcOptionsTypes(), 'edit-dir': String, 'ignore-existing': Boolean }
 }
 
 export const shorthands = {
@@ -35,6 +37,10 @@ export function help () {
           description: 'The package that needs to be modified will be extracted to this directory',
           name: '--edit-dir',
         },
+        {
+          description: 'Ignore existing patch files when patching',
+          name: '--ignore-existing',
+        },
       ],
     }],
     url: docsUrl('patch'),
@@ -42,9 +48,10 @@ export function help () {
   })
 }
 
-export type PatchCommandOptions = Pick<Config, 'dir' | 'registries' | 'tag' | 'storeDir'> & CreateStoreControllerOptions & {
+export type PatchCommandOptions = Pick<Config, 'dir' | 'registries' | 'tag' | 'storeDir' | 'rootProjectManifest' | 'lockfileDir'> & CreateStoreControllerOptions & {
   editDir?: string
   reporter?: (logObj: LogBase) => void
+  ignoreExisting?: boolean
 }
 
 export async function handler (opts: PatchCommandOptions, params: string[]) {
@@ -52,8 +59,43 @@ export async function handler (opts: PatchCommandOptions, params: string[]) {
     throw new PnpmError('PATCH_EDIT_DIR_EXISTS', `The target directory already exists: '${opts.editDir}'`)
   }
   const editDir = opts.editDir ?? tempy.directory()
-  await writePackage(params[0], editDir, opts)
+  const patchedDep = await writePackage(params[0], editDir, opts)
+  if (!opts.ignoreExisting && opts.rootProjectManifest?.pnpm?.patchedDependencies) {
+    tryPatchWithExistingPatchFile({
+      patchedDep,
+      patchedDir: editDir,
+      patchedDependencies: opts.rootProjectManifest.pnpm.patchedDependencies,
+      lockfileDir: opts.lockfileDir ?? opts.dir ?? process.cwd(),
+    })
+  }
   return `You can now edit the following folder: ${editDir}
 
 Once you're done with your changes, run "pnpm patch-commit ${editDir}"`
 }
+
+function tryPatchWithExistingPatchFile (
+  {
+    patchedDep,
+    patchedDir,
+    patchedDependencies,
+    lockfileDir,
+  }: {
+    patchedDep: ParseWantedDependencyResult
+    patchedDir: string
+    patchedDependencies: Record<string, string>
+    lockfileDir: string
+  }
+) {
+  if (!patchedDep.alias || !patchedDep.pref) {
+    return
+  }
+  const existingPatchFile = patchedDependencies[`${patchedDep.alias}@${patchedDep.pref}`]
+  if (!existingPatchFile) {
+    return
+  }
+  const existingPatchFilePath = path.resolve(lockfileDir, existingPatchFile)
+  if (!fs.existsSync(existingPatchFilePath)) {
+    throw new PnpmError('PATCH_FILE_NOT_FOUND', `Unable to find patch file ${existingPatchFilePath}`)
+  }
+  applyPatchToDir({ patchedDir, patchFilePath: existingPatchFilePath })
+}

packages/plugin-commands-patching/src/writePackage.ts

@@ -4,11 +4,13 @@ import {
   CreateStoreControllerOptions,
 } from '@pnpm/store-connection-manager'
 import { pickRegistryForPackage } from '@pnpm/pick-registry-for-package'
-import { parseWantedDependency } from '@pnpm/parse-wanted-dependency'
+import { parseWantedDependency, ParseWantedDependencyResult } from '@pnpm/parse-wanted-dependency'
+
+export { ParseWantedDependencyResult }
 
 export type WritePackageOptions = CreateStoreControllerOptions & Pick<Config, 'registries'>
 
-export async function writePackage (pkg: string, dest: string, opts: WritePackageOptions) {
+export async function writePackage (pkg: string, dest: string, opts: WritePackageOptions): Promise<ParseWantedDependencyResult> {
   const dep = parseWantedDependency(pkg)
   const store = await createOrConnectStoreController({
     ...opts,
@@ -26,4 +28,5 @@ export async function writePackage (pkg: string, dest: string, opts: WritePackag
     filesResponse,
     force: true,
   })
+  return dep
 }

packages/plugin-commands-patching/test/patch.test.ts

@@ -112,6 +112,78 @@ describe('patch and commit', () => {
 
     expect(fs.readFileSync('node_modules/is-positive/index.js', 'utf8')).toContain('// test patching')
   })
+
+  test('should reuse existing patch file by default', async () => {
+    let output = await patch.handler(defaultPatchOption, ['is-positive@1.0.0'])
+    let patchDir = getPatchDirFromPatchOutput(output)
+
+    fs.appendFileSync(path.join(patchDir, 'index.js'), '// test patching', 'utf8')
+    fs.unlinkSync(path.join(patchDir, 'license'))
+
+    await patchCommit.handler({
+      ...DEFAULT_OPTS,
+      dir: process.cwd(),
+    }, [patchDir])
+
+    const { manifest } = await readProjectManifest(process.cwd())
+    expect(manifest.pnpm?.patchedDependencies).toStrictEqual({
+      'is-positive@1.0.0': 'patches/is-positive@1.0.0.patch',
+    })
+    expect(fs.existsSync('patches/is-positive@1.0.0.patch')).toBe(true)
+
+    // re-patch
+    output = await patch.handler({ ...defaultPatchOption, rootProjectManifest: manifest }, ['is-positive@1.0.0'])
+    patchDir = getPatchDirFromPatchOutput(output)
+
+    expect(fs.existsSync(patchDir)).toBe(true)
+    expect(fs.existsSync(path.join(patchDir, 'license'))).toBe(false)
+    expect(fs.readFileSync(path.join(patchDir, 'index.js'), 'utf8')).toContain('// test patching')
+  })
+
+  test('if the patch file is not existed when patching, should throw an error', async () => {
+    const { writeProjectManifest, manifest } = await readProjectManifest(process.cwd())
+    await writeProjectManifest({
+      ...manifest,
+      pnpm: {
+        patchedDependencies: {
+          'is-positive@1.0.0': 'patches/not-found.patch',
+        },
+      },
+    })
+
+    try {
+      await patch.handler(defaultPatchOption, ['is-positive@1.0.0'])
+    } catch (err: any) { // eslint-disable-line
+      expect(err.code).toBe('ERR_PNPM_PATCH_FILE_NOT_FOUND')
+    }
+  })
+
+  test('should ignore patch files with --ignore-patches', async () => {
+    let output = await patch.handler(defaultPatchOption, ['is-positive@1.0.0'])
+    let patchDir = getPatchDirFromPatchOutput(output)
+
+    fs.appendFileSync(path.join(patchDir, 'index.js'), '// test patching', 'utf8')
+    fs.unlinkSync(path.join(patchDir, 'license'))
+
+    await patchCommit.handler({
+      ...DEFAULT_OPTS,
+      dir: process.cwd(),
+    }, [patchDir])
+
+    const { manifest } = await readProjectManifest(process.cwd())
+    expect(manifest.pnpm?.patchedDependencies).toStrictEqual({
+      'is-positive@1.0.0': 'patches/is-positive@1.0.0.patch',
+    })
+    expect(fs.existsSync('patches/is-positive@1.0.0.patch')).toBe(true)
+
+    // re-patch with --ignore-patches
+    output = await patch.handler({ ...defaultPatchOption, ignoreExisting: true }, ['is-positive@1.0.0'])
+    patchDir = getPatchDirFromPatchOutput(output)
+
+    expect(fs.existsSync(patchDir)).toBe(true)
+    expect(fs.existsSync(path.join(patchDir, 'license'))).toBe(true)
+    expect(fs.readFileSync(path.join(patchDir, 'index.js'), 'utf8')).not.toContain('// test patching')
+  })
 })
 
 describe('patching should work when there is a no EOL in the patched file', () => {

packages/plugin-commands-patching/tsconfig.json

@@ -21,6 +21,9 @@
     {
       "path": "../../config/pick-registry-for-package"
     },
+    {
+      "path": "../../patching/apply-patch"
+    },
     {
       "path": "../../pkg-manager/plugin-commands-installation"
     },

patching/apply-patch/README.md

@@ -0,0 +1,17 @@
+# @pnpm/patching.apply-patch
+
+> Apply a patch to a directory
+
+<!--@shields('npm')-->
+[![npm version](https://img.shields.io/npm/v/@pnpm/patching.apply-patch.svg)](https://www.npmjs.com/package/@pnpm/patching.apply-patch)
+<!--/@-->
+
+## Installation
+
+```sh
+pnpm add @pnpm/patching.apply-patch
+```
+
+## License
+
+MIT

patching/apply-patch/package.json

@@ -0,0 +1,42 @@
+{
+  "name": "@pnpm/patching.apply-patch",
+  "version": "0.0.0",
+  "description": "Apply a patch to a directory",
+  "main": "lib/index.js",
+  "types": "lib/index.d.ts",
+  "files": [
+    "lib",
+    "!*.map"
+  ],
+  "engines": {
+    "node": ">=14.6"
+  },
+  "scripts": {
+    "test": "pnpm run compile",
+    "prepublishOnly": "pnpm run compile",
+    "compile": "tsc --build && pnpm run lint --fix",
+    "lint": "eslint src/**/*.ts"
+  },
+  "repository": "https://github.com/pnpm/pnpm/blob/main/patching/apply-patch",
+  "keywords": [
+    "pnpm7",
+    "pnpm",
+    "patch"
+  ],
+  "license": "MIT",
+  "bugs": {
+    "url": "https://github.com/pnpm/pnpm/issues"
+  },
+  "homepage": "https://github.com/pnpm/pnpm/blob/main/patching/apply-patch#readme",
+  "dependencies": {
+    "@pnpm/error": "workspace:*",
+    "patch-package": "^6.5.1"
+  },
+  "devDependencies": {
+    "@pnpm/patching.apply-patch": "workspace:*"
+  },
+  "funding": "https://opencollective.com/pnpm",
+  "exports": {
+    ".": "./lib/index.js"
+  }
+}

patching/apply-patch/src/index.ts

@@ -0,0 +1,22 @@
+import { PnpmError } from '@pnpm/error'
+import { applyPatch } from 'patch-package/dist/applyPatches'
+
+export interface ApplyPatchToDirOpts {
+  patchedDir: string
+  patchFilePath: string
+}
+
+export function applyPatchToDir (opts: ApplyPatchToDirOpts) {
+  // Ideally, we would just run "patch" or "git apply".
+  // However, "patch" is not available on Windows and "git apply" is hard to execute on a subdirectory of an existing repository
+  const cwd = process.cwd()
+  process.chdir(opts.patchedDir)
+  const success = applyPatch({
+    patchDir: opts.patchedDir,
+    patchFilePath: opts.patchFilePath,
+  })
+  process.chdir(cwd)
+  if (!success) {
+    throw new PnpmError('PATCH_FAILED', `Could not apply patch ${opts.patchFilePath} to ${opts.patchedDir}`)
+  }
+}

patching/apply-patch/tsconfig.json

@@ -0,0 +1,16 @@
+{
+  "extends": "@pnpm/tsconfig",
+  "compilerOptions": {
+    "outDir": "lib",
+    "rootDir": "src"
+  },
+  "include": [
+    "src/**/*.ts",
+    "../../__typings__/**/*.d.ts"
+  ],
+  "references": [
+    {
+      "path": "../../packages/error"
+    }
+  ]
+}

patching/apply-patch/tsconfig.lint.json

@@ -0,0 +1,8 @@
+{
+  "extends": "./tsconfig.json",
+  "include": [
+    "src/**/*.ts",
+    "test/**/*.ts",
+    "../../__typings__/**/*.d.ts"
+  ]
+}

pnpm-lock.yaml

@@ -897,9 +897,6 @@ importers:
       '@pnpm/core-loggers':
         specifier: workspace:*
         version: link:../../packages/core-loggers
-      '@pnpm/error':
-        specifier: workspace:*
-        version: link:../../packages/error
       '@pnpm/fs.hard-link-dir':
         specifier: workspace:*
         version: link:../../fs/hard-link-dir
@@ -915,6 +912,9 @@ importers:
       '@pnpm/logger':
         specifier: ^5.0.0
         version: 5.0.0
+      '@pnpm/patching.apply-patch':
+        specifier: workspace:*
+        version: link:../../patching/apply-patch
       '@pnpm/read-package-json':
         specifier: workspace:*
         version: link:../../pkg-manifest/read-package-json
@@ -927,9 +927,6 @@ importers:
       p-defer:
         specifier: ^3.0.0
         version: 3.0.0
-      patch-package:
-        specifier: ^6.5.1
-        version: 6.5.1
       ramda:
         specifier: npm:@pnpm/ramda@0.28.1
         version: /@pnpm/ramda@0.28.1
@@ -2394,6 +2391,9 @@ importers:
       '@pnpm/parse-wanted-dependency':
         specifier: workspace:*
         version: link:../parse-wanted-dependency
+      '@pnpm/patching.apply-patch':
+        specifier: workspace:*
+        version: link:../../patching/apply-patch
       '@pnpm/pick-registry-for-package':
         specifier: workspace:*
         version: link:../../config/pick-registry-for-package
@@ -2504,6 +2504,19 @@ importers:
         specifier: workspace:*
         version: 'link:'
 
+  patching/apply-patch:
+    dependencies:
+      '@pnpm/error':
+        specifier: workspace:*
+        version: link:../../packages/error
+      patch-package:
+        specifier: ^6.5.1
+        version: 6.5.1
+    devDependencies:
+      '@pnpm/patching.apply-patch':
+        specifier: workspace:*
+        version: 'link:'
+
   pkg-manager/client:
     dependencies:
       '@pnpm/default-resolver':

pnpm-workspace.yaml

@@ -14,6 +14,7 @@ packages:
   - packages/*
   - pkg-manager/*
   - pkg-manifest/*
+  - patching/*
   - pnpm
   - pnpm/artifacts/*
   - releasing/*