feat(patching): move edit dir to node_modules (#8379)

---------

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

.changeset/five-poems-teach.md

@@ -0,0 +1,6 @@
+---
+"@pnpm/plugin-commands-patching": minor
+"pnpm": minor
+---
+
+Change the default edit dir location when running `pnpm patch` from a temporary directory to `node_modules/.pnpm_patches/pkg[@version]` to allow the code editor to open the edit dir in the same file tree as the main project.

cspell.json

@@ -28,6 +28,7 @@
     "certfile",
     "clonedeep",
     "cmds",
+    "codeload",
     "colorterm",
     "comver",
     "copyfiles",

patching/plugin-commands-patching/package.json

@@ -63,6 +63,7 @@
     "enquirer": "catalog:",
     "escape-string-regexp": "catalog:",
     "fast-glob": "catalog:",
+    "make-empty-dir": "catalog:",
     "normalize-path": "catalog:",
     "ramda": "catalog:",
     "realpath-missing": "catalog:",

patching/plugin-commands-patching/src/getEditDirPath.ts

@@ -0,0 +1,24 @@
+import path from 'path'
+import { type ParseWantedDependencyResult } from '@pnpm/parse-wanted-dependency'
+
+export interface GetEditDirOptions {
+  modulesDir: string
+}
+
+export function getEditDirPath (param: string, patchedDep: ParseWantedDependencyResult, opts: GetEditDirOptions): string {
+  const editDirName = getEditDirNameFromParsedDep(patchedDep) ?? param
+  return path.join(opts.modulesDir, '.pnpm_patches', editDirName)
+}
+
+function getEditDirNameFromParsedDep (patchedDep: ParseWantedDependencyResult): string | undefined {
+  if (patchedDep.alias && patchedDep.pref) {
+    const pref = patchedDep.pref.replace(/[\\/:*?"<>|]+/g, '+')
+    return `${patchedDep.alias}@${pref}`
+  }
+
+  if (patchedDep.alias) {
+    return patchedDep.alias
+  }
+
+  return undefined
+}

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

@@ -11,10 +11,10 @@ import pick from 'ramda/src/pick'
 import renderHelp from 'render-help'
 import chalk from 'chalk'
 import terminalLink from 'terminal-link'
-import tempy from 'tempy'
 import { PnpmError } from '@pnpm/error'
 import { type ParseWantedDependencyResult } from '@pnpm/parse-wanted-dependency'
 import { writePackage } from './writePackage'
+import { getEditDirPath } from './getEditDirPath'
 import { getPatchedDependency } from './getPatchedDependency'
 import { writeEditDirState } from './stateFile'
 import { tryReadProjectManifest } from '@pnpm/read-project-manifest'
@@ -77,7 +77,6 @@ export async function handler (opts: PatchCommandOptions, params: string[]): Pro
   if (!params[0]) {
     throw new PnpmError('MISSING_PACKAGE_NAME', '`pnpm patch` requires the package name')
   }
-  const editDir = opts.editDir ?? tempy.directory()
   const lockfileDir = opts.lockfileDir ?? opts.dir ?? process.cwd()
   const patchedDep = await getPatchedDependency(params[0], {
     lockfileDir,
@@ -85,6 +84,15 @@ export async function handler (opts: PatchCommandOptions, params: string[]): Pro
     virtualStoreDir: opts.virtualStoreDir,
   })
 
+  const modulesDir = path.join(lockfileDir, opts.modulesDir ?? 'node_modules')
+  const editDir = opts.editDir ? opts.editDir : getEditDirPath(params[0], patchedDep, { modulesDir })
+
+  if (fs.existsSync(editDir) && fs.readdirSync(editDir).length !== 0) {
+    throw new PnpmError('EDIT_DIR_NOT_EMPTY', `The directory ${editDir} is not empty`, {
+      hint: 'Either run `pnpm patch-commit` to commit or delete it then run `pnpm patch` to recreate it',
+    })
+  }
+
   await writePackage(patchedDep, editDir, opts)
 
   writeEditDirState({

patching/plugin-commands-patching/src/patchCommit.ts

@@ -14,6 +14,7 @@ import pick from 'ramda/src/pick'
 import equals from 'ramda/src/equals'
 import execa from 'safe-execa'
 import escapeStringRegexp from 'escape-string-regexp'
+import makeEmptyDir from 'make-empty-dir'
 import renderHelp from 'render-help'
 import tempy from 'tempy'
 import { writePackage } from './writePackage'
@@ -85,6 +86,9 @@ export async function handler (opts: PatchCommitCommandOptions, params: string[]
   })
   const patchedPkgDir = await preparePkgFilesForDiff(userDir)
   const patchContent = await diffFolders(srcDir, patchedPkgDir)
+  if (patchedPkgDir !== userDir) {
+    fs.rmSync(patchedPkgDir, { recursive: true })
+  }
 
   if (!patchContent.length) {
     return `No changes were found to the following directory: ${userDir}`
@@ -186,7 +190,8 @@ async function preparePkgFilesForDiff (src: string): Promise<string> {
   if (await areAllFilesInPkg(files, src)) {
     return src
   }
-  const dest = tempy.directory()
+  const dest = `${src}_tmp`
+  await makeEmptyDir(dest)
   await Promise.all(
     files.map(async (file) => {
       const srcFile = path.join(src, file)

patching/plugin-commands-patching/test/getEditDirPath.test.ts

@@ -0,0 +1,37 @@
+import path from 'path'
+import { getEditDirPath } from '../src/getEditDirPath'
+
+test('getEditDirPath() returns path to pkg@version inside node_modules/.pnpm_patches', () => {
+  expect(getEditDirPath('pkg', {
+    alias: 'pkg',
+    pref: '0.1.2',
+  }, { modulesDir: 'node_modules' })).toBe(path.join('node_modules', '.pnpm_patches', 'pkg@0.1.2'))
+})
+
+test('getEditDirPath() returns path to pkg@version inside .pnpm_patches inside specified modules dir', () => {
+  expect(getEditDirPath('pkg', {
+    alias: 'pkg',
+    pref: '0.1.2',
+  }, {
+    modulesDir: 'user-defined-modules-dir',
+  })).toBe(path.join('user-defined-modules-dir', '.pnpm_patches', 'pkg@0.1.2'))
+})
+
+test('getEditDirPath() returns valid path even if pref contains special characters', () => {
+  expect(getEditDirPath('pkg', {
+    alias: 'pkg',
+    pref: 'https://codeload.github.com/zkochan/hi/tar.gz',
+  }, { modulesDir: 'node_modules' })).toBe(path.join('node_modules', '.pnpm_patches', 'pkg@https+codeload.github.com+zkochan+hi+tar.gz'))
+})
+
+test('getEditDirPath() returns path with name of alias if pref is not available', () => {
+  expect(getEditDirPath('pkg', {
+    alias: 'resolved-pkg',
+  }, { modulesDir: 'node_modules' })).toBe(path.join('node_modules', '.pnpm_patches', 'resolved-pkg'))
+})
+
+test('getEditDirPath() returns path with name of param if alias is not available', () => {
+  expect(getEditDirPath('pkg', {
+    pref: '0.1.2',
+  }, { modulesDir: 'node_modules' })).toBe(path.join('node_modules', '.pnpm_patches', 'pkg'))
+})

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

@@ -59,13 +59,21 @@ describe('patch and commit', () => {
     })
   })
 
+  test('patch throws an error when edit dir is not empty', async () => {
+    fs.mkdirSync('node_modules/.pnpm_patches/is-positive@1.0.0', { recursive: true })
+    fs.writeFileSync('node_modules/.pnpm_patches/is-positive@1.0.0/package.json', '{}')
+    await expect(patch.handler(defaultPatchOption, ['is-positive@1.0.0'])).rejects.toMatchObject({
+      code: 'ERR_PNPM_EDIT_DIR_NOT_EMPTY',
+    })
+  })
+
   test('patch and commit with exact version', async () => {
     const output = await patch.handler(defaultPatchOption, ['is-positive@1.0.0'])
     const patchDir = getPatchDirFromPatchOutput(output)
-    const tempDir = os.tmpdir() // temp dir depends on the operating system (@see tempy)
 
-    // store patch files in a temporary directory when not given editDir option
-    expect(patchDir).toContain(tempDir)
+    // store patch files in a directory inside modules dir when not given editDir option
+    expect(patchDir).toContain(path.join('node_modules', '.pnpm_patches', 'is-positive@1.0.0'))
+    expect(path.basename(patchDir)).toBe('is-positive@1.0.0')
     expect(fs.existsSync(patchDir)).toBe(true)
 
     // sanity check to ensure that the license file contains the expected string
@@ -100,10 +108,9 @@ describe('patch and commit', () => {
   test('patch and commit without exact version', async () => {
     const output = await patch.handler(defaultPatchOption, ['is-positive'])
     const patchDir = getPatchDirFromPatchOutput(output)
-    const tempDir = os.tmpdir() // temp dir depends on the operating system (@see tempy)
 
-    // store patch files in a temporary directory when not given editDir option
-    expect(patchDir).toContain(tempDir)
+    expect(patchDir).toContain(path.join('node_modules', '.pnpm_patches', 'is-positive@'))
+    expect(path.basename(patchDir)).toMatch(/^is-positive@[0-9]+\.[0-9]+\.[0-9]+$/)
     expect(fs.existsSync(patchDir)).toBe(true)
 
     // sanity check to ensure that the license file contains the expected string
@@ -138,10 +145,10 @@ describe('patch and commit', () => {
   test('patch and commit with filtered files', async () => {
     const output = await patch.handler(defaultPatchOption, ['is-positive@1.0.0'])
     const patchDir = getPatchDirFromPatchOutput(output)
-    const tempDir = os.tmpdir() // temp dir depends on the operating system (@see tempy)
 
-    // store patch files in a temporary directory when not given editDir option
-    expect(patchDir).toContain(tempDir)
+    // store patch files in a directory inside modules dir when not given editDir option
+    expect(patchDir).toContain(path.join('node_modules', '.pnpm_patches', 'is-positive@1.0.0'))
+    expect(path.basename(patchDir)).toBe('is-positive@1.0.0')
     expect(fs.existsSync(patchDir)).toBe(true)
 
     // sanity check to ensure that the license file contains the expected string
@@ -274,6 +281,7 @@ describe('patch and commit', () => {
     expect(fs.existsSync('patches/is-positive@1.0.0.patch')).toBe(true)
 
     // re-patch
+    fs.rmSync(patchDir, { recursive: true })
     output = await patch.handler({ ...defaultPatchOption, rootProjectManifest: manifest }, ['is-positive@1.0.0'])
     patchDir = getPatchDirFromPatchOutput(output)
 
@@ -324,6 +332,7 @@ describe('patch and commit', () => {
     expect(fs.existsSync('patches/is-positive@1.0.0.patch')).toBe(true)
 
     // re-patch with --ignore-patches
+    fs.rmSync(patchDir, { recursive: true })
     output = await patch.handler({ ...defaultPatchOption, ignoreExisting: true }, ['is-positive@1.0.0'])
     patchDir = getPatchDirFromPatchOutput(output)
 
@@ -350,8 +359,8 @@ describe('patch and commit', () => {
   test('patch package with installed version', async () => {
     const output = await patch.handler(defaultPatchOption, ['is-positive@1'])
     const patchDir = getPatchDirFromPatchOutput(output)
-    const tempDir = os.tmpdir()
-    expect(patchDir).toContain(tempDir)
+    expect(patchDir).toContain(path.join('node_modules', '.pnpm_patches', 'is-positive@1'))
+    expect(path.basename(patchDir)).toMatch(/^is-positive@1\.[0-9]+\.[0-9]+$/)
     expect(fs.existsSync(patchDir)).toBe(true)
     expect(JSON.parse(fs.readFileSync(path.join(patchDir, 'package.json'), 'utf8')).version).toBe('1.0.0')
   })
@@ -528,9 +537,9 @@ describe('prompt to choose version', () => {
     ]]])
 
     const patchDir = getPatchDirFromPatchOutput(output)
-    const tempDir = os.tmpdir()
 
-    expect(patchDir).toContain(tempDir)
+    expect(patchDir).toContain(path.join('node_modules', '.pnpm_patches', 'chalk@'))
+    expect(path.basename(patchDir)).toMatch(/^chalk@[0-9]+\.[0-9]+\.[0-9]+$/)
     expect(fs.existsSync(patchDir)).toBe(true)
     expect(JSON.parse(fs.readFileSync(path.join(patchDir, 'package.json'), 'utf8')).version).toBe('5.3.0')
     expect(fs.existsSync(path.join(patchDir, 'source/index.js'))).toBe(true)
@@ -595,9 +604,9 @@ describe('prompt to choose version', () => {
     ]]])
 
     const patchDir = getPatchDirFromPatchOutput(output)
-    const tempDir = os.tmpdir()
 
-    expect(patchDir).toContain(tempDir)
+    expect(patchDir).toContain(path.join('node_modules', '.pnpm_patches', 'chalk@'))
+    expect(path.basename(patchDir)).toMatch(/^chalk@[0-9]+\.[0-9]+\.[0-9]+$/)
     expect(fs.existsSync(patchDir)).toBe(true)
     expect(JSON.parse(fs.readFileSync(path.join(patchDir, 'package.json'), 'utf8')).version).toBe('5.3.0')
     expect(fs.existsSync(path.join(patchDir, 'source/index.js'))).toBe(true)
@@ -655,9 +664,9 @@ describe('patching should work when there is a no EOL in the patched file', () =
   it('should work when adding content on a newline', async () => {
     const output = await patch.handler(defaultPatchOption, ['safe-execa@0.1.2'])
     const userPatchDir = getPatchDirFromPatchOutput(output)
-    const tempDir = os.tmpdir()
 
-    expect(userPatchDir).toContain(tempDir)
+    expect(userPatchDir).toContain(path.join('node_modules', '.pnpm_patches', 'safe-execa@0.1.2'))
+    expect(path.basename(userPatchDir)).toBe('safe-execa@0.1.2')
     expect(fs.existsSync(userPatchDir)).toBe(true)
     expect(fs.existsSync(path.join(userPatchDir, 'lib/index.js'))).toBe(true)
 
@@ -684,9 +693,9 @@ describe('patching should work when there is a no EOL in the patched file', () =
   it('should work fine when new content is appended', async () => {
     const output = await patch.handler(defaultPatchOption, ['safe-execa@0.1.2'])
     const userPatchDir = getPatchDirFromPatchOutput(output)
-    const tempDir = os.tmpdir()
 
-    expect(userPatchDir).toContain(tempDir)
+    expect(userPatchDir).toContain(path.join('node_modules', '.pnpm_patches', 'safe-execa@0.1.2'))
+    expect(path.basename(userPatchDir)).toBe('safe-execa@0.1.2')
     expect(fs.existsSync(userPatchDir)).toBe(true)
     expect(fs.existsSync(path.join(userPatchDir, 'lib/index.js'))).toBe(true)
 
@@ -770,9 +779,9 @@ describe('patch and commit in workspaces', () => {
     })
     const output = await patch.handler(defaultPatchOption, ['is-positive@1.0.0'])
     const patchDir = getPatchDirFromPatchOutput(output)
-    const tempDir = os.tmpdir()
 
-    expect(patchDir).toContain(tempDir)
+    expect(patchDir).toContain(path.join('node_modules', '.pnpm_patches', 'is-positive@1.0.0'))
+    expect(path.basename(patchDir)).toBe('is-positive@1.0.0')
     expect(fs.existsSync(patchDir)).toBe(true)
 
     expect(fs.readFileSync(path.join(patchDir, 'license'), 'utf8')).toContain('The MIT License (MIT)')
@@ -831,9 +840,9 @@ describe('patch and commit in workspaces', () => {
       dir: process.cwd(),
     }, ['is-positive@1.0.0'])
     const patchDir = getPatchDirFromPatchOutput(output)
-    const tempDir = os.tmpdir()
 
-    expect(patchDir).toContain(tempDir)
+    expect(patchDir).toContain(path.join('node_modules', '.pnpm_patches', 'is-positive@1.0.0'))
+    expect(path.basename(patchDir)).toBe('is-positive@1.0.0')
     expect(fs.existsSync(patchDir)).toBe(true)
 
     expect(fs.readFileSync(path.join(patchDir, 'license'), 'utf8')).toContain('The MIT License (MIT)')
@@ -921,6 +930,7 @@ describe('patch and commit in workspaces', () => {
     expect(fs.existsSync('./node_modules/is-positive/license')).toBe(false)
 
     // re-patch project-1
+    fs.rmSync(patchDir, { recursive: true })
     output = await patch.handler({
       ...defaultPatchOption,
       dir: process.cwd(),
@@ -1029,8 +1039,6 @@ describe('patch with custom modules-dir and virtual-store-dir', () => {
   })
 
   test('should work with custom modules-dir and virtual-store-dir', async () => {
-    const manifest = fs.readFileSync(path.join(customModulesDirFixture, 'package.json'), 'utf8')
-    const lockfileYaml = fs.readFileSync(path.join(customModulesDirFixture, 'pnpm-lock.yaml'), 'utf8')
     const { allProjects, allProjectsGraph, selectedProjectsGraph } = await filterPackagesFromDir(customModulesDirFixture, [])
     await install.handler({
       ...DEFAULT_OPTS,
@@ -1048,8 +1056,8 @@ describe('patch with custom modules-dir and virtual-store-dir', () => {
     })
     const output = await patch.handler(defaultPatchOption, ['is-positive@1'])
     const patchDir = getPatchDirFromPatchOutput(output)
-    const tempDir = os.tmpdir()
-    expect(patchDir).toContain(tempDir)
+    expect(patchDir).toContain(path.join('fake_modules', '.pnpm_patches', 'is-positive@1'))
+    expect(path.basename(patchDir)).toMatch(/^is-positive@1\.[0-9]+\.[0-9]+$/)
     expect(fs.existsSync(patchDir)).toBe(true)
     expect(JSON.parse(fs.readFileSync(path.join(patchDir, 'package.json'), 'utf8')).version).toBe('1.0.0')
 
@@ -1071,9 +1079,6 @@ describe('patch with custom modules-dir and virtual-store-dir', () => {
       workspaceDir: customModulesDirFixture,
     }, [patchDir])
     expect(fs.readFileSync(path.join(customModulesDirFixture, 'packages/bar/fake_modules/is-positive/index.js'), 'utf8')).toContain('// test patching')
-    // restore package.json and package-lock.yaml
-    fs.writeFileSync(path.join(customModulesDirFixture, 'package.json'), manifest, 'utf8')
-    fs.writeFileSync(path.join(customModulesDirFixture, 'pnpm-lock.yaml'), lockfileYaml, 'utf8')
   })
 })
 

pnpm-lock.yaml

@@ -3765,6 +3765,9 @@ importers:
       fast-glob:
         specifier: 'catalog:'
         version: 3.3.2
+      make-empty-dir:
+        specifier: 'catalog:'
+        version: 3.0.2
       normalize-path:
         specifier: 'catalog:'
         version: 3.0.0