From fc581d371d43f382a14abd9aadcf2e999d800885 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Tue, 28 Jun 2022 02:23:48 +0300 Subject: [PATCH] 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 | 6 ++ .changeset/unlucky-mangos-fix.md | 5 ++ packages/core/test/install/patch.ts | 32 ++++++++ packages/dependency-path/src/index.ts | 7 +- .../src/resolveDependencies.ts | 14 ++-- .../resolve-dependencies/src/resolvePeers.ts | 10 +-- .../resolve-dependencies/test/resolvePeers.ts | 79 ------------------- 7 files changed, 53 insertions(+), 100 deletions(-) create mode 100644 .changeset/eighty-bags-hammer.md create mode 100644 .changeset/unlucky-mangos-fix.md diff --git a/.changeset/eighty-bags-hammer.md b/.changeset/eighty-bags-hammer.md new file mode 100644 index 0000000000..4fde2e34bb --- /dev/null +++ b/.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). diff --git a/.changeset/unlucky-mangos-fix.md b/.changeset/unlucky-mangos-fix.md new file mode 100644 index 0000000000..adf5924947 --- /dev/null +++ b/.changeset/unlucky-mangos-fix.md @@ -0,0 +1,5 @@ +--- +"dependency-path": patch +--- + +Remove patchFileHash from createPeersFolderSuffix(). diff --git a/packages/core/test/install/patch.ts b/packages/core/test/install/patch.ts index a1aec671d3..dca86808e8 100644 --- a/packages/core/test/install/patch.ts +++ b/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', + ]) +}) diff --git a/packages/dependency-path/src/index.ts b/packages/dependency-path/src/index.ts index 9a3491cca5..ee2759d937 100644 --- a/packages/dependency-path/src/index.ts +++ b/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. diff --git a/packages/resolve-dependencies/src/resolveDependencies.ts b/packages/resolve-dependencies/src/resolveDependencies.ts index 3124c691e6..644a501268 100644 --- a/packages/resolve-dependencies/src/resolveDependencies.ts +++ b/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, diff --git a/packages/resolve-dependencies/src/resolvePeers.ts b/packages/resolve-dependencies/src/resolvePeers.ts index 1572d537d8..98495efa58 100644 --- a/packages/resolve-dependencies/src/resolvePeers.ts +++ b/packages/resolve-dependencies/src/resolvePeers.ts @@ -39,7 +39,6 @@ export type PartialResolvedPackage = Pick export interface GenericDependenciesGraph { @@ -181,9 +180,6 @@ function resolvePeersOfNode ( 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 ( 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 ( } 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}` diff --git a/packages/resolve-dependencies/test/resolvePeers.ts b/packages/resolve-dependencies/test/resolvePeers.ts index cc28f6e08c..86be5b3d31 100644 --- a/packages/resolve-dependencies/test/resolvePeers.ts +++ b/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() -})