Skip to content

Fix package manager detection in hydrogen upgrade for monorepos#3794

Open
JohnCashmore wants to merge 5 commits into
Shopify:mainfrom
JohnCashmore:fix/upgrade-monorepo-package-manager-detection
Open

Fix package manager detection in hydrogen upgrade for monorepos#3794
JohnCashmore wants to merge 5 commits into
Shopify:mainfrom
JohnCashmore:fix/upgrade-monorepo-package-manager-detection

Conversation

@JohnCashmore

Copy link
Copy Markdown

WHY are these changes introduced?

Fixes #3793

When a Hydrogen app lives inside a monorepo workspace (pnpm/yarn/bun), the lockfile sits at the workspace root — not in the app directory. hydrogen upgrade detects the package manager with cli-kit's getPackageManager(appPath), which only checks that single directory for a lockfile before falling back to npm. The result: the upgrade runs npm install --legacy-peer-deps inside a pnpm workspace, which fails on workspace: protocol dependencies (or pollutes the app dir with a stray package-lock.json), and the final "Undo these upgrades?" message prints npm i instead of the workspace's package manager.

WHAT is this pull request doing?

  • Adds findPackageManagerByLockfile(directory, options?) to packages/cli/src/lib/package-managers.ts: walks up from the app directory looking for a known lockfile, built on the existing packageManagers metadata table (including alternative lockfiles such as bun.lock and npm-shrinkwrap.json).
    • Nearest lockfile wins — a lockfile in the app directory itself beats any ancestor, so single-repo behavior is unchanged.
    • Same-directory tie-break matches cli-kit's precedence (yarn → pnpm → bun → npm), so projects with multiple lockfiles in one directory behave exactly as before.
    • No lockfile anywhere → returns 'unknown', preserving the existing npm + --legacy-peer-deps fallback path untouched.
  • Replaces the three getPackageManager(appPath) call sites in upgrade.ts (dependency removal, dependency upgrade, and the undo-instructions message) with the new helper. The undo message is additionally wrapped in resolvePackageManagerName() so it can never render the literal string unknown.
  • Migrates the upgrade.test.ts package-manager mock to the new module and adds 9 unit tests covering: single-repo detection, monorepo ancestor detection (pnpm/yarn/bun), nearest-wins precedence, no-lockfile fallback, alternative lockfiles, and the same-directory tie-break order.
  • Adds a changeset (patch bump for @shopify/cli-hydrogen).

Out of scope (left untouched, happy to follow up): the same single-directory detection pattern exists in build.ts, setup/css.ts, setup/vite.ts, and lib/shell.ts.

HOW to test your changes?

Unit tests:

pnpm --filter @shopify/cli-hydrogen test src/lib/package-managers.test.ts src/commands/hydrogen/upgrade.test.ts

Manual end-to-end:

  1. Create a pnpm workspace (pnpm-workspace.yaml with packages: ['apps/*']) containing a Hydrogen app at apps/storefront pinned to an older Hydrogen version. Run pnpm install from the root (single pnpm-lock.yaml at the root, no lockfile in the app dir) and commit.
  2. Build this branch's CLI (pnpm --filter @shopify/cli-hydrogen build) and run the upgrade against the app directory.
  3. Observe: the "Upgrading dependencies" task runs pnpm add … (previously npm install --legacy-peer-deps), no package-lock.json appears in the app dir, the root pnpm-lock.yaml updates, and the closing undo message reads … && pnpm i.

Checklist

  • I've read the Contributing Guidelines
  • I've considered possible cross-platform impacts (Mac, Linux, Windows) — the walk-up loop uses cli-kit's resolvePath/dirname/joinPath and terminates when dirname(dir) === dir, which holds at filesystem roots on both POSIX (/) and Windows drive roots (C:\)
  • I've added a changeset if this PR contains user-facing or functional changes. Test changes or internal-only config changes do not require a changeset.
  • I've added tests to cover my changes
  • I've added or updated the documentation — n/a: behavior fix with no doc pages affected; the changeset carries the user-facing release note

@JohnCashmore JohnCashmore requested a review from a team as a code owner June 10, 2026 11:02
@JohnCashmore

Copy link
Copy Markdown
Author

I have signed the CLA!

@fredericoo fredericoo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all looks good except one test, pls revisit – then im happy to get this onto the next release!

return {
...original,
getPackageManager: vi.fn(() => Promise.resolve('pnpm')),
findPackageManagerByLockfile: vi.fn(() => Promise.resolve('pnpm')),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking: this still lets the regression pass if the upgrade command is not actually wired to the new detector everywhere.

The helper tests prove findPackageManagerByLockfile() can walk up to an ancestor lockfile, but the upgradeNodeModules() tests mock renderTasks, so the install/remove task callbacks that call the detector never run. The final undo-message path also isn't asserted, so it could still print npm i for a pnpm workspace.

Let's add regression coverage that proves the command uses ancestor-lockfile detection end-to-end: execute the upgrade task callback and assert exec is called with pnpm, and assert the summary/undo instructions render pnpm i for the same monorepo shape.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch @fredericoo — I've added regression coverage that runs the real detector against a monorepo shape (lockfile at the workspace root, app nested without its own): it asserts the upgrade/remove tasks invoke pnpm and that the undo instructions render pnpm i rather than npm i.

I think this covers what you were after, but happy to adjust if there's a project standard for this kind of coverage I should follow instead.

@JohnCashmore JohnCashmore force-pushed the fix/upgrade-monorepo-package-manager-detection branch from 0b4cf57 to 056f718 Compare June 16, 2026 13:41
… detection

Execute the upgrade/remove task callbacks and the undo-summary path with
the real findPackageManagerByLockfile against a monorepo shape (ancestor
pnpm-lock.yaml, app nested without its own lockfile), proving the upgrade
command feeds ancestor-lockfile detection into exec and the undo
instructions rather than relying on the suite-wide stub.
@JohnCashmore JohnCashmore force-pushed the fix/upgrade-monorepo-package-manager-detection branch from a40e796 to 3483396 Compare June 16, 2026 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hydrogen upgrade detects the wrong package manager in monorepos and runs npm inside pnpm/yarn/bun workspaces

2 participants