fix corepack detection for package manager version determination (#11596

) The previous logic was checking for the env var `ENABLE_EXPERIMENTAL_COREPACK` to determine if corepack was being used by a project. However, this value only means that the build system should consider corepack, not that it's actively being used. We need to check that flag AND the existence of a `packageManager` property in the project's `package.json`.
vercel · May 15, 2024 · ccd7eb1 · ccd7eb1
1 parent 41c44d6
commit ccd7eb1
Show file tree

Hide file tree

Showing 9 changed files with 95 additions and 23 deletions.
diff --git a/.changeset/soft-bananas-care.md b/.changeset/soft-bananas-care.md
@@ -0,0 +1,5 @@
+---
+"@vercel/build-utils": minor
+---
+
+fix corepack detection for package manager version determination
diff --git a/packages/build-utils/src/fs/run-user-scripts.ts b/packages/build-utils/src/fs/run-user-scripts.ts
@@ -426,9 +426,8 @@ export async function runNpmInstall(
 
   try {
     await runNpmInstallSema.acquire();
-    const { cliType, packageJsonPath, lockfileVersion } = await scanParentDirs(
-      destPath
-    );
+    const { cliType, packageJsonPath, packageJson, lockfileVersion } =
+      await scanParentDirs(destPath, true);
 
     if (!packageJsonPath) {
       debug(
@@ -463,6 +462,7 @@ export async function runNpmInstall(
     opts.env = getEnvForPackageManager({
       cliType,
       lockfileVersion,
+      packageJsonPackageManager: packageJson?.packageManager,
       nodeVersion,
       env,
     });
@@ -547,29 +547,39 @@ export async function runNpmInstall(
 export function getEnvForPackageManager({
   cliType,
   lockfileVersion,
+  packageJsonPackageManager,
   nodeVersion,
   env,
 }: {
   cliType: CliType;
   lockfileVersion: number | undefined;
+  packageJsonPackageManager?: string | undefined;
   nodeVersion: NodeVersion | undefined;
   env: { [x: string]: string | undefined };
 }) {
+  const corepackFlagged = env.ENABLE_EXPERIMENTAL_COREPACK === '1';
+  const corepackEnabled = corepackFlagged && Boolean(packageJsonPackageManager);
+
   const {
     detectedLockfile,
     detectedPackageManager,
     path: newPath,
   } = getPathOverrideForPackageManager({
     cliType,
     lockfileVersion,
+    corepackEnabled,
     nodeVersion,
-    env,
   });
 
-  const corepackEnabled = env.ENABLE_EXPERIMENTAL_COREPACK === '1';
-  debug(
-    `Detected ${detectedPackageManager} given lockfileVersion "${lockfileVersion}", package manager cli "${cliType}", and corepack enabled? ${corepackEnabled}: ${newPath}`
-  );
+  if (corepackEnabled) {
+    debug(
+      `Detected corepack use for "${packageJsonPackageManager}". Not overriding package manager version.`
+    );
+  } else {
+    debug(
+      `Detected ${detectedPackageManager}. Added "${newPath}" to path. Based on assumed package manager "${cliType}", lockfile "${detectedLockfile}", and lockfileVersion "${lockfileVersion}"`
+    );
+  }
 
   const newEnv: { [x: string]: string | undefined } = {
     ...env,
@@ -655,13 +665,13 @@ function shouldUseNpm7(
 export function getPathOverrideForPackageManager({
   cliType,
   lockfileVersion,
+  corepackEnabled,
   nodeVersion,
-  env,
 }: {
   cliType: CliType;
   lockfileVersion: number | undefined;
+  corepackEnabled: boolean;
   nodeVersion: NodeVersion | undefined;
-  env: { [x: string]: string | undefined };
 }): {
   /**
    * Which lockfile was detected.
@@ -683,8 +693,6 @@ export function getPathOverrideForPackageManager({
     path: undefined,
   };
 
-  const corepackEnabled = env.ENABLE_EXPERIMENTAL_COREPACK === '1';
-
   switch (cliType) {
     case 'npm':
       switch (true) {
@@ -748,6 +756,7 @@ export function getPathOverrideForPackageManager({
 /**
  * Helper to get the binary paths that link to the used package manager.
  * Note: Make sure it doesn't contain any `console.log` calls.
+ * @deprecated use `getEnvForPackageManager` instead
  */
 export function getPathForPackageManager({
   cliType,
@@ -779,11 +788,16 @@ export function getPathForPackageManager({
    */
   yarnNodeLinker: string | undefined;
 } {
+  // This is not the correct check for whether or not corepack is being used. For that, you'd have to check
+  // the package.json's `packageManager` property. However, this deprecated function is keeping it's old,
+  // broken behavior.
+  const corepackEnabled = env.ENABLE_EXPERIMENTAL_COREPACK === '1';
+
   const overrides = getPathOverrideForPackageManager({
     cliType,
     lockfileVersion,
+    corepackEnabled,
     nodeVersion,
-    env,
   });
 
   const alreadyInPath = (newPath: string) => {
@@ -818,10 +832,14 @@ export async function runCustomInstallCommand({
   spawnOpts?: SpawnOptions;
 }) {
   console.log(`Running "install" command: \`${installCommand}\`...`);
-  const { cliType, lockfileVersion } = await scanParentDirs(destPath);
+  const { cliType, lockfileVersion, packageJson } = await scanParentDirs(
+    destPath,
+    true
+  );
   const env = getEnvForPackageManager({
     cliType,
     lockfileVersion,
+    packageJsonPackageManager: packageJson?.packageManager,
     nodeVersion,
     env: spawnOpts?.env || {},
   });
@@ -859,6 +877,7 @@ export async function runPackageJsonScript(
     env: getEnvForPackageManager({
       cliType,
       lockfileVersion,
+      packageJsonPackageManager: packageJson?.packageManager,
       nodeVersion: undefined,
       env: cloneEnv(process.env, spawnOpts?.env),
     }),

diff --git a/packages/build-utils/test/unit.get-env-for-package-manager.test.ts b/packages/build-utils/test/unit.get-env-for-package-manager.test.ts
@@ -35,6 +35,7 @@ describe('Test `getEnvForPackageManager()`', () => {
       args: {
         cliType: 'npm',
         nodeVersion: { major: 14, range: '14.x', runtime: 'nodejs14.x' },
+        packageJsonPackageManager: undefined,
         lockfileVersion: 1,
         env: {
           FOO: 'bar',
@@ -51,6 +52,7 @@ describe('Test `getEnvForPackageManager()`', () => {
       args: {
         cliType: 'npm',
         nodeVersion: { major: 14, range: '14.x', runtime: 'nodejs14.x' },
+        packageJsonPackageManager: undefined,
         lockfileVersion: 2,
         env: {
           FOO: 'bar',
@@ -69,6 +71,7 @@ describe('Test `getEnvForPackageManager()`', () => {
       args: {
         cliType: 'npm',
         nodeVersion: { major: 14, range: '14.x', runtime: 'nodejs14.x' },
+        packageJsonPackageManager: 'pnpm@latest',
         lockfileVersion: 2,
         env: {
           FOO: 'bar',
@@ -87,6 +90,7 @@ describe('Test `getEnvForPackageManager()`', () => {
       args: {
         cliType: 'npm',
         nodeVersion: { major: 14, range: '14.x', runtime: 'nodejs14.x' },
+        packageJsonPackageManager: undefined,
         lockfileVersion: 2,
         env: {
           FOO: 'bar',
@@ -105,6 +109,7 @@ describe('Test `getEnvForPackageManager()`', () => {
       args: {
         cliType: 'npm',
         nodeVersion: { major: 16, range: '16.x', runtime: 'nodejs16.x' },
+        packageJsonPackageManager: undefined,
         lockfileVersion: 2,
         env: {
           FOO: 'bar',
@@ -123,6 +128,7 @@ describe('Test `getEnvForPackageManager()`', () => {
       args: {
         cliType: 'yarn',
         nodeVersion: { major: 16, range: '16.x', runtime: 'nodejs16.x' },
+        packageJsonPackageManager: undefined,
         lockfileVersion: 2,
         env: {
           FOO: 'bar',
@@ -140,6 +146,7 @@ describe('Test `getEnvForPackageManager()`', () => {
       args: {
         cliType: 'yarn',
         nodeVersion: { major: 16, range: '16.x', runtime: 'nodejs16.x' },
+        packageJsonPackageManager: undefined,
         lockfileVersion: 2,
         env: {
           FOO: 'bar',
@@ -158,6 +165,7 @@ describe('Test `getEnvForPackageManager()`', () => {
       args: {
         cliType: 'pnpm',
         nodeVersion: { major: 16, range: '16.x', runtime: 'nodejs16.x' },
+        packageJsonPackageManager: undefined,
         lockfileVersion: 5.4,
         env: {
           FOO: 'bar',
@@ -177,6 +185,7 @@ describe('Test `getEnvForPackageManager()`', () => {
       args: {
         cliType: 'pnpm',
         nodeVersion: { major: 18, range: '18.x', runtime: 'nodejs18.x' },
+        packageJsonPackageManager: undefined,
         lockfileVersion: 6.0,
         env: {
           FOO: 'bar',
@@ -196,6 +205,7 @@ describe('Test `getEnvForPackageManager()`', () => {
       args: {
         cliType: 'pnpm',
         nodeVersion: { major: 18, range: '18.x', runtime: 'nodejs18.x' },
+        packageJsonPackageManager: undefined,
         lockfileVersion: 9.0,
         env: {
           FOO: 'bar',
@@ -215,6 +225,7 @@ describe('Test `getEnvForPackageManager()`', () => {
       args: {
         cliType: 'bun',
         nodeVersion: { major: 18, range: '18.x', runtime: 'nodejs18.x' },
+        packageJsonPackageManager: undefined,
         lockfileVersion: 0,
         env: {
           FOO: 'bar',
@@ -234,6 +245,7 @@ describe('Test `getEnvForPackageManager()`', () => {
       args: {
         cliType: 'pnpm',
         nodeVersion: { major: 16, range: '16.x', runtime: 'nodejs16.x' },
+        packageJsonPackageManager: 'npm@latest',
         lockfileVersion: 5.4,
         env: {
           FOO: 'bar',
@@ -252,6 +264,7 @@ describe('Test `getEnvForPackageManager()`', () => {
       args: {
         cliType: 'pnpm',
         nodeVersion: { major: 16, range: '16.x', runtime: 'nodejs16.x' },
+        packageJsonPackageManager: undefined,
         lockfileVersion: 5.4,
         env: {
           FOO: 'bar',
@@ -270,6 +283,7 @@ describe('Test `getEnvForPackageManager()`', () => {
       getEnvForPackageManager({
         cliType: args.cliType,
         lockfileVersion: args.lockfileVersion,
+        packageJsonPackageManager: args.packageJsonPackageManager,
         nodeVersion: args.nodeVersion,
         env: args.env,
       })
@@ -302,6 +316,7 @@ describe('Test `getPathOverrideForPackageManager()`', () => {
       args: {
         cliType: 'npm',
         nodeVersion: { major: 14, range: '14.x', runtime: 'nodejs14.x' },
+        packageJsonPackageManager: undefined,
         lockfileVersion: 1,
         env: {
           FOO: 'bar',
@@ -318,6 +333,7 @@ describe('Test `getPathOverrideForPackageManager()`', () => {
       args: {
         cliType: 'npm',
         nodeVersion: { major: 14, range: '14.x', runtime: 'nodejs14.x' },
+        packageJsonPackageManager: undefined,
         lockfileVersion: 2,
         env: {
           FOO: 'bar',
@@ -335,6 +351,7 @@ describe('Test `getPathOverrideForPackageManager()`', () => {
       args: {
         cliType: 'npm',
         nodeVersion: { major: 14, range: '14.x', runtime: 'nodejs14.x' },
+        packageJsonPackageManager: 'pnpm@latest',
         lockfileVersion: 2,
         env: {
           FOO: 'bar',
@@ -353,6 +370,7 @@ describe('Test `getPathOverrideForPackageManager()`', () => {
       args: {
         cliType: 'npm',
         nodeVersion: { major: 16, range: '16.x', runtime: 'nodejs16.x' },
+        packageJsonPackageManager: undefined,
         lockfileVersion: 2,
         env: {
           FOO: 'bar',
@@ -370,6 +388,7 @@ describe('Test `getPathOverrideForPackageManager()`', () => {
       args: {
         cliType: 'pnpm',
         nodeVersion: { major: 16, range: '16.x', runtime: 'nodejs16.x' },
+        packageJsonPackageManager: undefined,
         lockfileVersion: 5.3, // detects as pnpm@6, which is the default
         env: {
           FOO: 'bar',
@@ -387,6 +406,7 @@ describe('Test `getPathOverrideForPackageManager()`', () => {
       args: {
         cliType: 'pnpm',
         nodeVersion: { major: 16, range: '16.x', runtime: 'nodejs16.x' },
+        packageJsonPackageManager: undefined,
         lockfileVersion: 5.4,
         env: {
           FOO: 'bar',
@@ -404,6 +424,7 @@ describe('Test `getPathOverrideForPackageManager()`', () => {
       args: {
         cliType: 'pnpm',
         nodeVersion: { major: 18, range: '18.x', runtime: 'nodejs18.x' },
+        packageJsonPackageManager: undefined,
         lockfileVersion: 6.1,
         env: {
           FOO: 'bar',
@@ -421,6 +442,7 @@ describe('Test `getPathOverrideForPackageManager()`', () => {
       args: {
         cliType: 'pnpm',
         nodeVersion: { major: 18, range: '18.x', runtime: 'nodejs18.x' },
+        packageJsonPackageManager: undefined,
         lockfileVersion: 7.0,
         env: {
           FOO: 'bar',
@@ -438,6 +460,7 @@ describe('Test `getPathOverrideForPackageManager()`', () => {
       args: {
         cliType: 'bun',
         nodeVersion: { major: 18, range: '18.x', runtime: 'nodejs18.x' },
+        packageJsonPackageManager: undefined,
         lockfileVersion: 0,
         env: {
           FOO: 'bar',
@@ -455,6 +478,7 @@ describe('Test `getPathOverrideForPackageManager()`', () => {
       args: {
         cliType: 'pnpm',
         nodeVersion: { major: 16, range: '16.x', runtime: 'nodejs16.x' },
+        packageJsonPackageManager: 'npm@latest',
         lockfileVersion: 5.4,
         env: {
           FOO: 'bar',
@@ -472,16 +496,18 @@ describe('Test `getPathOverrideForPackageManager()`', () => {
       getPathOverrideForPackageManager({
         cliType: args.cliType,
         lockfileVersion: args.lockfileVersion,
+        // naive assumption that enabling corepack as a feature means it's used, but this is fine for tests
+        corepackEnabled: Boolean(args.env.ENABLE_EXPERIMENTAL_COREPACK),
         nodeVersion: args.nodeVersion,
-        env: args.env,
       })
     ).toStrictEqual(want);
   });
 });
+
 describe('Test `getPathForPackageManager()`', () => {
   test.each<{
     name: string;
-    args: Parameters<typeof getEnvForPackageManager>[0];
+    args: Parameters<typeof getPathForPackageManager>[0];
     want: unknown;
   }>([
     {

diff --git a/packages/hydrogen/src/build.ts b/packages/hydrogen/src/build.ts
@@ -51,11 +51,15 @@ export const build: BuildV2 = async ({
   );
 
   const spawnOpts = getSpawnOptions(meta, nodeVersion);
-  const { cliType, lockfileVersion } = await scanParentDirs(entrypointDir);
+  const { cliType, lockfileVersion, packageJson } = await scanParentDirs(
+    entrypointDir,
+    true
+  );
 
   spawnOpts.env = getEnvForPackageManager({
     cliType,
     lockfileVersion,
+    packageJsonPackageManager: packageJson?.packageManager,
     nodeVersion,
     env: spawnOpts.env || {},
   });

diff --git a/packages/next/src/index.ts b/packages/next/src/index.ts
@@ -257,10 +257,15 @@ export const build: BuildV2 = async ({
   const nextVersionRange = await getNextVersionRange(entryPath);
   const nodeVersion = await getNodeVersion(entryPath, undefined, config, meta);
   const spawnOpts = getSpawnOptions(meta, nodeVersion);
-  const { cliType, lockfileVersion } = await scanParentDirs(entryPath);
+  const { cliType, lockfileVersion, packageJson } = await scanParentDirs(
+    entryPath,
+    true
+  );
+
   spawnOpts.env = getEnvForPackageManager({
     cliType,
     lockfileVersion,
+    packageJsonPackageManager: packageJson?.packageManager,
     nodeVersion,
     env: spawnOpts.env || {},
   });