fix: don't fail when the patched pkg appears multiple times (#4945)

Don't fail when the patched package appears multiple times in the dependency graph

close #4938

.changeset/eighty-bags-hammer.md

@@ -0,0 +1,6 @@
+---
+"@pnpm/core": patch
+"@pnpm/resolve-dependencies": patch
+---
+
+Don't fail when the patched package appears multiple times in the dependency graph [#4938](https://github.com/pnpm/pnpm/issues/4938).

.changeset/unlucky-mangos-fix.md

@@ -0,0 +1,5 @@
+---
+"dependency-path": patch
+---
+
+Remove patchFileHash from createPeersFolderSuffix().

packages/core/test/install/patch.ts

@@ -320,3 +320,35 @@ test('patch package when the package is not in onlyBuiltDependencies list', asyn
   // 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 patched package has no dependencies and appears multipe times', 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,
+    overrides: {
+      'is-positive': '1.0.0',
+    },
+  }, {}, {}, { packageImportMethod: 'hardlink' })
+  await install({
+    dependencies: {
+      'is-positive': '1.0.0',
+      'is-not-positive': '1.0.0',
+    },
+  }, opts)
+
+  expect(fs.readFileSync('node_modules/is-positive/index.js', 'utf8')).toContain('// patched')
+
+  const lockfile = await project.readLockfile()
+  expect(Object.keys(lockfile.packages)).toStrictEqual([
+    '/is-not-positive/1.0.0',
+    '/is-positive/1.0.0_jnbpamcxayl5i4ehrkoext3any',
+  ])
+})

packages/dependency-path/src/index.ts

@@ -148,11 +148,8 @@ function depPathToFilenameUnescaped (depPath: string) {
   return depPath.replace(':', '+')
 }
 
