fix: readPackage hook should not modify manifest

PR #2693
close #2164

Co-authored-by: qdl <levinqdl@magicengine.com.cn>
Co-authored-by: Zoltan Kochan <z@kochan.io>

.changeset/tasty-drinks-peel.md

@@ -0,0 +1,6 @@
+---
+"pnpm": patch
+"supi": patch
+---
+
+Changes that are made by the `readPackage` hook are not saved to the `package.json` files of projects.

.changeset/two-readers-whisper.md

@@ -0,0 +1,5 @@
+---
+"@pnpm/get-context": minor
+---
+
+Add `originalManifest` that stores the unmodified.

packages/get-context/src/index.ts

@@ -35,7 +35,7 @@ export interface PnpmContext<T> {
   projects: Array<{
     modulesDir: string,
     id: string,
-  } & T & Required<ProjectOptions>>,
+  } & HookOptions & T & Required<ProjectOptions>>,
   rootModulesDir: string,
   hoistPattern: string[] | undefined,
   hoistedModulesDir: string,
@@ -55,8 +55,12 @@ export interface ProjectOptions {
   rootDir: string,
 }
 
+interface HookOptions {
+  originalManifest?: ProjectManifest,
+}
+
 export default async function getContext<T> (
-  projects: (ProjectOptions & T)[],
+  projects: (ProjectOptions & HookOptions & T)[],
   opts: {
     force: boolean,
     forceNewModules?: boolean,
@@ -119,10 +123,10 @@ export default async function getContext<T> (
     })
   })
   if (opts.hooks?.readPackage) {
-    projects = projects.map((project) => ({
-      ...project,
-      manifest: opts.hooks!.readPackage!(project.manifest),
-    }))
+    for (const project of importersContext.projects) {
+      project.originalManifest = project.manifest
+      project.manifest = opts.hooks!.readPackage!(R.clone(project.manifest))
+    }
   }
 
   const extraBinPaths = [

packages/pnpm/test/hooks.ts

@@ -0,0 +1,80 @@
+import prepare, { preparePackages } from '@pnpm/prepare'
+import loadJsonFile = require('load-json-file')
+import fs = require('mz/fs')
+import path = require('path')
+import tape = require('tape')
+import promisifyTape from 'tape-promise'
+import writeYamlFile = require('write-yaml-file')
+import { execPnpm } from './utils'
+
+const test = promisifyTape(tape)
+
+test('readPackage hook in single project doesn\'t modify manifest', async (t) => {
+  const project = prepare(t)
+  const pnpmfile = `
+      module.exports = { hooks: { readPackage } }
+      function readPackage (pkg) {
+      pkg.dependencies = pkg.dependencies || {}
+      pkg.dependencies['dep-of-pkg-with-1-dep'] = '100.1.0'
+      return pkg
+      }
+  `
+  await fs.writeFile('pnpmfile.js', pnpmfile, 'utf8')
+  let pkg
+  await execPnpm(['add', 'is-positive@1.0.0'])
+  pkg = await loadJsonFile(path.resolve('package.json'))
+  t.deepEqual(pkg && pkg.dependencies, { 'is-positive': '1.0.0' }, 'add dependency & readPackage hook work')
+
+  await execPnpm(['update', 'is-positive@2.0.0'])
+  pkg = await loadJsonFile(path.resolve('package.json'))
+  t.deepEqual(pkg && pkg.dependencies, { 'is-positive': '2.0.0' }, 'update dependency & readPackage hook work')
+
+  await execPnpm(['install'])
+  pkg = await loadJsonFile(path.resolve('package.json'))
+  t.deepEqual(pkg && pkg.dependencies, { 'is-positive': '2.0.0' }, 'install & readPackage hook work')
+
+  await execPnpm(['remove', 'is-positive'])
+  pkg = await loadJsonFile(path.resolve('package.json'))
+  t.notOk(pkg.dependencies, 'remove & readPackage hook work')
+})
+
+test.skip('readPackage hook in monorepo doesn\'t modify manifest', async (t) => {
+  const projects = preparePackages(t, [
+    {
+      name: 'project-a',
+      version: '1.0.0',
+    },
+    {
+      name: 'project-b',
+      version: '1.0.0',
+    },
+  ])
+
+  const pnpmfile = `
+      module.exports = { hooks: { readPackage } }
+      function readPackage (pkg) {
+        pkg.dependencies = pkg.dependencies || {}
+        pkg.dependencies['dep-of-pkg-with-1-dep'] = '100.1.0'
+        return pkg
+      }
+    `
+  await fs.writeFile('pnpmfile.js', pnpmfile, 'utf8')
+  await writeYamlFile('pnpm-workspace.yaml', { packages: ['**', '!store/**'] })
+
+  let pkg
+  await execPnpm(['add', 'is-positive@1.0.0', '--filter', 'project-a'])
+  pkg = await loadJsonFile(path.resolve('project-a/package.json'))
+  t.deepEqual(pkg && pkg.dependencies, { 'is-positive': '1.0.0' }, 'add dependency & readPackage hook work')
+
+  await execPnpm(['update', 'is-positive@2.0.0', '--filter', 'project-a'])
+  pkg = await loadJsonFile(path.resolve('project-a/package.json'))
+  t.deepEqual(pkg && pkg.dependencies, { 'is-positive': '2.0.0' }, 'update dependency & readPackage hook work')
+
+  await execPnpm(['install', '--filter', 'project-a'])
+  pkg = await loadJsonFile(path.resolve('project-a/package.json'))
+  t.deepEqual(pkg && pkg.dependencies, { 'is-positive': '2.0.0' }, 'install & readPackage hook work')
+
+  await execPnpm(['remove', 'is-positive', '--filter', 'project-a'])
+  pkg = await loadJsonFile(path.resolve('project-a/package.json'))
+  t.notOk(pkg.dependencies, 'remove & readPackage hook work')
+})

packages/pnpm/test/index.ts

@@ -5,6 +5,7 @@ import './complete.test'
 import './formatError.test'
 import './getOptionType.test'
 import './help.spec'
+import './hooks'
 import './install'
 import './link'
 import './monorepo'

packages/supi/src/install/index.ts

@@ -529,6 +529,7 @@ export type ImporterToUpdate = {
   binsDir: string,
   id: string,
   manifest: ProjectManifest,
+  originalManifest?: ProjectManifest,
   modulesDir: string,
   rootDir: string,
   pruneDirectDependencies: boolean,
@@ -573,7 +574,8 @@ async function installInContext (
     projects
       .map(async (project) => {
         if (project.mutation !== 'uninstallSome') return
-        project.manifest = await removeDeps(project.manifest, project.dependencyNames, {
+        const field = project.originalManifest ? 'originalManifest' : 'manifest'
+        project[field] = await removeDeps(project[field] as ProjectManifest, project.dependencyNames, {
           prefix: project.rootDir,
           saveType: project.targetDependenciesField,
         })
@@ -638,13 +640,16 @@ async function installInContext (
 
   const projectsToLink = await Promise.all<ProjectToLink>(projectsToResolve.map(async (project, index) => {
     const resolvedImporter = resolvedImporters[project.id]
-    let newPkg: ProjectManifest | undefined = project.manifest
+    let updatedManifest: ProjectManifest | undefined = project.manifest
+    let updatedOriginalManifest: ProjectManifest | undefined = project.originalManifest
     if (project.updatePackageManifest) {
-      newPkg = await updateProjectManifest(projectsToResolve[index], {
+      const manifests = await updateProjectManifest(projectsToResolve[index], {
         directDependencies: resolvedImporter.directDependencies,
         preserveWorkspaceProtocol: opts.preserveWorkspaceProtocol,
         saveWorkspaceProtocol: opts.saveWorkspaceProtocol,
       })
+      updatedManifest = manifests[0]
+      updatedOriginalManifest = manifests[1]
     } else {
       packageManifestLogger.debug({
         prefix: project.rootDir,
@@ -652,10 +657,10 @@ async function installInContext (
       })
     }
 
-    if (newPkg) {
+    if (updatedManifest) {
       const projectSnapshot = ctx.wantedLockfile.importers[project.id]
       ctx.wantedLockfile.importers[project.id] = addDirectDependenciesToLockfile(
-        newPkg,
+        updatedManifest,
         projectSnapshot,
         resolvedImporter.linkedDependencies,
         resolvedImporter.directDependencies,
@@ -680,7 +685,7 @@ async function installInContext (
       directNodeIdsByAlias: resolvedImporter.directNodeIdsByAlias,
       id: project.id,
       linkedDependencies: resolvedImporter.linkedDependencies,
-      manifest: newPkg || project.manifest,
+      manifest: updatedOriginalManifest ?? project.originalManifest ?? project.manifest,
       modulesDir: project.modulesDir,
       pruneDirectDependencies: project.pruneDirectDependencies,
       removePackages: project.removePackages,

packages/supi/src/utils/getPref.ts

@@ -35,12 +35,19 @@ export async function updateProjectManifest (
       })
     }
   }
-  return save(
+  const hookedManifest = await save(
     importer.rootDir,
     importer.manifest,
     specsToUpsert,
     { dryRun: true }
   )
+  const originalManifest = importer.originalManifest && await save(
+    importer.rootDir,
+    importer.originalManifest,
+    specsToUpsert,
+    { dryRun: true }
+  )
+  return [hookedManifest, originalManifest]
 }
 
 function resolvedDirectDepToSpecObject (