fix: patch package even if it is not in the onlyBuiltDependencies list (#4925)

close #4914

.changeset/nasty-shoes-attack.md

@@ -0,0 +1,5 @@
+---
+"@pnpm/lockfile-types": minor
+---
+
+Add optional "patched" field to package object in the lockfile.

packages/build-modules/src/buildSequence.ts

@@ -44,18 +44,20 @@ export default function buildSequence (
 }
 
 function getSubgraphToBuild (
-  graph: Record<string, Pick<DependenciesGraphNode, 'children' | 'requiresBuild'>>,
+  graph: Record<string, Pick<DependenciesGraphNode, 'children' | 'requiresBuild' | 'patchFile'>>,
   entryNodes: string[],
   nodesToBuild: Set<string>,
   walked: Set<string>
 ) {
   let currentShouldBeBuilt = false
   for (const depPath of entryNodes) {
-    if (!graph[depPath]) continue // packages that are already in node_modules are skipped
+    const node = graph[depPath]
+    if (!node) continue // packages that are already in node_modules are skipped
     if (walked.has(depPath)) continue
     walked.add(depPath)
-    const childShouldBeBuilt = getSubgraphToBuild(graph, Object.values(graph[depPath].children), nodesToBuild, walked) ||
-      graph[depPath].requiresBuild
+    const childShouldBeBuilt = getSubgraphToBuild(graph, Object.values(node.children), nodesToBuild, walked) ||
+      node.requiresBuild ||
+      node.patchFile != null
     if (childShouldBeBuilt) {
       nodesToBuild.add(depPath)
       currentShouldBeBuilt = true

packages/build-modules/src/index.ts

@@ -43,7 +43,10 @@ export default async (
   const buildDepOpts = { ...opts, warn }
   const chunks = buildSequence(depGraph, rootDepPaths)
   const groups = chunks.map((chunk) => {
-    chunk = chunk.filter((depPath) => depGraph[depPath].requiresBuild && !depGraph[depPath].isBuilt)
+    chunk = chunk.filter((depPath) => {
+      const node = depGraph[depPath]
+      return (node.requiresBuild || node.patchFile != null) && !node.isBuilt
+    })
     if (opts.depsToBuild != null) {
       chunk = chunk.filter((depPath) => opts.depsToBuild!.has(depPath))
     }
@@ -102,7 +105,7 @@ async function buildDependency (
       try {
         const sideEffectsCacheKey = calcDepState(depGraph, opts.depsStateCache, depPath, {
           patchFileHash: depNode.patchFile?.hash,
-          ignoreScripts: Boolean(opts.ignoreScripts),
+          isBuilt: hasSideEffects,
         })
         await opts.storeController.upload(depNode.dir, {
           sideEffectsCacheKey,

packages/calc-dep-state/src/index.ts

@@ -24,11 +24,11 @@ export function calcDepState (
   depPath: string,
   opts: {
     patchFileHash?: string
-    ignoreScripts: boolean
+    isBuilt: boolean
   }
 ): string {
   let result = ENGINE_NAME
-  if (!opts.ignoreScripts) {
+  if (opts.isBuilt) {
     const depStateObj = calcDepStateObj(depPath, depsGraph, cache, new Set())
     result += `-${JSON.stringify(depStateObj)}`
   }

packages/calc-dep-state/test/index.ts

@@ -18,12 +18,12 @@ const depsGraph = {
 
 test('calcDepState()', () => {
   expect(calcDepState(depsGraph, {}, '/registry/foo/1.0.0', {
-    ignoreScripts: false,
+    isBuilt: true,
   })).toBe(`${ENGINE_NAME}-{}`)
 })
 
 test('calcDepState() when scripts are ignored', () => {
   expect(calcDepState(depsGraph, {}, '/registry/foo/1.0.0', {
-    ignoreScripts: true,
+    isBuilt: false,
   })).toBe(ENGINE_NAME)
 })

packages/core/src/install/link.ts

@@ -409,21 +409,21 @@ async function linkAllPkgs (
     depNodes.map(async (depNode) => {
       const filesResponse = await depNode.fetchingFiles()
 
+      if (typeof depNode.requiresBuild === 'function') {
+        depNode.requiresBuild = await depNode.requiresBuild()
+      }
       let sideEffectsCacheKey: string | undefined
       if (opts.sideEffectsCacheRead && filesResponse.sideEffects && !isEmpty(filesResponse.sideEffects)) {
         sideEffectsCacheKey = calcDepState(opts.depGraph, opts.depsStateCache, depNode.depPath, {
-          ignoreScripts: opts.ignoreScripts,
+          isBuilt: !opts.ignoreScripts && depNode.requiresBuild,
           patchFileHash: depNode.patchFile?.hash,
         })
       }
-      if (typeof depNode.requiresBuild === 'function') {
-        depNode.requiresBuild = await depNode.requiresBuild()
-      }
       const { importMethod, isBuilt } = await storeController.importPackage(depNode.dir, {
         filesResponse,
         force: opts.force,
         sideEffectsCacheKey,
-        requiresBuild: depNode.requiresBuild,
+        requiresBuild: depNode.requiresBuild || depNode.patchFile != null,
       })
       if (importMethod) {
         progressLogger.debug({

packages/core/test/install/patch.ts

@@ -44,7 +44,7 @@ test('patch package', async () => {
 
   const filesIndexFile = path.join(opts.storeDir, 'files/c7/1ccf199e0fdae37aad13946b937d67bcd35fa111b84d21b3a19439cfdc2812c5d8da8a735e94c2a1ccb77b4583808ee8405313951e7146ac83ede3671dc292-index.json')
   const filesIndex = await loadJsonFile<PackageFilesIndex>(filesIndexFile)
-  const sideEffectsKey = `${ENGINE_NAME}-{}-${patchFileHash}`
+  const sideEffectsKey = `${ENGINE_NAME}-${patchFileHash}`
   const patchedFileIntegrity = filesIndex.sideEffects?.[sideEffectsKey]['index.js']?.integrity
   expect(patchedFileIntegrity).toBeTruthy()
   const originalFileIntegrity = filesIndex.files['index.js'].integrity
@@ -233,3 +233,90 @@ test('patch package when scripts are ignored', async () => {
   // The original file did not break, when a patched version was created
   expect(fs.readFileSync('node_modules/is-positive/index.js', 'utf8')).not.toContain('// patched')
 })
+
+test('patch package when the package is not in onlyBuiltDependencies list', async () => {
+  const project = prepareEmpty()
+  const patchPath = path.join(f.find('patch-pkg'), 'is-positive@1.0.0.patch')
+
+  const patchedDependencies = {
+    'is-positive@1.0.0': path.relative(process.cwd(), patchPath),
+  }
+  const opts = await testDefaults({
+    fastUnpack: false,
+    sideEffectsCacheRead: true,
+    sideEffectsCacheWrite: true,
+    patchedDependencies,
+    onlyBuiltDependencies: [],
+  }, {}, {}, { packageImportMethod: 'hardlink' })
+  await install({
+    dependencies: {
+      'is-positive': '1.0.0',
+    },
+  }, opts)
+
+  expect(fs.readFileSync('node_modules/is-positive/index.js', 'utf8')).toContain('// patched')
+
+  const patchFileHash = 'jnbpamcxayl5i4ehrkoext3any'
+  const lockfile = await project.readLockfile()
+  expect(lockfile.patchedDependencies).toStrictEqual({
+    'is-positive@1.0.0': {
+      path: patchedDependencies['is-positive@1.0.0'],
+      hash: patchFileHash,
+    },
+  })
+  expect(lockfile.packages[`/is-positive/1.0.0_${patchFileHash}`]).toBeTruthy()
+
+  const filesIndexFile = path.join(opts.storeDir, 'files/c7/1ccf199e0fdae37aad13946b937d67bcd35fa111b84d21b3a19439cfdc2812c5d8da8a735e94c2a1ccb77b4583808ee8405313951e7146ac83ede3671dc292-index.json')
+  const filesIndex = await loadJsonFile<PackageFilesIndex>(filesIndexFile)
+  const sideEffectsKey = `${ENGINE_NAME}-${patchFileHash}`
+  const patchedFileIntegrity = filesIndex.sideEffects?.[sideEffectsKey]['index.js']?.integrity
+  expect(patchedFileIntegrity).toBeTruthy()
+  const originalFileIntegrity = filesIndex.files['index.js'].integrity
+  expect(originalFileIntegrity).toBeTruthy()
+  // The integrity of the original file differs from the integrity of the patched file
+  expect(originalFileIntegrity).not.toEqual(patchedFileIntegrity)
+
+  // The same with frozen lockfile
+  await rimraf('node_modules')
+  await install({
+    dependencies: {
+      'is-positive': '1.0.0',
+    },
+  }, {
+    ...opts,
+    frozenLockfile: true,
+  })
+  expect(fs.readFileSync('node_modules/is-positive/index.js', 'utf8')).toContain('// patched')
+
+  // The same with frozen lockfile and hoisted node_modules
+  await rimraf('node_modules')
+  await install({
+    dependencies: {
+      'is-positive': '1.0.0',
+    },
+  }, {
+    ...opts,
+    frozenLockfile: true,
+    nodeLinker: 'hoisted',
+  })
+  expect(fs.readFileSync('node_modules/is-positive/index.js', 'utf8')).toContain('// patched')
+
+  process.chdir('..')
+  fs.mkdirSync('project2')
+  process.chdir('project2')
+
+  await install({
+    dependencies: {
+      'is-positive': '1.0.0',
+    },
+  }, await testDefaults({
+    fastUnpack: false,
+    sideEffectsCacheRead: true,
+    sideEffectsCacheWrite: true,
+    onlyBuiltDependencies: [],
+    offline: true,
+  }, {}, {}, { packageImportMethod: 'hardlink' }))
+
+  // The original file did not break, when a patched version was created
+  expect(fs.readFileSync('node_modules/is-positive/index.js', 'utf8')).not.toContain('// patched')
+})

packages/headless/src/index.ts

@@ -689,14 +689,14 @@ async function linkAllPkgs (
       let sideEffectsCacheKey: string | undefined
       if (opts.sideEffectsCacheRead && filesResponse.sideEffects && !isEmpty(filesResponse.sideEffects)) {
         sideEffectsCacheKey = calcDepState(opts.depGraph, opts.depsStateCache, depNode.dir, {
-          ignoreScripts: opts.ignoreScripts,
+          isBuilt: !opts.ignoreScripts && depNode.requiresBuild,
           patchFileHash: depNode.patchFile?.hash,
         })
       }
       const { importMethod, isBuilt } = await storeController.importPackage(depNode.dir, {
         filesResponse,
         force: opts.force,
-        requiresBuild: depNode.requiresBuild,
+        requiresBuild: depNode.requiresBuild || depNode.patchFile != null,
         sideEffectsCacheKey,
       })
       if (importMethod) {

packages/headless/src/linkHoistedModules.ts

@@ -104,14 +104,14 @@ async function linkAllPkgsInOrder (
       let sideEffectsCacheKey: string | undefined
       if (opts.sideEffectsCacheRead && filesResponse.sideEffects && !isEmpty(filesResponse.sideEffects)) {
         sideEffectsCacheKey = _calcDepState(dir, {
+          isBuilt: !opts.ignoreScripts && depNode.requiresBuild,
           patchFileHash: depNode.patchFile?.hash,
-          ignoreScripts: opts.ignoreScripts,
         })
       }
       const { importMethod, isBuilt } = await storeController.importPackage(depNode.dir, {
         filesResponse,
         force: opts.force || depNode.depPath !== prevGraph[dir]?.depPath,
-        requiresBuild: depNode.requiresBuild,
+        requiresBuild: depNode.requiresBuild || depNode.patchFile != null,
         sideEffectsCacheKey,
       })
       if (importMethod) {

packages/lockfile-types/src/index.ts

@@ -69,6 +69,7 @@ export interface PackageSnapshot {
   dev?: true | false
   optional?: true
   requiresBuild?: true
+  patched?: true
   prepare?: true
   hasBin?: true
   // name and version are only needed

packages/resolve-dependencies/src/index.ts

@@ -271,8 +271,7 @@ async function finishLockfileUpdates (
           Boolean(pkgJson.scripts.postinstall)
         ) ||
         filesResponse.filesIndex['binding.gyp'] ||
-          Object.keys(filesResponse.filesIndex).some((filename) => !(filename.match(/^[.]hooks[\\/]/) == null)) || // TODO: optimize this
-        depNode.patchFile != null
+          Object.keys(filesResponse.filesIndex).some((filename) => !(filename.match(/^[.]hooks[\\/]/) == null)) // TODO: optimize this
       )
     } else {
       // This should never ever happen

packages/resolve-dependencies/src/updateLockfile.ts

@@ -156,6 +156,9 @@ function toLockfileDependency (
   if (pkg.hasBin) {
     result['hasBin'] = true
   }
+  if (pkg.patchFile) {
+    result['patched'] = true
+  }
   const requiresBuildIsKnown = typeof pkg.requiresBuild === 'boolean'
   let pending = false
   if (requiresBuildIsKnown) {