-export function createPeersFolderSuffix (peers: Array<{name: string, version: string}>, patchFileHash?: string): string {
-  let folderName = peers.map(({ name, version }) => `${name.replace('/', '+')}@${version}`).sort().join('+')
-  if (patchFileHash) {
-    folderName += `_${patchFileHash}`
-  }
+export function createPeersFolderSuffix (peers: Array<{name: string, version: string}>): string {
+  const folderName = peers.map(({ name, version }) => `${name.replace('/', '+')}@${version}`).sort().join('+')
 
   // We don't want the folder name to get too long.
   // Otherwise, an ENAMETOOLONG error might happen.

packages/resolve-dependencies/src/resolveDependencies.ts

@@ -823,7 +823,13 @@ async function resolveDependency (
   if (!pkg.name) { // TODO: don't fail on optional dependencies
     throw new PnpmError('MISSING_PACKAGE_NAME', `Can't install ${wantedDependency.pref}: Missing package name`)
   }
-  const depPath = dp.relative(ctx.registries, pkg.name, pkgResponse.body.id)
+  let depPath = dp.relative(ctx.registries, pkg.name, pkgResponse.body.id)
+  const nameAndVersion = `${pkg.name}@${pkg.version}`
+  const patchFile = ctx.patchedDependencies?.[nameAndVersion]
+  if (patchFile) {
+    ctx.appliedPatches.add(nameAndVersion)
+    depPath += `_${patchFile.hash}`
+  }
 
   // We are building the dependency tree only until there are new packages
   // or the packages repeat in a unique order.
@@ -919,12 +925,6 @@ async function resolveDependency (
       status: 'resolved',
     })
 
-    const nameAndVersion = `${pkg.name}@${pkg.version}`
-    const patchFile = ctx.patchedDependencies?.[nameAndVersion]
-    if (patchFile) {
-      ctx.appliedPatches.add(nameAndVersion)
-    }
-
     ctx.resolvedPackagesByDepPath[depPath] = await getResolvedPackage({
       allowBuild: ctx.allowBuild,
       dependencyLockfile: currentPkg.dependencyLockfile,

packages/resolve-dependencies/src/resolvePeers.ts

@@ -39,7 +39,6 @@ export type PartialResolvedPackage = Pick<ResolvedPackage,
 | 'peerDependencies'
 | 'peerDependenciesMeta'
 | 'version'
-| 'patchFile'
 >
 
 export interface GenericDependenciesGraph<T extends PartialResolvedPackage> {
@@ -181,9 +180,6 @@ function resolvePeersOfNode<T extends PartialResolvedPackage> (
     isEmpty(resolvedPackage.peerDependencies)
   ) {
     ctx.pathsByNodeId[nodeId] = resolvedPackage.depPath
-    if (resolvedPackage.patchFile) {
-      ctx.pathsByNodeId[nodeId] += `_${resolvedPackage.patchFile.hash}`
-    }
     return { resolvedPeers: {}, missingPeers: [] }
   }
   if (typeof node.children === 'function') {
@@ -252,9 +248,6 @@ function resolvePeersOfNode<T extends PartialResolvedPackage> (
   let depPath: string
   if (isEmpty(allResolvedPeers)) {
     depPath = resolvedPackage.depPath
-    if (resolvedPackage.patchFile) {
-      depPath += `_${resolvedPackage.patchFile.hash}`
-    }
   } else {
     const peersFolderSuffix = createPeersFolderSuffix(
       Object.entries(allResolvedPeers)
@@ -268,8 +261,7 @@ function resolvePeersOfNode<T extends PartialResolvedPackage> (
           }
           const { name, version } = ctx.dependenciesTree[nodeId].resolvedPackage
           return { name, version }
-        }),
-      resolvedPackage.patchFile?.hash
+        })
     )
     // eslint-disable-next-line @typescript-eslint/restrict-template-expressions
     depPath = `${resolvedPackage.depPath}${peersFolderSuffix}`

packages/resolve-dependencies/test/resolvePeers.ts

@@ -516,82 +516,3 @@ describe('unmet peer dependency issue resolved from subdependency', () => {
     expect(peerDependencyIssuesByProjects.project.bad.dep[0].resolvedFrom).toStrictEqual([{ name: 'foo', version: '1.0.0' }])
   })
 })
-
-test('patch hash is added to the dependency path together with peer deps hash', () => {
-  const { dependenciesGraph } = resolvePeers({
-    projects: [
-      {
-        directNodeIdsByAlias: {
-          foo: '>project>foo/1.0.0>',
-          bar: '>project>bar/1.0.0>',
-          qar: '>project>qar/1.0.0>',
-        },
-        topParents: [],
-        rootDir: '',
-        id: 'project',
-      },
-    ],
-    dependenciesTree: {
-      '>project>foo/1.0.0>': {
-        children: {},
-        installable: true,
-        resolvedPackage: {
-          name: 'foo',
-          depPath: 'foo/1.0.0',
-          version: '1.0.0',
-          peerDependencies: {
-            bar: '1.0.0',
-          },
-          patchFile: {
-            hash: 'aaa',
-            path: '/patch.patch',
-          },
-        },
-        depth: 0,
-      },
-      '>project>bar/1.0.0>': {
-        children: {},
-        installable: true,
-        resolvedPackage: {
-          name: 'bar',
-          depPath: 'bar/1.0.0',
-          version: '1.0.0',
-          peerDependencies: {},
-        },
-        depth: 0,
-      },
-      '>project>qar/1.0.0>': {
-        children: {
-          qardep: '>project>qar/1.0.0>qardep/1.0.0>',
-        },
-        installable: true,
-        resolvedPackage: {
-          name: 'qar',
-          depPath: 'qar/1.0.0',
-          version: '1.0.0',
-          peerDependencies: {},
-          patchFile: {
-            hash: 'bbb',
-            path: 'bbb.patch',
-          },
-        },
-        depth: 0,
-      },
-      '>project>qar/1.0.0>qardep/1.0.0>': {
-        children: {},
-        installable: true,
-        resolvedPackage: {
-          name: 'qardep',
-          depPath: 'qardep/1.0.0',
-          version: '1.0.0',
-          peerDependencies: {},
-        },
-        depth: 0,
-      },
-    },
-    virtualStoreDir: '',
-    lockfileDir: '',
-  })
-  expect(dependenciesGraph['foo/1.0.0_bar@1.0.0_aaa']).toBeTruthy()
-  expect(dependenciesGraph['qar/1.0.0_bbb']).toBeTruthy()
-})