From 50e347d23c74c19fe8fabc2917148d5d4c6b488e Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Sun, 6 Feb 2022 17:13:46 +0200 Subject: [PATCH] fix: perf improvement + don't fail if an injected package has no package.json (#4304) --- .changeset/red-mice-smile.md | 5 ++ .changeset/six-walls-fetch.md | 5 ++ .../storeController/createImportPackage.ts | 36 ++++++++++---- .../test/createImportPackage.spec.ts | 48 +++++++++++++++++++ 4 files changed, 84 insertions(+), 10 deletions(-) create mode 100644 .changeset/red-mice-smile.md create mode 100644 .changeset/six-walls-fetch.md diff --git a/.changeset/red-mice-smile.md b/.changeset/red-mice-smile.md new file mode 100644 index 0000000000..f226fcc74f --- /dev/null +++ b/.changeset/red-mice-smile.md @@ -0,0 +1,5 @@ +--- +"pnpm": minor +--- + +When checking that a package is linked from the store, check the existence of the package and read its stats with a single filesystem operation [#4304](https://github.com/pnpm/pnpm/pull/4304). diff --git a/.changeset/six-walls-fetch.md b/.changeset/six-walls-fetch.md new file mode 100644 index 0000000000..ef988d9586 --- /dev/null +++ b/.changeset/six-walls-fetch.md @@ -0,0 +1,5 @@ +--- +"@pnpm/package-store": patch +--- + +When checking whether a package is linked from the store, don't fail if the package has no `package.json` file. diff --git a/packages/package-store/src/storeController/createImportPackage.ts b/packages/package-store/src/storeController/createImportPackage.ts index 26a13087e0..65feb7f320 100644 --- a/packages/package-store/src/storeController/createImportPackage.ts +++ b/packages/package-store/src/storeController/createImportPackage.ts @@ -1,4 +1,4 @@ -import { constants, promises as fs } from 'fs' +import { constants, promises as fs, Stats } from 'fs' import path from 'path' import { globalInfo, globalWarn } from '@pnpm/logger' import { packageImportMethodLogger } from '@pnpm/core-loggers' @@ -107,9 +107,11 @@ async function hardlinkPkg ( to: string, opts: ImportOptions ) { - const pkgJsonPath = path.join(to, 'package.json') - - if (!opts.fromStore || opts.force || !await exists(pkgJsonPath) || !await pkgLinkedToStore(pkgJsonPath, opts.filesMap['package.json'], to)) { + if ( + !opts.fromStore || + opts.force || + !await pkgLinkedToStore(opts.filesMap, to) + ) { await importIndexedDir(importFile, to, opts.filesMap) return 'hardlink' } @@ -131,18 +133,32 @@ async function linkOrCopy (existingPath: string, newPath: string) { } async function pkgLinkedToStore ( - pkgJsonPath: string, - pkgJsonPathInStore: string, + filesMap: Record, to: string ) { - if (await isSameFile(pkgJsonPath, pkgJsonPathInStore)) return true + if (filesMap['package.json']) { + if (await isSameFile(path.join(to, 'package.json'), filesMap['package.json'])) { + return true + } + } else { + // An injected package might not have a package.json. + // This will probably only even happen in a Bit workspace. + const [anyFile] = Object.keys(filesMap) + if (await isSameFile(path.join(to, anyFile), filesMap[anyFile])) return true + } globalInfo(`Relinking ${to} from the store`) return false } -async function isSameFile (file1: string, file2: string) { - const stats = await Promise.all([fs.stat(file1), fs.stat(file2)]) - return stats[0].ino === stats[1].ino +async function isSameFile (linkedFile: string, fileFromStore: string) { + let stats0!: Stats + try { + stats0 = await fs.stat(linkedFile) + } catch (err: any) { // eslint-disable-line + if (err.code === 'ENOENT') return false + } + const stats1 = await fs.stat(fileFromStore) + return stats0.ino === stats1.ino } export async function copyPkg ( diff --git a/packages/package-store/test/createImportPackage.spec.ts b/packages/package-store/test/createImportPackage.spec.ts index 1d15df4e44..272a00efd4 100644 --- a/packages/package-store/test/createImportPackage.spec.ts +++ b/packages/package-store/test/createImportPackage.spec.ts @@ -145,3 +145,51 @@ test('packageImportMethod=hardlink: fall back to copying if hardlinking fails', expect(fsMock.promises.copyFile).toBeCalledWith(path.join('hash1'), path.join('project', '_tmp', 'package.json')) expect(fsMock.promises.copyFile).toBeCalledWith(path.join('hash2'), path.join('project', '_tmp', 'index.js')) }) + +test('packageImportMethod=hardlink does not relink package from store if package.json is linked from the store', async () => { + const importPackage = createImportPackage('hardlink') + fsMock.promises.copyFile = jest.fn() + fsMock.promises.rename = jest.fn() + fsMock.promises.stat = jest.fn(() => ({ ino: 1 })) + expect(await importPackage('project/package', { + filesMap: { + 'index.js': 'hash2', + 'package.json': 'hash1', + }, + force: false, + fromStore: true, + })).toBe(undefined) +}) + +test('packageImportMethod=hardlink relinks package from store if package.json is not linked from the store', async () => { + const importPackage = createImportPackage('hardlink') + fsMock.promises.copyFile = jest.fn() + fsMock.promises.rename = jest.fn() + let ino = 0 + fsMock.promises.stat = jest.fn(() => ({ ino: ++ino })) + expect(await importPackage('project/package', { + filesMap: { + 'index.js': 'hash2', + 'package.json': 'hash1', + }, + force: false, + fromStore: true, + })).toBe('hardlink') +}) + +test('packageImportMethod=hardlink does not relink package from store if package.json is not present in the store', async () => { + const importPackage = createImportPackage('hardlink') + fsMock.promises.copyFile = jest.fn() + fsMock.promises.rename = jest.fn() + fsMock.promises.stat = jest.fn((file) => { + expect(typeof file).toBe('string') + return { ino: 1 } + }) + expect(await importPackage('project/package', { + filesMap: { + 'index.js': 'hash2', + }, + force: false, + fromStore: true, + })).toBe(undefined) +})