fix: side effects upload when node-linker is set to hoisted

This fixes a bug introduced by #5814
pnpm · Jan 9, 2023 · c5f4d33 · c5f4d33
1 parent 3ebce5d
commit c5f4d33
Show file tree

Hide file tree

Showing 8 changed files with 100 additions and 13 deletions.
diff --git a/.changeset/hip-schools-yawn.md 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
@@ -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
@@ -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
@@ -182,18 +182,23 @@ async function fetchDeps (
     const resolution = pkgSnapshotToResolution(depPath, pkgSnapshot, opts.registries)
     let fetchResponse!: ReturnType<FetchPackageToStoreFunction>
     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
@@ -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<void>
 }
 
+function getFilesIndexFilePath (
+  ctx: {
+    getFilePathInCafs: (integrity: string, fileType: FileType) => string
+    storeDir: string
+  },
+  opts: Pick<FetchPackageToStoreOptions, 'pkg' | 'ignoreScripts'>
+) {
+  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<BundledManifest>()
     const files = pDefer<PackageFilesResponse>()
     const finishing = pDefer<undefined>()
-    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<BundledManifest>,
     files: pDefer.DeferredPromise<PackageFilesResponse>,
-    finishing: pDefer.DeferredPromise<void>
+    finishing: pDefer.DeferredPromise<void>,
+    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
@@ -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
@@ -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
@@ -45,6 +45,7 @@ export type UploadPkgToStore = (builtPkgLocation: string, opts: UploadPkgToStore
 export interface StoreController {
   requestPackage: RequestPackageFunction
   fetchPackage: FetchPackageToStoreFunction
+  getFilesIndexFilePath: GetFilesIndexFilePath
   importPackage: ImportPackageFunction
   close: () => Promise<void>
   prune: () => Promise<void>
@@ -60,6 +61,11 @@ export type FetchPackageToStoreFunction = (
   finishing: () => Promise<void>
 }
 
+export type GetFilesIndexFilePath = (opts: Pick<FetchPackageToStoreOptions, 'pkg' | 'ignoreScripts'>) => {
+  filesIndexFile: string
+  target: string
+}
+
 export interface PkgNameVersion {
   name?: string
   version?: string