fix(patch): allow to edit a package in any directory (#5331)

close #5330

ref #5293

.changeset/fluffy-crews-join.md

@@ -0,0 +1,6 @@
+---
+"@pnpm/plugin-commands-patching": patch
+"pnpm": patch
+---
+
+Fix `pnpm patch` using a custom `--edit-dir`.

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

@@ -1,18 +1,15 @@
 import fs from 'fs'
-import path from 'path'
 import { docsUrl } from '@pnpm/cli-utils'
 import { Config, types as allTypes } from '@pnpm/config'
 import { LogBase } from '@pnpm/logger'
 import {
-  createOrConnectStoreController,
   CreateStoreControllerOptions,
 } from '@pnpm/store-connection-manager'
-import parseWantedDependency from '@pnpm/parse-wanted-dependency'
 import pick from 'ramda/src/pick'
-import pickRegistryForPackage from '@pnpm/pick-registry-for-package'
 import renderHelp from 'render-help'
 import tempy from 'tempy'
 import PnpmError from '@pnpm/error'
+import { writePackage } from './writePackage'
 
 export function rcOptionsTypes () {
   return pick([], allTypes)
@@ -51,38 +48,10 @@ export type PatchCommandOptions = Pick<Config, 'dir' | 'registries' | 'tag' | 's
 }
 
 export async function handler (opts: PatchCommandOptions, params: string[]) {
-  const store = await createOrConnectStoreController({
-    ...opts,
-    packageImportMethod: 'clone-or-copy',
-  })
-  const dep = parseWantedDependency(params[0])
-  const pkgResponse = await store.ctrl.requestPackage(dep, {
-    downloadPriority: 1,
-    lockfileDir: opts.dir,
-    preferredVersions: {},
-    projectDir: opts.dir,
-    registry: (dep.alias && pickRegistryForPackage(opts.registries, dep.alias)) ?? opts.registries.default,
-  })
-  const filesResponse = await pkgResponse.files!()
-  const tempDir = tempy.directory()
-  const userChangesDir = opts.editDir ? createPackageDirectory(opts.editDir) : path.join(tempDir, 'user')
-  await Promise.all([
-    store.ctrl.importPackage(path.join(tempDir, 'source'), {
-      filesResponse,
-      force: true,
-    }),
-    store.ctrl.importPackage(userChangesDir, {
-      filesResponse,
-      force: true,
-    }),
-  ])
-  return `You can now edit the following folder: ${userChangesDir}`
-}
-
-function createPackageDirectory (editDir: string) {
-  if (fs.existsSync(editDir)) {
-    throw new PnpmError('PATCH_EDIT_DIR_EXISTS', `The target directory already exists: '${editDir}'`)
+  if (opts.editDir && fs.existsSync(opts.editDir) && fs.readdirSync(opts.editDir).length > 0) {
+    throw new PnpmError('PATCH_EDIT_DIR_EXISTS', `The target directory already exists: '${opts.editDir}'`)
   }
-  fs.mkdirSync(editDir, { recursive: true })
-  return editDir
+  const editDir = opts.editDir ?? tempy.directory()
+  await writePackage(params[0], editDir, opts)
+  return `You can now edit the following folder: ${editDir}`
 }

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

@@ -9,6 +9,8 @@ import pick from 'ramda/src/pick'
 import execa from 'safe-execa'
 import escapeStringRegexp from 'escape-string-regexp'
 import renderHelp from 'render-help'
+import tempy from 'tempy'
+import { writePackage } from './writePackage'
 
 export const rcOptionsTypes = cliOptionsTypes
 
@@ -29,13 +31,14 @@ export function help () {
 
 export async function handler (opts: install.InstallCommandOptions, params: string[]) {
   const userDir = params[0]
-  const srcDir = path.join(userDir, '../source')
-  const patchContent = await diffFolders(srcDir, userDir)
   const lockfileDir = opts.lockfileDir ?? opts.dir ?? process.cwd()
   const patchesDir = path.join(lockfileDir, 'patches')
   await fs.promises.mkdir(patchesDir, { recursive: true })
-  const patchedPkgManifest = await readPackageJsonFromDir(srcDir)
+  const patchedPkgManifest = await readPackageJsonFromDir(userDir)
   const pkgNameAndVersion = `${patchedPkgManifest.name}@${patchedPkgManifest.version}`
+  const srcDir = tempy.directory()
+  await writePackage(pkgNameAndVersion, srcDir, opts)
+  const patchContent = await diffFolders(srcDir, userDir)
   const patchFileName = pkgNameAndVersion.replace('/', '__')
   await fs.promises.writeFile(path.join(patchesDir, `${patchFileName}.patch`), patchContent, 'utf8')
   let { manifest, writeProjectManifest } = await tryReadProjectManifest(lockfileDir)

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

@@ -0,0 +1,29 @@
+import { Config } from '@pnpm/config'
+import {
+  createOrConnectStoreController,
+  CreateStoreControllerOptions,
+} from '@pnpm/store-connection-manager'
+import pickRegistryForPackage from '@pnpm/pick-registry-for-package'
+import parseWantedDependency from '@pnpm/parse-wanted-dependency'
+
+export type WritePackageOptions = CreateStoreControllerOptions & Pick<Config, 'registries'>
+
+export async function writePackage (pkg: string, dest: string, opts: WritePackageOptions) {
+  const dep = parseWantedDependency(pkg)
+  const store = await createOrConnectStoreController({
+    ...opts,
+    packageImportMethod: 'clone-or-copy',
+  })
+  const pkgResponse = await store.ctrl.requestPackage(dep, {
+    downloadPriority: 1,
+    lockfileDir: opts.dir,
+    preferredVersions: {},
+    projectDir: opts.dir,
+    registry: (dep.alias && pickRegistryForPackage(opts.registries, dep.alias)) ?? opts.registries.default,
+  })
+  const filesResponse = await pkgResponse.files!()
+  await store.ctrl.importPackage(dest, {
+    filesResponse,
+    force: true,
+  })
+}

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

@@ -10,7 +10,6 @@ import { DEFAULT_OPTS } from './utils/index'
 
 describe('patch and commit', () => {
   let defaultPatchOption: patch.PatchCommandOptions
-  const tempySpy = jest.spyOn(tempy, 'directory')
 
   beforeEach(() => {
     prepare({
@@ -37,24 +36,23 @@ describe('patch and commit', () => {
 
   test('patch and commit', async () => {
     const output = await patch.handler(defaultPatchOption, ['is-positive@1.0.0'])
-    const userPatchDir = output.substring(output.indexOf(':') + 1).trim()
+    const patchDir = output.substring(output.indexOf(':') + 1).trim()
     const tempDir = os.tmpdir() // temp dir depends on the operating system (@see tempy)
 
-    // store patch files(user, source) in temporary directory when not given editDir option
-    expect(userPatchDir).toContain(tempDir)
-    expect(fs.existsSync(userPatchDir)).toBe(true)
-    expect(fs.existsSync(userPatchDir.replace('/user', '/source'))).toBe(true)
+    // store patch files in a temporary directory when not given editDir option
+    expect(patchDir).toContain(tempDir)
+    expect(fs.existsSync(patchDir)).toBe(true)
 
     // sanity check to ensure that the license file contains the expected string
-    expect(fs.readFileSync(path.join(userPatchDir, 'license'), 'utf8')).toContain('The MIT License (MIT)')
+    expect(fs.readFileSync(path.join(patchDir, 'license'), 'utf8')).toContain('The MIT License (MIT)')
 
-    fs.appendFileSync(path.join(userPatchDir, 'index.js'), '// test patching', 'utf8')
-    fs.unlinkSync(path.join(userPatchDir, 'license'))
+    fs.appendFileSync(path.join(patchDir, 'index.js'), '// test patching', 'utf8')
+    fs.unlinkSync(path.join(patchDir, 'license'))
 
     await patchCommit.handler({
       ...DEFAULT_OPTS,
       dir: process.cwd(),
-    }, [userPatchDir])
+    }, [patchDir])
 
     const { manifest } = await readProjectManifest(process.cwd())
     expect(manifest.pnpm?.patchedDependencies).toStrictEqual({
@@ -69,18 +67,30 @@ describe('patch and commit', () => {
     expect(fs.existsSync('node_modules/is-positive/license')).toBe(false)
   })
 
-  test('store source files in temporary directory and user files in user directory, when given editDir option', async () => {
-    const editDir = 'test/user/is-positive'
+  test('patch and commit with a custom edit dir', async () => {
+    const editDir = path.join(tempy.directory())
 
-    const patchFn = async () => patch.handler({ ...defaultPatchOption, editDir }, ['is-positive@1.0.0'])
-    const output = await patchFn()
-    const userPatchDir = output.substring(output.indexOf(':') + 1).trim()
+    const output = await patch.handler({ ...defaultPatchOption, editDir }, ['is-positive@1.0.0'])
+    const patchDir = output.substring(output.indexOf(':') + 1).trim()
 
-    expect(userPatchDir).toBe(editDir)
-    expect(fs.existsSync(userPatchDir)).toBe(true)
-    expect(fs.existsSync(path.join(tempySpy.mock.results[0].value, '/source'))).toBe(true)
+    expect(patchDir).toBe(editDir)
+    expect(fs.existsSync(patchDir)).toBe(true)
 
-    // If editDir already exists, it should throw an error
-    await expect(patchFn()).rejects.toThrow(`The target directory already exists: '${editDir}'`)
+    fs.appendFileSync(path.join(patchDir, 'index.js'), '// test patching', 'utf8')
+
+    await patchCommit.handler({
+      ...DEFAULT_OPTS,
+      dir: process.cwd(),
+    }, [patchDir])
+
+    expect(fs.readFileSync('node_modules/is-positive/index.js', 'utf8')).toContain('// test patching')
+  })
+
+  test('patch throws an error if the edit-dir already exists and is not empty', async () => {
+    const editDir = tempy.directory()
+    fs.writeFileSync(path.join(editDir, 'test.txt'), '', 'utf8')
+
+    await expect(() => patch.handler({ ...defaultPatchOption, editDir }, ['is-positive@1.0.0']))
+      .rejects.toThrow(`The target directory already exists: '${editDir}'`)
   })
 })