Fix package manager detection in hydrogen upgrade for monorepos

.changeset/upgrade-monorepo-package-manager.md

@@ -0,0 +1,5 @@
+---
+'@shopify/cli-hydrogen': patch
+---
+
+Fix `hydrogen upgrade` package manager detection in monorepos. The command now searches parent directories for a lockfile (such as a pnpm workspace root), so upgrades run with your workspace's package manager instead of falling back to npm.

packages/cli/src/commands/hydrogen/upgrade.test.ts

@@ -1,7 +1,7 @@
 import {createRequire} from 'node:module';
 import {tmpdir} from 'node:os';
-import {mkdtemp, readFile, rm} from 'node:fs/promises';
-import {describe, it, expect, vi, beforeEach} from 'vitest';
+import {mkdir, mkdtemp, readFile, rm} from 'node:fs/promises';
+import {describe, it, expect, vi, beforeEach, afterEach} from 'vitest';
 import {writeFile, fileExists} from '@shopify/cli-kit/node/fs';
 import {joinPath} from '@shopify/cli-kit/node/path';
 import {
@@ -13,9 +13,11 @@ import {
 import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output';
 import {type PackageJson} from '@shopify/cli-kit/node/node-package-manager';
 import {exec} from '@shopify/cli-kit/node/system';
+import * as system from '@shopify/cli-kit/node/system';
 import {
   buildUpgradeCommandArgs,
   displayConfirmation,
+  displayUpgradeSummary,
   generateUpgradeInstructionsFile,
   getAbsoluteVersion,
   getAvailableUpgrades,
@@ -31,20 +33,34 @@ import {
   getChangelog,
   displayDevUpgradeNotice,
 } from './upgrade.js';
+import * as packageManagers from '../../lib/package-managers.js';
 import {getSkeletonSourceDir} from '../../lib/build.js';
 
 // Test version numbers used to avoid conflicts with duplicate changelog versions
 const TEST_VERSION_DEPENDENCY_UPGRADE = '9999.99.99';
 const TEST_VERSION_DEV_DEPENDENCY_UPGRADE = '9999.99.98';
 
 vi.mock('@shopify/cli-kit/node/session');
-vi.mock('@shopify/cli-kit/node/node-package-manager', async () => {
+vi.mock('../../lib/package-managers.js', async () => {
   const original = await vi.importActual<
-    typeof import('@shopify/cli-kit/node/node-package-manager')
-  >('@shopify/cli-kit/node/node-package-manager');
+    typeof import('../../lib/package-managers.js')
+  >('../../lib/package-managers.js');
   return {
     ...original,
-    getPackageManager: vi.fn(() => Promise.resolve('pnpm')),
+    findPackageManagerByLockfile: vi.fn(() => Promise.resolve('pnpm')),
+  };
+});
+
+vi.mock('@shopify/cli-kit/node/system', async () => {
+  const original = await vi.importActual<
+    typeof import('@shopify/cli-kit/node/system')
+  >('@shopify/cli-kit/node/system');
+
+  return {
+    ...original,
+    // Delegates to the real exec by default so helpers that drive git keep
+    // working; individual tests override it to capture command invocations.
+    exec: vi.fn(original.exec),
   };
 });
 
@@ -2010,6 +2026,130 @@ describe('upgrade', async () => {
       expect(args).toEqual(result);
     });
   });
+
+  // Regression coverage for monorepo workspaces, where the lockfile lives at
+  // the workspace root rather than in the app directory. These tests use the
+  // real findPackageManagerByLockfile (not the suite-wide 'pnpm' stub) and
+  // execute the command's task/summary paths end-to-end, so they fail if the
+  // upgrade command stops feeding ancestor-lockfile detection into the
+  // install/remove tasks or the undo instructions.
+  describe('monorepo package manager detection', () => {
+    afterEach(async () => {
+      // Restore the suite-wide stubs for the rest of the file, since the
+      // module-level mock implementations persist across tests.
+      vi.mocked(
+        packageManagers.findPackageManagerByLockfile,
+      ).mockImplementation(() => Promise.resolve('pnpm'));
+      const actualSystem = await vi.importActual<
+        typeof import('@shopify/cli-kit/node/system')
+      >('@shopify/cli-kit/node/system');
+      vi.mocked(system.exec).mockImplementation(actualSystem.exec);
+    });
+
+    async function useRealLockfileDetection() {
+      const {findPackageManagerByLockfile} = await vi.importActual<
+        typeof import('../../lib/package-managers.js')
+      >('../../lib/package-managers.js');
+      vi.mocked(
+        packageManagers.findPackageManagerByLockfile,
+      ).mockImplementation(findPackageManagerByLockfile);
+    }
+
+    it('runs the install and remove tasks with the workspace package manager from an ancestor lockfile', async () => {
+      await useRealLockfileDetection();
+
+      const {releases} = await getChangelog();
+      const selectedRelease = releases.find(
+        (release) => release.version === '2023.10.0',
+      ) as (typeof releases)[0];
+
+      const currentDependencies = {
+        ...OUTDATED_HYDROGEN_PACKAGE_JSON.dependencies,
+        ...OUTDATED_HYDROGEN_PACKAGE_JSON.devDependencies,
+      };
+
+      await inTemporaryDirectory(async (workspaceRoot) => {
+        // Monorepo shape: the only lockfile is at the workspace root and the
+        // Hydrogen app lives in a nested directory with no lockfile of its own.
+        await writeFile(joinPath(workspaceRoot, 'pnpm-lock.yaml'), '');
+        const appPath = joinPath(workspaceRoot, 'apps', 'storefront');
+        await mkdir(appPath, {recursive: true});
+
+        // Capture the package-manager invocation without running a real install.
+        const execSpy = vi
+          .mocked(system.exec)
+          .mockResolvedValue(undefined as never);
+
+        await upgradeNodeModules({
+          appPath,
+          selectedRelease,
+          currentDependencies,
+          // present in currentDependencies, so the removal task is created
+          cumulativeRemoveDependencies: ['@remix-run/react'],
+          cumulativeRemoveDevDependencies: [],
+        });
+
+        // renderTasks is stubbed to a no-op, so run the task callbacks directly.
+        const tasks = vi.mocked(renderTasks).mock.calls.at(-1)?.[0] as Array<{
+          title: string;
+          task: () => Promise<void>;
+        }>;
+
+        const removalTask = tasks.find(
+          (task) => task.title === 'Removing deprecated dependencies',
+        );
+        const upgradeTask = tasks.find(
+          (task) => task.title === 'Upgrading dependencies',
+        );
+
+        await removalTask?.task();
+        await upgradeTask?.task();
+
+        expect(execSpy).toHaveBeenCalledWith(
+          'pnpm',
+          expect.arrayContaining(['remove', '@remix-run/react']),
+          expect.objectContaining({cwd: appPath}),
+        );
+        expect(execSpy).toHaveBeenCalledWith(
+          'pnpm',
+          expect.arrayContaining(['add']),
+          expect.objectContaining({cwd: appPath}),
+        );
+        // Never falls back to npm + --legacy-peer-deps for a pnpm workspace.
+        expect(execSpy).not.toHaveBeenCalledWith(
+          'npm',
+          expect.anything(),
+          expect.anything(),
+        );
+      });
+    });
+
+    it('renders the workspace package manager in the undo instructions for an ancestor lockfile', async () => {
+      await useRealLockfileDetection();
+
+      const {releases} = await getChangelog();
+      const selectedRelease = releases.find(
+        (release) => release.version === '2023.10.0',
+      ) as (typeof releases)[0];
+
+      await inTemporaryDirectory(async (workspaceRoot) => {
+        await writeFile(joinPath(workspaceRoot, 'pnpm-lock.yaml'), '');
+        const appPath = joinPath(workspaceRoot, 'apps', 'storefront');
+        await mkdir(appPath, {recursive: true});
+
+        await displayUpgradeSummary({
+          appPath,
+          currentVersion: '2023.1.6',
+          selectedRelease,
+        });
+
+        const output = outputMock.info();
+        expect(output).toContain('Undo these upgrades?');
+        expect(output).toContain('&& pnpm i`');
+        expect(output).not.toContain('&& npm i`');
+      });
+    });
+  });
 });
 
 // cumulative result when upgrading from 2023.1.6 (outdated) to 2023.4.1

packages/cli/src/commands/hydrogen/upgrade.ts

@@ -23,9 +23,9 @@ import {
 } from '@shopify/cli-kit/node/fs';
 import {
   getDependencies,
-  getPackageManager,
   type PackageJson,
 } from '@shopify/cli-kit/node/node-package-manager';
+import {findPackageManagerByLockfile} from '../../lib/package-managers.js';
 import {exec} from '@shopify/cli-kit/node/system';
 import {AbortError} from '@shopify/cli-kit/node/error';
 import {dirname, joinPath, resolvePath} from '@shopify/cli-kit/node/path';
@@ -1031,7 +1031,7 @@ export async function upgradeNodeModules({
       task: async () => {
         await uninstallNodeModules({
           directory: appPath,
-          packageManager: await getPackageManager(appPath),
+          packageManager: await findPackageManagerByLockfile(appPath),
           args: depsToRemove,
         });
       },
@@ -1053,7 +1053,7 @@ export async function upgradeNodeModules({
     tasks.push({
       title: `Upgrading dependencies`,
       task: async () => {
-        const packageManager = await getPackageManager(appPath);
+        const packageManager = await findPackageManagerByLockfile(appPath);
         const command =
           packageManager === 'npm'
             ? 'install'
@@ -1256,7 +1256,7 @@ async function promptUpgradeOptions(
 /**
  * Displays a summary of the upgrade and next steps
  */
-async function displayUpgradeSummary({
+export async function displayUpgradeSummary({
   appPath,
   currentVersion,
   selectedRelease,
@@ -1299,7 +1299,9 @@ async function displayUpgradeSummary({
     ? `You've upgraded Hydrogen ${selectedPinnedVersion} dependencies`
     : `You've upgraded from ${fromToMsg}`;
 
-  const packageManager = await getPackageManager(appPath);
+  const packageManager = resolvePackageManagerName(
+    await findPackageManagerByLockfile(appPath),
+  );
 
   return renderSuccess({
     headline,

packages/cli/src/lib/package-managers.test.ts

@@ -0,0 +1,89 @@
+import {describe, it, expect} from 'vitest';
+import {inTemporaryDirectory, writeFile, mkdir} from '@shopify/cli-kit/node/fs';
+import {joinPath} from '@shopify/cli-kit/node/path';
+import {findPackageManagerByLockfile} from './package-managers.js';
+
+describe('findPackageManagerByLockfile()', () => {
+  it('detects a lockfile in the directory itself (single-repo)', async () => {
+    await inTemporaryDirectory(async (tmpDir) => {
+      await writeFile(joinPath(tmpDir, 'pnpm-lock.yaml'), '');
+
+      await expect(findPackageManagerByLockfile(tmpDir)).resolves.toBe('pnpm');
+    });
+  });
+
+  it('walks up to an ancestor lockfile (monorepo workspace root)', async () => {
+    await inTemporaryDirectory(async (tmpDir) => {
+      const appDir = joinPath(tmpDir, 'apps', 'storefront');
+      await mkdir(appDir);
+      await writeFile(joinPath(tmpDir, 'pnpm-lock.yaml'), '');
+
+      await expect(findPackageManagerByLockfile(appDir)).resolves.toBe('pnpm');
+    });
+  });
+
+  it('detects yarn workspaces via an ancestor yarn.lock', async () => {
+    await inTemporaryDirectory(async (tmpDir) => {
+      const appDir = joinPath(tmpDir, 'apps', 'storefront');
+      await mkdir(appDir);
+      await writeFile(joinPath(tmpDir, 'yarn.lock'), '');
+
+      await expect(findPackageManagerByLockfile(appDir)).resolves.toBe('yarn');
+    });
+  });
+
+  it('detects bun workspaces via an ancestor bun.lockb', async () => {
+    await inTemporaryDirectory(async (tmpDir) => {
+      const appDir = joinPath(tmpDir, 'apps', 'storefront');
+      await mkdir(appDir);
+      await writeFile(joinPath(tmpDir, 'bun.lockb'), '');
+
+      await expect(findPackageManagerByLockfile(appDir)).resolves.toBe('bun');
+    });
+  });
+
+  it('prefers the nearest lockfile (app dir beats ancestor)', async () => {
+    await inTemporaryDirectory(async (tmpDir) => {
+      const appDir = joinPath(tmpDir, 'apps', 'storefront');
+      await mkdir(appDir);
+      await writeFile(joinPath(tmpDir, 'pnpm-lock.yaml'), '');
+      await writeFile(joinPath(appDir, 'package-lock.json'), '');
+
+      await expect(findPackageManagerByLockfile(appDir)).resolves.toBe('npm');
+    });
+  });
+
+  it('resolves "unknown" when no lockfile exists anywhere', async () => {
+    await inTemporaryDirectory(async (tmpDir) => {
+      // stopAt keeps the walk hermetic against lockfiles above the OS temp dir
+      await expect(
+        findPackageManagerByLockfile(tmpDir, {stopAt: tmpDir}),
+      ).resolves.toBe('unknown');
+    });
+  });
+
+  it('detects bun.lock (text-based alternative lockfile)', async () => {
+    await inTemporaryDirectory(async (tmpDir) => {
+      await writeFile(joinPath(tmpDir, 'bun.lock'), '');
+
+      await expect(findPackageManagerByLockfile(tmpDir)).resolves.toBe('bun');
+    });
+  });
+
+  it('detects npm-shrinkwrap.json (alternative npm lockfile)', async () => {
+    await inTemporaryDirectory(async (tmpDir) => {
+      await writeFile(joinPath(tmpDir, 'npm-shrinkwrap.json'), '');
+
+      await expect(findPackageManagerByLockfile(tmpDir)).resolves.toBe('npm');
+    });
+  });
+
+  it('matches cli-kit precedence when multiple lockfiles coexist (yarn wins over npm)', async () => {
+    await inTemporaryDirectory(async (tmpDir) => {
+      await writeFile(joinPath(tmpDir, 'yarn.lock'), '');
+      await writeFile(joinPath(tmpDir, 'package-lock.json'), '');
+
+      await expect(findPackageManagerByLockfile(tmpDir)).resolves.toBe('yarn');
+    });
+  });
+});

packages/cli/src/lib/package-managers.ts

@@ -1,3 +1,5 @@
+import {fileExists} from '@shopify/cli-kit/node/fs';
+import {dirname, joinPath, resolvePath} from '@shopify/cli-kit/node/path';
 import {
   type Lockfile,
   type PackageManager as Name,
@@ -34,3 +36,49 @@ export const packageManagers: PackageManager[] = [
     installCommand: 'bun install --frozen-lockfile',
   },
 ];
+
+// Matches cli-kit's lockfile precedence (yarn, pnpm, bun, npm) so the
+// tie-break is unchanged when multiple lockfiles coexist in one directory.
+const lockfileDetectionOrder: Name[] = ['yarn', 'pnpm', 'bun', 'npm'];
+
+/**
+ * Detects the package manager by searching for a known lockfile in
+ * `directory` and, if none is found there, each ancestor directory in turn.
+ * Monorepo workspaces keep the lockfile at the workspace root rather than in
+ * each app directory, so checking only the app directory misses it. The
+ * nearest lockfile wins, preserving single-repo behavior. Returns 'unknown'
+ * when no lockfile exists up to `options.stopAt` (or the filesystem root).
+ */
+export async function findPackageManagerByLockfile(
+  directory: string,
+  options?: {stopAt?: string},
+): Promise<Name> {
+  let currentDirectory = resolvePath(directory);
+  const stopAt = options?.stopAt ? resolvePath(options.stopAt) : undefined;
+
+  for (;;) {
+    for (const name of lockfileDetectionOrder) {
+      const packageManager = packageManagers.find((pm) => pm.name === name);
+      if (!packageManager) continue;
+
+      const lockfiles = [
+        packageManager.lockfile,
+        ...(packageManager.alternativeLockfiles ?? []),
+      ];
+
+      for (const lockfile of lockfiles) {
+        if (await fileExists(joinPath(currentDirectory, lockfile))) {
+          return packageManager.name;
+        }
+      }
+    }
+
+    if (currentDirectory === stopAt) break;
+
+    const parentDirectory = dirname(currentDirectory);
+    if (parentDirectory === currentDirectory) break;
+    currentDirectory = parentDirectory;
+  }
+
+  return 'unknown';
+}