From c5f4d339be58f4ea5d52fe99c905170908afbe48 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Mon, 9 Jan 2023 18:32:53 +0200 Subject: [PATCH] fix: side effects upload when node-linker is set to hoisted This fixes a bug introduced by #5814 --- .changeset/hip-schools-yawn.md | 8 ++++ .changeset/kind-squids-sleep.md | 8 ++++ .../core/test/install/lifecycleScripts.ts | 37 +++++++++++++++++++ .../headless/src/lockfileToHoistedDepGraph.ts | 15 +++++--- .../package-requester/src/packageRequester.ts | 37 +++++++++++++++---- .../src/storeController/index.ts | 1 + store/server/src/connectStoreController.ts | 1 + store/store-controller-types/src/index.ts | 6 +++ 8 files changed, 100 insertions(+), 13 deletions(-) create mode 100644 .changeset/hip-schools-yawn.md create mode 100644 .changeset/kind-squids-sleep.md diff --git a/.changeset/hip-schools-yawn.md b/.changeset/hip-schools-yawn.md new file mode 100644 index 00000000000..21d4e54b13f --- /dev/null +++ b/.changeset/hip-schools-yawn.md @@ -0,0 +1,8 @@ +--- +"@pnpm/headless": patch +"pnpm": patch +--- + +The upload of built artifacts (side effects) should not fail when `node-linker` is set to `hoisted` and installation runs on a project that already had a `node_modules` directory. + +This fixes a bug introduced by [#5814](https://github.com/pnpm/pnpm/pull/5814). diff --git a/.changeset/kind-squids-sleep.md b/.changeset/kind-squids-sleep.md new file mode 100644 index 00000000000..cf50411ef55 --- /dev/null +++ b/.changeset/kind-squids-sleep.md @@ -0,0 +1,8 @@ +--- +"@pnpm/package-requester": minor +"@pnpm/store-controller-types": minor +"@pnpm/package-store": minor +"@pnpm/server": minor +--- + +New function added to the store: `getFilesIndexFilePath`. diff --git a/pkg-manager/core/test/install/lifecycleScripts.ts b/pkg-manager/core/test/install/lifecycleScripts.ts index 0852fcba22e..88bb81ba691 100644 --- a/pkg-manager/core/test/install/lifecycleScripts.ts +++ b/pkg-manager/core/test/install/lifecycleScripts.ts @@ -717,3 +717,40 @@ test('run pre/postinstall scripts in a workspace that uses node-linker=hoisted', await projects['project-4'].has('@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-preinstall.js') await projects['project-4'].has('@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-postinstall.js') }) + +test('run pre/postinstall scripts in a project that uses node-linker=hoisted. Should not fail on repeat install', async () => { + const project = prepareEmpty() + const manifest = await addDependenciesToPackage({}, + ['@pnpm.e2e/pre-and-postinstall-scripts-example@1.0.0'], + await testDefaults({ fastUnpack: false, targetDependenciesField: 'devDependencies', nodeLinker: 'hoisted', sideEffectsCacheRead: true, sideEffectsCacheWrite: true }) + ) + + { + expect(await exists('node_modules/@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-prepare.js')).toBeFalsy() + expect(await exists('node_modules/@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-preinstall.js')).toBeTruthy() + + const generatedByPreinstall = project.requireModule('@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-preinstall') + expect(typeof generatedByPreinstall).toBe('function') + + const generatedByPostinstall = project.requireModule('@pnpm.e2e/pre-and-postinstall-scripts-example/generated-by-postinstall') + expect(typeof generatedByPostinstall).toBe('function') + } + + const reporter = jest.fn() + await addDependenciesToPackage(manifest, + ['example@npm:@pnpm.e2e/pre-and-postinstall-scripts-example@2.0.0'], + await testDefaults({ + fastUnpack: false, + targetDependenciesField: 'devDependencies', + nodeLinker: 'hoisted', + reporter, + sideEffectsCacheRead: true, + sideEffectsCacheWrite: true, + }) + ) + + expect(reporter).not.toBeCalledWith(expect.objectContaining({ + level: 'warn', + message: `An error occurred while uploading ${path.resolve('node_modules/@pnpm.e2e/pre-and-postinstall-scripts-example')}`, + })) +}) diff --git a/pkg-manager/headless/src/lockfileToHoistedDepGraph.ts b/pkg-manager/headless/src/lockfileToHoistedDepGraph.ts index 64196e68f4f..026c66e459d 100644 --- a/pkg-manager/headless/src/lockfileToHoistedDepGraph.ts +++ b/pkg-manager/headless/src/lockfileToHoistedDepGraph.ts @@ -182,18 +182,23 @@ async function fetchDeps ( const resolution = pkgSnapshotToResolution(depPath, pkgSnapshot, opts.registries) let fetchResponse!: ReturnType const skipFetch = opts.currentHoistedLocations?.[depPath]?.includes(depLocation) + const pkgResolution = { + id: packageId, + resolution, + } if (skipFetch) { - fetchResponse = {} as any // eslint-disable-line @typescript-eslint/no-explicit-any + const { filesIndexFile } = opts.storeController.getFilesIndexFilePath({ + ignoreScripts: opts.ignoreScripts, + pkg: pkgResolution, + }) + fetchResponse = { filesIndexFile } as any // eslint-disable-line @typescript-eslint/no-explicit-any } else { try { fetchResponse = opts.storeController.fetchPackage({ force: false, lockfileDir: opts.lockfileDir, ignoreScripts: opts.ignoreScripts, - pkg: { - id: packageId, - resolution, - }, + pkg: pkgResolution, expectedPkg: { name: pkgName, version: pkgVersion, diff --git a/pkg-manager/package-requester/src/packageRequester.ts b/pkg-manager/package-requester/src/packageRequester.ts index 897afd0d037..d1be0044ed6 100644 --- a/pkg-manager/package-requester/src/packageRequester.ts +++ b/pkg-manager/package-requester/src/packageRequester.ts @@ -33,6 +33,7 @@ import { BundledManifestFunction, FetchPackageToStoreFunction, FetchPackageToStoreOptions, + GetFilesIndexFilePath, PackageResponse, PkgNameVersion, RequestPackageFunction, @@ -96,6 +97,7 @@ export function createPackageRequester ( } ): RequestPackageFunction & { fetchPackageToStore: FetchPackageToStoreFunction + getFilesIndexFilePath: GetFilesIndexFilePath requestPackage: RequestPackageFunction } { opts = opts || {} @@ -135,7 +137,14 @@ export function createPackageRequester ( storeDir: opts.storeDir, }) - return Object.assign(requestPackage, { fetchPackageToStore, requestPackage }) + return Object.assign(requestPackage, { + fetchPackageToStore, + getFilesIndexFilePath: getFilesIndexFilePath.bind(null, { + getFilePathInCafs, + storeDir: opts.storeDir, + }), + requestPackage, + }) } async function resolveAndFetch ( @@ -296,6 +305,21 @@ interface FetchLock { finishing: Promise } +function getFilesIndexFilePath ( + ctx: { + getFilePathInCafs: (integrity: string, fileType: FileType) => string + storeDir: string + }, + opts: Pick +) { + const targetRelative = depPathToFilename(opts.pkg.id) + const target = path.join(ctx.storeDir, targetRelative) + const filesIndexFile = opts.pkg.resolution['integrity'] + ? ctx.getFilePathInCafs(opts.pkg.resolution['integrity'], 'index') + : path.join(target, opts.ignoreScripts ? 'integrity-not-built.json' : 'integrity.json') + return { filesIndexFile, target } +} + function fetchToStore ( ctx: { checkFilesIntegrity: ( @@ -323,18 +347,14 @@ function fetchToStore ( if (!opts.pkg.name) { opts.fetchRawManifest = true } - const targetRelative = depPathToFilename(opts.pkg.id) - const target = path.join(ctx.storeDir, targetRelative) if (!ctx.fetchingLocker.has(opts.pkg.id)) { const bundledManifest = pDefer() const files = pDefer() const finishing = pDefer() - const filesIndexFile = opts.pkg.resolution['integrity'] - ? ctx.getFilePathInCafs(opts.pkg.resolution['integrity'], 'index') - : path.join(target, opts.ignoreScripts ? 'integrity-not-built.json' : 'integrity.json') + const { filesIndexFile, target } = getFilesIndexFilePath(ctx, opts) - doFetchToStore(filesIndexFile, bundledManifest, files, finishing) // eslint-disable-line + doFetchToStore(filesIndexFile, bundledManifest, files, finishing, target) // eslint-disable-line if (opts.fetchRawManifest) { ctx.fetchingLocker.set(opts.pkg.id, { @@ -426,7 +446,8 @@ function fetchToStore ( filesIndexFile: string, bundledManifest: pDefer.DeferredPromise, files: pDefer.DeferredPromise, - finishing: pDefer.DeferredPromise + finishing: pDefer.DeferredPromise, + target: string ) { try { const isLocalTarballDep = opts.pkg.id.startsWith('file:') diff --git a/store/package-store/src/storeController/index.ts b/store/package-store/src/storeController/index.ts index 99bfcf5259d..26d6166059e 100644 --- a/store/package-store/src/storeController/index.ts +++ b/store/package-store/src/storeController/index.ts @@ -50,6 +50,7 @@ export async function createPackageStore ( return { close: async () => {}, // eslint-disable-line:no-empty fetchPackage: packageRequester.fetchPackageToStore, + getFilesIndexFilePath: packageRequester.getFilesIndexFilePath, importPackage: cafs.importPackage, prune: prune.bind(null, { storeDir, cacheDir: initOpts.cacheDir }), requestPackage: packageRequester.requestPackage, diff --git a/store/server/src/connectStoreController.ts b/store/server/src/connectStoreController.ts index df5f21eec4f..d7b42eb024f 100644 --- a/store/server/src/connectStoreController.ts +++ b/store/server/src/connectStoreController.ts @@ -30,6 +30,7 @@ export async function connectStoreController ( resolve({ close: async () => { }, fetchPackage: fetchPackage.bind(null, remotePrefix, limitedFetch), + getFilesIndexFilePath: () => ({ filesIndexFile: '', target: '' }), // NOT IMPLEMENTED importPackage: async (to: string, opts: { filesResponse: PackageFilesResponse force: boolean diff --git a/store/store-controller-types/src/index.ts b/store/store-controller-types/src/index.ts index 03358aa6a9c..07bcf8a082a 100644 --- a/store/store-controller-types/src/index.ts +++ b/store/store-controller-types/src/index.ts @@ -45,6 +45,7 @@ export type UploadPkgToStore = (builtPkgLocation: string, opts: UploadPkgToStore export interface StoreController { requestPackage: RequestPackageFunction fetchPackage: FetchPackageToStoreFunction + getFilesIndexFilePath: GetFilesIndexFilePath importPackage: ImportPackageFunction close: () => Promise prune: () => Promise @@ -60,6 +61,11 @@ export type FetchPackageToStoreFunction = ( finishing: () => Promise } +export type GetFilesIndexFilePath = (opts: Pick) => { + filesIndexFile: string + target: string +} + export interface PkgNameVersion { name?: string version?: string