From be228923f97bb71fa7ddb4e98084d2de87564c6a Mon Sep 17 00:00:00 2001 From: Chancellor Clark Date: Tue, 25 Aug 2026 09:43:49 -0600 Subject: [PATCH] feat(cli): TRAC-1536 Reconcile Catalyst dependency versions on upgrade Every Catalyst tag ships core/package.json with `workspace:` specifiers, because @bigcommerce/catalyst-core is private and pnpm's publish-time rewrite never runs on the tagged tree. Both sides of the 3-way merge therefore agreed on those lines, so a project's @bigcommerce/catalyst* dependency versions never moved through an upgrade: flat projects kept whatever `catalyst create` resolved at scaffold time, and monorepo- structure projects kept `workspace:^` indefinitely. Read the versions each tag actually published from that tag's own tarball and pin the downloaded base and target trees to them, so the existing merge carries the bump like any other change, or conflicts when the merchant pinned deliberately. A dependency the project still holds as `workspace:` is skipped on both sides, since normalizing one side alone would manufacture a conflict on something that works fine as-is; those are surfaced instead as an opt-in migration prompt. Also add two advisories the merge cannot cover: a reminder to reinstall when the upgrade changed package.json, and a notice when the pinned @bigcommerce/catalyst is behind the published version, which it always will be eventually since `catalyst create` pins it exactly and the dependency does not exist upstream for a merge to touch. Refs TRAC-1536 Co-Authored-By: Claude Opus 5 (1M context) --- .../upgrade-catalyst-dependency-migration.md | 11 + .../src/cli/commands/upgrade.action.spec.ts | 69 +++ .../cli/commands/upgrade.integration.spec.ts | 64 +++ .../catalyst/src/cli/commands/upgrade.spec.ts | 262 +++++++++++ packages/catalyst/src/cli/commands/upgrade.ts | 405 +++++++++++++++++- .../src/cli/lib/detect-package-manager.ts | 29 ++ .../src/cli/lib/rewrite-core-package.ts | 12 +- packages/catalyst/tests/mocks/handlers.ts | 6 + 8 files changed, 835 insertions(+), 23 deletions(-) create mode 100644 .changeset/upgrade-catalyst-dependency-migration.md diff --git a/.changeset/upgrade-catalyst-dependency-migration.md b/.changeset/upgrade-catalyst-dependency-migration.md new file mode 100644 index 0000000000..d01a85482a --- /dev/null +++ b/.changeset/upgrade-catalyst-dependency-migration.md @@ -0,0 +1,11 @@ +--- +"@bigcommerce/catalyst": minor +--- + +`catalyst upgrade` now keeps your `@bigcommerce/catalyst*` dependencies up to date. + +Until now these versions never moved during an upgrade, so a project stayed pinned to whatever it was created with. The upgrade now brings them to the versions that shipped with the release you're upgrading to, handled like any other change: applied automatically, or flagged as a conflict if you'd pinned one on purpose. + +If your project still references these packages with `workspace:^`, the upgrade offers to swap them for published versions so your package manager can keep them current from then on. Declining, running without a terminal, or using `--dry-run` changes nothing; `--yes` accepts. + +Two new reminders round it out: run an install when the upgrade touched your `package.json`, and update `@bigcommerce/catalyst` itself when a newer version is out. diff --git a/packages/catalyst/src/cli/commands/upgrade.action.spec.ts b/packages/catalyst/src/cli/commands/upgrade.action.spec.ts index 1b7c9e513d..074d93de89 100644 --- a/packages/catalyst/src/cli/commands/upgrade.action.spec.ts +++ b/packages/catalyst/src/cli/commands/upgrade.action.spec.ts @@ -5,6 +5,7 @@ * Downloads use the shared CLI cache so subsequent runs are offline. */ +import { confirm } from '@inquirer/prompts'; import { execa } from 'execa'; import { cp, mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; @@ -16,6 +17,7 @@ vi.setConfig({ hookTimeout: 60_000 }); import { downloadCore, parseRef, upgrade } from './upgrade'; +vi.mock('@inquirer/prompts', () => ({ confirm: vi.fn() })); vi.mock('../lib/telemetry', () => ({ getTelemetry: () => ({ track: vi.fn() }) })); vi.mock('../lib/logger', () => ({ consola: { log: vi.fn(), success: vi.fn(), error: vi.fn(), warn: vi.fn(), info: vi.fn() }, @@ -34,6 +36,10 @@ const TIMEOUT = 120_000; const createdDirs: string[] = []; let originalCwd: string; +// vitest.setup.ts pins stdin to non-interactive; opt back in for the branches +// that are TTY-gated, and reset in afterEach. +const setTty = (value: boolean) => + Object.defineProperty(process.stdin, 'isTTY', { value, configurable: true }); // JSON.parse returns `any`; centralise the unsafe-return suppression here. // eslint-disable-next-line @typescript-eslint/no-unsafe-return @@ -53,6 +59,8 @@ beforeEach(() => { afterEach(async () => { process.chdir(originalCwd); + setTty(false); + vi.mocked(confirm).mockReset(); vi.restoreAllMocks(); await Promise.all( createdDirs @@ -341,3 +349,64 @@ test( }, TIMEOUT, ); + +// ── workspace-protocol dependency migration ────────────────────────────────── +// A project seeded straight from the tarball still carries `workspace:^`, which +// is exactly the monorepo-structure case: `catalyst create` never rewrote it. + +const CLIENT = '@bigcommerce/catalyst-client'; + +const readDeps = async (projectDir: string): Promise> => { + const pkg = parseJson(await readFile(join(projectDir, 'package.json'), 'utf-8')); + + // eslint-disable-next-line @typescript-eslint/consistent-type-assertions + return pkg.dependencies as Record; +}; + +// The gate is `stampedPkg && (--yes || (isTTY && confirm))`; these four rows walk it. +test.each([ + ['accepting the prompt migrates', [TARGET_VERSION], true, true, '^1.0.2'], + ['declining the prompt keeps workspace:', [TARGET_VERSION], true, false, 'workspace:^'], + ['--yes migrates without prompting', [TARGET_VERSION, '--yes'], true, undefined, '^1.0.2'], + ['a non-interactive run keeps workspace:', [TARGET_VERSION], false, undefined, 'workspace:^'], +] as const)( + '%s', + async (_name, argv, tty, confirmValue, expected) => { + const root = await mkTmp(); + const projectDir = await setup163Project(root); + + expect((await readDeps(projectDir))[CLIENT]).toBe('workspace:^'); + + setTty(tty); + + if (confirmValue !== undefined) vi.mocked(confirm).mockResolvedValue(confirmValue); + + process.chdir(projectDir); + await upgrade.parseAsync([...argv], { from: 'user' }); + + expect((await readDeps(projectDir))[CLIENT]).toBe(expected); + expect(confirm).toHaveBeenCalledTimes(confirmValue === undefined ? 0 : 1); + }, + TIMEOUT, +); + +test( + '--dry-run reports the workspace: references without rewriting them', + async () => { + const root = await mkTmp(); + const projectDir = await setup163Project(root); + + setTty(true); + + process.chdir(projectDir); + await upgrade.parseAsync([TARGET_VERSION, '--dry-run'], { from: 'user' }); + + expect(confirm).not.toHaveBeenCalled(); + expect((await readDeps(projectDir))[CLIENT]).toBe('workspace:^'); + + const status = (await execa('git', ['status', '--porcelain'], { cwd: projectDir })).stdout; + + expect(status.trim()).toBe(''); + }, + TIMEOUT, +); diff --git a/packages/catalyst/src/cli/commands/upgrade.integration.spec.ts b/packages/catalyst/src/cli/commands/upgrade.integration.spec.ts index 9bc7ebbd08..89a0bc1115 100644 --- a/packages/catalyst/src/cli/commands/upgrade.integration.spec.ts +++ b/packages/catalyst/src/cli/commands/upgrade.integration.spec.ts @@ -19,9 +19,13 @@ import { downloadCore, mergeCorePerFile, mergeCoreTree, + normalizeWorkspaceDeps, resolveProject, + rewriteWorkspaceSpecifier, } from './upgrade'; +const CLIENT = '@bigcommerce/catalyst-client'; + const SUPPORTS_TREE = (() => { try { const match = /(\d+)\.(\d+)/.exec(execSync('git --version').toString()); @@ -98,6 +102,66 @@ describe.each(engines)('integration (engine: %s)', (engine) => { ? mergeCoreTree(baseDir, theirsDir, oursDir) : mergeCorePerFile(baseDir, theirsDir, oursDir, emptyFile); + test( + 'bumps a Catalyst dependency across the real 1.6.3 → 1.7.0 tags', + async () => { + const root = await mkTmp(); + const baseDir = join(root, 'base'); + const theirsDir = join(root, 'theirs'); + const [baseVersions, targetVersions] = await Promise.all([ + downloadCore(REPO, BASE_REF, baseDir), + downloadCore(REPO, TARGET_REF, theirsDir), + ]); + + // The published client really did move between these two tags — the whole + // reason the dependency needs reconciling at all. + expect(baseVersions[CLIENT]).toBe('1.0.1'); + expect(targetVersions[CLIENT]).toBe('1.0.2'); + + // Both tags still carry the workspace specifier in their committed tree, + // which is why an unassisted merge leaves the merchant's version frozen. + expect(await readFile(join(baseDir, 'package.json'), 'utf-8')).toContain( + `"${CLIENT}": "workspace:^"`, + ); + + // A flat project: `catalyst create` resolved the specifier at scaffold + // time, so the merchant holds a real range. + const oursDir = join(root, 'project'); + + await cp(baseDir, oursDir, { recursive: true }); + + const pkgPath = join(oursDir, 'package.json'); + + await writeFile( + pkgPath, + rewriteWorkspaceSpecifier(await readFile(pkgPath, 'utf-8'), CLIENT, '1.0.1'), + ); + await initGitProject(oursDir); + + const { workspace, bumped } = await normalizeWorkspaceDeps( + await readFile(pkgPath, 'utf-8'), + baseDir, + theirsDir, + baseVersions, + targetVersions, + ); + + expect(bumped).toContain(CLIENT); + // The project isn't on the workspace protocol, so there's nothing to migrate. + expect(workspace.map((finding) => finding.name)).not.toContain(CLIENT); + + const emptyFile = join(root, '.empty'); + + await writeFile(emptyFile, ''); + + const result = await runMerge(baseDir, theirsDir, oursDir, emptyFile); + + expect(result.conflicted).not.toContain('package.json'); + expect(await readFile(pkgPath, 'utf-8')).toContain(`"${CLIENT}": "^1.0.2"`); + }, + TIMEOUT, + ); + test( 'clean project upgrades without conflicts and all changes staged', async () => { diff --git a/packages/catalyst/src/cli/commands/upgrade.spec.ts b/packages/catalyst/src/cli/commands/upgrade.spec.ts index f2b81f347f..dd1c705731 100644 --- a/packages/catalyst/src/cli/commands/upgrade.spec.ts +++ b/packages/catalyst/src/cli/commands/upgrade.spec.ts @@ -1,11 +1,15 @@ import { Command } from '@commander-js/extra-typings'; import { execa } from 'execa'; +import { http, HttpResponse } from 'msw'; import { execSync } from 'node:child_process'; import { access, mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { dirname, join } from 'node:path'; import { afterEach, describe, expect, test, vi } from 'vitest'; +import { server } from '../../../tests/mocks/node'; +import { detectLockfileManager } from '../lib/detect-package-manager'; + // The tree engine runs many git subprocesses sequentially on Windows CI; give // every test in this file enough headroom (the fast ones finish in < 1 s). vi.setConfig({ testTimeout: 30_000 }); @@ -13,12 +17,16 @@ vi.setConfig({ testTimeout: 30_000 }); import { applyIndexState, computeBaseSimilarity, + findStaleCli, mergeCorePerFile, mergeCoreTree, + migrateWorkspaceDeps, + normalizeWorkspaceDeps, parseRef, resolveBaseRef, resolveProject, resolveStrategy, + rewriteWorkspaceSpecifier, upgrade, } from './upgrade'; @@ -526,3 +534,257 @@ describe.skipIf(!SUPPORTS_TREE)('mergeCoreTree rename fidelity', () => { expect(await readFile(join(oursDir, 'new.ts'), 'utf-8')).toBe('a\nb\nc\nd\ne-merchant\nf\n'); }); }); + +// ── Catalyst dependency reconciliation ──────────────────────────────────────── + +const CLIENT = '@bigcommerce/catalyst-client'; +const ESLINT_CONFIG = '@bigcommerce/eslint-config-catalyst'; + +// Mirrors the shape of a real core/package.json closely enough for the textual +// rewrite: 2-space indent, deps split across two fields. +const corePkg = (client: string, eslintConfig: string): string => + `${JSON.stringify( + { + name: '@bigcommerce/catalyst-core', + version: '1.6.3', + dependencies: { [CLIENT]: client, next: '^15.5.0' }, + devDependencies: { [ESLINT_CONFIG]: eslintConfig }, + }, + null, + 2, + )}\n`; + +describe('rewriteWorkspaceSpecifier', () => { + test('swaps a workspace specifier for a caret range, leaving the rest byte-identical', () => { + const raw = corePkg('workspace:^', 'workspace:^'); + const rewritten = rewriteWorkspaceSpecifier(raw, CLIENT, '1.0.2'); + + expect(rewritten).toContain(`"${CLIENT}": "^1.0.2"`); + // Only that one value moved. + expect(rewritten).toBe(raw.replace('"workspace:^",', '"^1.0.2",')); + }); + + test('leaves a dependency that is already a real range alone', () => { + const raw = corePkg('^1.0.1', 'workspace:^'); + + expect(rewriteWorkspaceSpecifier(raw, CLIENT, '1.0.2')).toBe(raw); + }); + + test('is a no-op for a package that is not present', () => { + const raw = corePkg('workspace:^', 'workspace:^'); + + expect(rewriteWorkspaceSpecifier(raw, '@bigcommerce/nope', '9.9.9')).toBe(raw); + }); +}); + +describe('normalizeWorkspaceDeps', () => { + // Writes base/theirs trees carrying `workspace:^` (what every tag actually + // ships) and returns their dirs. + async function trees(root: string): Promise<{ baseDir: string; theirsDir: string }> { + const baseDir = join(root, 'base'); + const theirsDir = join(root, 'theirs'); + + await Promise.all([ + write(join(baseDir, 'package.json'), corePkg('workspace:^', 'workspace:^')), + write(join(theirsDir, 'package.json'), corePkg('workspace:^', 'workspace:^')), + ]); + + return { baseDir, theirsDir }; + } + + test('rewrites both sides to each tag version when the project holds a real range', async () => { + const root = await mkTmp(); + const { baseDir, theirsDir } = await trees(root); + + const result = await normalizeWorkspaceDeps( + corePkg('^1.0.1', '^1.0.0'), + baseDir, + theirsDir, + { [CLIENT]: '1.0.1', [ESLINT_CONFIG]: '1.0.0' }, + { [CLIENT]: '1.0.2', [ESLINT_CONFIG]: '1.0.0' }, + ); + + expect(await readFile(join(baseDir, 'package.json'), 'utf-8')).toContain( + `"${CLIENT}": "^1.0.1"`, + ); + expect(await readFile(join(theirsDir, 'package.json'), 'utf-8')).toContain( + `"${CLIENT}": "^1.0.2"`, + ); + expect(result.workspace).toEqual([]); + // eslint-config-catalyst didn't move between the two tags. + expect(result.bumped).toEqual([CLIENT]); + }); + + test('leaves both sides untouched for a dependency the project still holds as workspace:', async () => { + const root = await mkTmp(); + const { baseDir, theirsDir } = await trees(root); + const before = await readFile(join(baseDir, 'package.json'), 'utf-8'); + + const result = await normalizeWorkspaceDeps( + corePkg('workspace:^', 'workspace:^'), + baseDir, + theirsDir, + { [CLIENT]: '1.0.1', [ESLINT_CONFIG]: '1.0.0' }, + { [CLIENT]: '1.0.2', [ESLINT_CONFIG]: '1.0.0' }, + ); + + // Normalizing one side only would manufacture a conflict on a dependency + // that works fine as-is, so neither side moves. + expect(await readFile(join(baseDir, 'package.json'), 'utf-8')).toBe(before); + expect(await readFile(join(theirsDir, 'package.json'), 'utf-8')).toBe(before); + expect(result.bumped).toEqual([]); + expect(result.workspace).toEqual([ + { name: CLIENT, ours: 'workspace:^', version: '1.0.2' }, + { name: ESLINT_CONFIG, ours: 'workspace:^', version: '1.0.0' }, + ]); + }); + + test('leaves a side alone when that tag never published the package', async () => { + const root = await mkTmp(); + const { baseDir, theirsDir } = await trees(root); + + const result = await normalizeWorkspaceDeps( + corePkg('^1.0.1', '^1.0.0'), + baseDir, + theirsDir, + {}, // the package didn't exist at the base ref + { [CLIENT]: '1.0.2' }, + ); + + expect(await readFile(join(baseDir, 'package.json'), 'utf-8')).toContain( + `"${CLIENT}": "workspace:^"`, + ); + expect(await readFile(join(theirsDir, 'package.json'), 'utf-8')).toContain( + `"${CLIENT}": "^1.0.2"`, + ); + // Nothing to compare against on the base side, so no bump is claimed. + expect(result.bumped).toEqual([]); + }); + + test('returns nothing when a downloaded package.json is missing', async () => { + const root = await mkTmp(); + + await expect( + normalizeWorkspaceDeps( + corePkg('^1.0.1', '^1.0.0'), + join(root, 'nope'), + join(root, 'gone'), + {}, + {}, + ), + ).resolves.toEqual({ workspace: [], bumped: [] }); + }); +}); + +describe('migrateWorkspaceDeps', () => { + test('applies every finding to the project package.json text', () => { + const migrated = migrateWorkspaceDeps(corePkg('workspace:^', 'workspace:*'), [ + { name: CLIENT, ours: 'workspace:^', version: '1.0.2' }, + { name: ESLINT_CONFIG, ours: 'workspace:*', version: '1.0.0' }, + ]); + + expect(migrated).toContain(`"${CLIENT}": "^1.0.2"`); + expect(migrated).toContain(`"${ESLINT_CONFIG}": "^1.0.0"`); + expect(migrated).not.toContain('workspace:'); + }); +}); + +describe('findStaleCli', () => { + const withRegistryVersion = (version: string) => + server.use( + http.get('https://registry.npmjs.org/:scope/:name/latest', () => + HttpResponse.json({ name: '@bigcommerce/catalyst', version }), + ), + ); + + const projectWith = (cliRange: string) => + JSON.stringify({ devDependencies: { '@bigcommerce/catalyst': cliRange } }); + + test('reports the gap when the pinned CLI is behind the published one', async () => { + withRegistryVersion('1.2.0'); + + await expect(findStaleCli(projectWith('1.1.0'))).resolves.toEqual({ + current: '1.1.0', + latest: '1.2.0', + }); + }); + + test('stays quiet when the project is already on the published version', async () => { + withRegistryVersion('1.2.0'); + + await expect(findStaleCli(projectWith('1.2.0'))).resolves.toBeNull(); + }); + + test('stays quiet when the range already admits the published version', async () => { + withRegistryVersion('1.2.5'); + + // `^1.2.0` picks up 1.2.5 on the next install, so there is nothing to say. + await expect(findStaleCli(projectWith('^1.2.0'))).resolves.toBeNull(); + }); + + test('reports a range that cannot reach the published version', async () => { + withRegistryVersion('2.0.0'); + + await expect(findStaleCli(projectWith('^1.1.0'))).resolves.toEqual({ + current: '1.1.0', + latest: '2.0.0', + }); + }); + + test('stays quiet when the registry returns a non-semver version', async () => { + withRegistryVersion('not-a-version'); + + await expect(findStaleCli(projectWith('1.1.0'))).resolves.toBeNull(); + }); + + test('skips the registry entirely when the project has no CLI dependency', async () => { + await expect( + findStaleCli(JSON.stringify({ dependencies: { next: '^15.5.0' } })), + ).resolves.toBeNull(); + }); + + test('ignores an unparseable range rather than throwing', async () => { + await expect(findStaleCli(projectWith('catalyst.tgz'))).resolves.toBeNull(); + }); + + test('stays quiet when the registry is unreachable', async () => { + // The default handler 404s. + await expect(findStaleCli(projectWith('1.1.0'))).resolves.toBeNull(); + }); + + test('stays quiet when package.json still has conflict markers', async () => { + await expect( + findStaleCli('<<<<<<< ours\n{}\n=======\n{}\n>>>>>>> theirs\n'), + ).resolves.toBeNull(); + }); +}); + +describe('detectLockfileManager', () => { + test.each([ + ['pnpm-lock.yaml', 'pnpm'], + ['yarn.lock', 'yarn'], + ['bun.lock', 'bun'], + ['package-lock.json', 'npm'], + ])('maps %s to %s', async (lockfile, manager) => { + const root = await mkTmp(); + + await write(join(root, lockfile), ''); + + expect(await detectLockfileManager(root)).toBe(manager); + }); + + test('prefers pnpm when several lockfiles are present', async () => { + const root = await mkTmp(); + + await Promise.all([ + write(join(root, 'package-lock.json'), ''), + write(join(root, 'pnpm-lock.yaml'), ''), + ]); + + expect(await detectLockfileManager(root)).toBe('pnpm'); + }); + + test('returns null when there is no lockfile, so the caller can keep looking', async () => { + expect(await detectLockfileManager(await mkTmp())).toBeNull(); + }); +}); diff --git a/packages/catalyst/src/cli/commands/upgrade.ts b/packages/catalyst/src/cli/commands/upgrade.ts index 8f0b8ea94c..4908cafcfa 100644 --- a/packages/catalyst/src/cli/commands/upgrade.ts +++ b/packages/catalyst/src/cli/commands/upgrade.ts @@ -15,9 +15,11 @@ import { } from 'node:fs/promises'; import { homedir, tmpdir } from 'node:os'; import { dirname, join, relative } from 'node:path'; +import { minVersion, satisfies, lt as semverLt, validRange, valid as validSemver } from 'semver'; import yoctoSpinner from 'yocto-spinner'; import { z } from 'zod'; +import { detectLockfileManager, PackageManager } from '../lib/detect-package-manager'; import { consola } from '../lib/logger'; import { getTelemetry } from '../lib/telemetry'; @@ -27,6 +29,26 @@ const CorePackageJson = z.object({ catalyst: z.object({ version: z.string(), ref: z.string() }).optional(), }); +const DEP_FIELDS = [ + 'dependencies', + 'devDependencies', + 'optionalDependencies', + 'peerDependencies', +] as const; + +const DepsPackageJson = z.looseObject({ + dependencies: z.record(z.string(), z.string()).optional(), + devDependencies: z.record(z.string(), z.string()).optional(), + optionalDependencies: z.record(z.string(), z.string()).optional(), + peerDependencies: z.record(z.string(), z.string()).optional(), +}); + +const WorkspacePackageJson = z.object({ name: z.string(), version: z.string() }); + +// The CLI itself. `catalyst create` pins it exactly in devDependencies and it +// doesn't exist in the upstream tree, so no merge will ever move it. +const CATALYST_CLI_PACKAGE = '@bigcommerce/catalyst'; + // Catalyst versions are git tags on the upstream monorepo (e.g. // "@bigcommerce/catalyst-core@1.7.0"), NOT npm packages. GitHub serves a // tarball for any tag via the repos///tarball/ endpoint. @@ -125,6 +147,165 @@ async function readResolvedVersion(coreDir: string): Promise { return pkg.catalyst?.version ?? pkg.version; } +// ── Catalyst dependency reconciliation ──────────────────────────────────────── +// Every tag's committed core/package.json still carries `workspace:` specifiers +// for the Catalyst packages: core is `private`, so it is never published and +// pnpm's publish-time rewrite never runs on the tagged tree. Base and theirs +// therefore agree on those lines, the merge correctly leaves ours alone, and the +// merchant's Catalyst dependency versions never move. `catalyst create` resolves +// them at scaffold time instead, which is why flat projects hold a real range +// and monorepo-structure projects still hold `workspace:^`. + +// Package name → the version that tag published. Indexing can miss: a package +// that didn't exist yet at an older ref simply isn't in that tag's manifest. +export type WorkspaceVersions = Record; + +// A Catalyst dependency the merchant still references through the workspace +// protocol, alongside the published version it would migrate to. +export interface WorkspaceDepFinding { + name: string; + ours: string; + version: string; +} + +const readDepSpecifiers = (raw: string): Record => { + let parsed: z.infer; + + try { + parsed = DepsPackageJson.parse(JSON.parse(raw)); + } catch { + return {}; // conflict markers or hand-broken JSON — nothing to reconcile + } + + return Object.fromEntries(DEP_FIELDS.flatMap((field) => Object.entries(parsed[field] ?? {}))); +}; + +// Scoped package names carry "@" and "/", and some carry "."; escape before +// embedding in a pattern. +const escapeRegExp = (value: string) => value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + +// Swap a dependency's `workspace:` specifier for `^` in raw +// package.json text. Deliberately textual rather than a JSON round-trip: parsing +// and re-stringifying would reformat the whole file, desyncing the downloaded +// trees from the merchant's copy and turning a one-line dependency bump into a +// whole-file conflict. It also keeps working on a file that still has conflict +// markers in other sections. +export const rewriteWorkspaceSpecifier = (raw: string, name: string, version: string): string => + raw.replace( + new RegExp(`("${escapeRegExp(name)}"\\s*:\\s*)"workspace:[^"]*"`, 'g'), + (_match, key: string) => `${key}"^${version}"`, + ); + +// Rewrites the downloaded base/theirs package.json so each side names the +// version its own tag actually shipped. The normal 3-way merge then carries the +// bump for free — or conflicts, if the merchant pinned deliberately. +// +// A dependency the merchant still holds as `workspace:` is skipped on both +// sides: normalizing there would manufacture a conflict on something that works +// fine as-is. Those come back as findings for the migration prompt instead. +export async function normalizeWorkspaceDeps( + ourPkgRaw: string, + baseDir: string, + theirsDir: string, + baseVersions: WorkspaceVersions, + targetVersions: WorkspaceVersions, +): Promise<{ workspace: WorkspaceDepFinding[]; bumped: string[] }> { + const basePath = join(baseDir, 'package.json'); + const theirsPath = join(theirsDir, 'package.json'); + const [originalBase, originalTheirs] = await Promise.all([ + readFile(basePath, 'utf-8').catch(() => null), + readFile(theirsPath, 'utf-8').catch(() => null), + ]); + + if (originalBase === null || originalTheirs === null) return { workspace: [], bumped: [] }; + + const ourSpecifiers = readDepSpecifiers(ourPkgRaw); + const names = new Set( + [ + ...Object.entries(readDepSpecifiers(originalBase)), + ...Object.entries(readDepSpecifiers(originalTheirs)), + ] + .filter(([, range]) => range?.startsWith('workspace:')) + .map(([name]) => name), + ); + + const decisions = [...names].map((name) => ({ + name, + ours: ourSpecifiers[name], + baseVersion: baseVersions[name], + targetVersion: targetVersions[name], + })); + + const workspace = decisions.flatMap(({ name, ours, targetVersion }) => + ours?.startsWith('workspace:') && targetVersion !== undefined + ? [{ name, ours, version: targetVersion }] + : [], + ); + + // Everything else gets both sides pinned to the version its own tag shipped. + // A version is missing when the package didn't exist at that ref yet; leave + // that side untouched so the merge sees no change rather than a bogus one. + const normalizable = decisions.filter(({ ours }) => !ours?.startsWith('workspace:')); + + const baseRaw = normalizable.reduce( + (acc, { name, baseVersion }) => + baseVersion === undefined ? acc : rewriteWorkspaceSpecifier(acc, name, baseVersion), + originalBase, + ); + const theirsRaw = normalizable.reduce( + (acc, { name, targetVersion }) => + targetVersion === undefined ? acc : rewriteWorkspaceSpecifier(acc, name, targetVersion), + originalTheirs, + ); + const bumped = normalizable + .filter( + ({ baseVersion, targetVersion }) => + baseVersion !== undefined && targetVersion !== undefined && baseVersion !== targetVersion, + ) + .map(({ name }) => name); + + await Promise.all([ + baseRaw === originalBase ? null : writeFile(basePath, baseRaw), + theirsRaw === originalTheirs ? null : writeFile(theirsPath, theirsRaw), + ]); + + return { workspace, bumped }; +} + +// Applies the findings to the merchant's own package.json text. +export const migrateWorkspaceDeps = (raw: string, findings: WorkspaceDepFinding[]): string => + findings.reduce((acc, { name, version }) => rewriteWorkspaceSpecifier(acc, name, version), raw); + +// The CLI that performs upgrades can't upgrade itself through the merge, so +// check npm directly. Best-effort: a registry hiccup must never fail an upgrade. +export async function findStaleCli( + pkgRaw: string, +): Promise<{ current: string; latest: string } | null> { + const range = readDepSpecifiers(pkgRaw)[CATALYST_CLI_PACKAGE]; + + // A merchant may point the dep at a tarball, git URL, or `link:` path, none of + // which minVersion() can parse (it throws rather than returning null). + if (range === undefined || !validRange(range)) return null; + + const body: unknown = await fetch(`https://registry.npmjs.org/${CATALYST_CLI_PACKAGE}/latest`, { + // A hanging registry would otherwise stall the CLI indefinitely. + signal: AbortSignal.timeout(5000), + }) + .then((res) => (res.ok ? res.json() : null)) + .catch(() => null); + const latest = z.object({ version: z.string() }).safeParse(body).data?.version; + + // The registry promises a string, not a semver one, and an unparseable version + // would throw straight out of satisfies(). A range that already admits the + // published version needs no advisory either — the next install picks it up, + // which leaves this for the exact pin `catalyst create` writes. + if (latest === undefined || !validSemver(latest) || satisfies(latest, range)) return null; + + const current = minVersion(range)?.version; + + return current !== undefined && semverLt(current, latest) ? { current, latest } : null; +} + // ── per-file 3-way merge engine ─────────────────────────────────────────────── type MergeOutcome = 'applied' | 'added' | 'deleted' | 'conflicted'; @@ -620,31 +801,79 @@ export async function resolveProject(cwd: string): Promise { } // ── tarball download (source side stays the monorepo) ───────────────────────── +// Harvests the versions the monorepo's publishable packages carried at this ref. +// This is the only place they're knowable: the tag's core/package.json names them +// as `workspace:`, and they version independently of core (no changesets +// `linked`/`fixed` group), so an npm `latest` lookup would answer for today +// rather than for the ref being downloaded. +const readWorkspaceVersions = async (repoDir: string): Promise => { + const packagesDir = join(repoDir, 'packages'); + const entries = await readdir(packagesDir, { withFileTypes: true }).catch(() => []); + + const manifests = await Promise.all( + entries + .filter((entry) => entry.isDirectory()) + .map(async (entry) => { + const raw = await readFile(join(packagesDir, entry.name, 'package.json'), 'utf-8').catch( + () => null, + ); + + if (raw === null) return null; + + try { + return WorkspacePackageJson.parse(JSON.parse(raw)); + } catch { + return null; + } + }), + ); + + return Object.fromEntries( + manifests.filter((pkg) => pkg !== null).map(({ name, version }) => [name, version]), + ); +}; + +const readCachedVersions = async (path: string): Promise => { + const raw = await readFile(path, 'utf-8').catch(() => null); + + if (raw === null) return null; + + try { + return z.record(z.string(), z.string()).parse(JSON.parse(raw)); + } catch { + return null; + } +}; + export async function downloadCore( repository: string, ref: string, destDir: string, token?: string, -): Promise { +): Promise { // Never cache moving tags (latest/canary/alpha) — they'd go stale silently. const cacheable = !MOVING_TAGS.has(parseRef(ref).version); // Use a separator before the ref so that repository strings differing only // by characters that sanitize to '_' (e.g. '/' vs '_') produce distinct keys. const cacheKey = `${repository.replace(/[^a-zA-Z0-9._-]/g, '_')}__${ref.replace(/[^a-zA-Z0-9._-]/g, '_')}`; const cachePath = join(CACHE_DIR, cacheKey); + const versionsPath = `${cachePath}.versions.json`; // Guard against a partially-written cache entry: two concurrent downloadCore // calls for the same ref can race — the first creates cachePath mid-copy, and // the second sees it as present but gets an incomplete directory. Validate with - // a package.json sentinel (every extracted core/ tree must have one). - if ( - cacheable && - (await pathExists(cachePath)) && - (await pathExists(join(cachePath, 'package.json'))) - ) { + // a package.json sentinel (every extracted core/ tree must have one) plus the + // versions sidecar, which also makes entries written before the sidecar existed + // fall through to a fresh download rather than answering with no versions. + const cachedVersions = + cacheable && (await pathExists(join(cachePath, 'package.json'))) + ? await readCachedVersions(versionsPath) + : null; + + if (cachedVersions) { await cp(cachePath, destDir, { recursive: true }); - return; + return cachedVersions; } // encodeURIComponent turns "@bigcommerce/catalyst-core@1.7.0" into @@ -681,16 +910,23 @@ export async function downloadCore( const coreDir = join(rawDir, 'core'); const sourceDir = (await pathExists(coreDir)) ? coreDir : rawDir; + // Read the sibling packages before rawDir is torn down — destDir keeps core/ only. + const workspaceVersions = await readWorkspaceVersions(rawDir); + await rename(sourceDir, destDir); await rm(tarballPath, { force: true }); await rm(rawDir, { recursive: true, force: true }); if (cacheable) { await mkdir(CACHE_DIR, { recursive: true }); - await cp(destDir, cachePath, { recursive: true }).catch(() => { - /* best-effort cache; ignore failures */ - }); + await cp(destDir, cachePath, { recursive: true }) + .then(() => writeFile(versionsPath, JSON.stringify(workspaceVersions))) + .catch(() => { + /* best-effort cache; ignore failures */ + }); } + + return workspaceVersions; } // ── base-ref resolution / auto-detect ───────────────────────────────────────── @@ -727,7 +963,36 @@ export async function resolveBaseRef( } // ── summary output ──────────────────────────────────────────────────────────── -function printSummary(result: MergeResult, relDir: string, stampedPkg: boolean): void { +function printWorkspaceFindings(findings: WorkspaceDepFinding[]): void { + if (findings.length === 0) return; + + const pad = Math.max(...findings.map((finding) => finding.name.length)); + const rows = findings + .map((finding) => ` ${finding.name.padEnd(pad)} ${finding.ours} → ^${finding.version}`) + .join('\n'); + + consola.warn( + `Catalyst dependencies still using the workspace protocol (${findings.length}):\n${rows}\n\nThey resolve to your local packages/ copies, which \`catalyst upgrade\` does not update — so they never receive upgrades.`, + ); +} + +interface Summary { + result: MergeResult; + relDir: string; + stampedPkg: boolean; + migratedDeps: number; + packageManager: PackageManager; + staleCli: { current: string; latest: string } | null; +} + +function printSummary({ + result, + relDir, + stampedPkg, + migratedDeps, + packageManager, + staleCli, +}: Summary): void { // package.json is excluded from the conflict list when the stamp resolved it. const unresolved = result.conflicted.filter((f) => !(stampedPkg && f === 'package.json')); @@ -741,15 +1006,38 @@ function printSummary(result: MergeResult, relDir: string, stampedPkg: boolean): consola.log(`\n${parts.join(', ')}`); + // Any package.json change can move dependencies, and the upgrade never installs, + // so the lockfile is left behind either way. + const pkgConflicted = unresolved.includes('package.json'); + const depsChanged = + migratedDeps > 0 || + [...result.applied, ...result.added, ...result.conflicted].includes('package.json'); + + // Stay quiet while package.json itself is unresolved: an install against a + // manifest full of conflict markers just fails. It joins the resolution steps + // below instead, so it runs in the right order. + if (depsChanged && !pkgConflicted) { + consola.info( + `Dependencies changed — run \`${packageManager} install\` to update your lockfile.`, + ); + } + if (unresolved.length) { const files = unresolved.map((f) => ` ${f}`).join('\n'); + const install = depsChanged && pkgConflicted ? ` && ${packageManager} install` : ''; consola.warn( - `\nStaged the clean changes. ${unresolved.length} file(s) need conflict resolution (the <<>> markers):\n${files}\n\nResolve them, then: git add ${relDir} && git commit`, + `\nStaged the clean changes. ${unresolved.length} file(s) need conflict resolution (the <<>> markers):\n${files}\n\nResolve them, then: git add ${relDir} && git commit${install}`, ); } else { consola.success('Staged all changes — review with `git diff --cached`, then commit.'); } + + if (staleCli) { + consola.info( + `Your Catalyst CLI is behind (${staleCli.current} → ${staleCli.latest}). Update it with \`${packageManager} add -D ${CATALYST_CLI_PACKAGE}@${staleCli.latest}\`.`, + ); + } } export const upgrade = new Command('upgrade') @@ -893,10 +1181,20 @@ to raise the GitHub API rate limit.`, await writeFile(emptyFile, ''); + // Start the registry check now so its round-trip overlaps the tarball + // downloads rather than adding dead air after the merge. Reading the + // pre-merge manifest is also more robust: the CLI dependency isn't in + // either upstream tree, so nothing below can move it, and this way the + // advisory survives a merge that leaves conflict markers in package.json. + const staleCliCheck = findStaleCli(project.rawContent).catch(() => null); + const downloadSpinner = yoctoSpinner().start(`Downloading ${baseRef} and ${upstreamRef}...`); + let baseVersions: WorkspaceVersions; + let targetVersions: WorkspaceVersions; + try { - await Promise.all([ + [baseVersions, targetVersions] = await Promise.all([ downloadCore(options.repository, baseRef, baseDir, token), downloadCore(options.repository, upstreamRef, theirsDir, token), ]); @@ -931,6 +1229,18 @@ to raise the GitHub API rate limit.`, ? `${basePackage}@${resolvedVersion}` : upstreamRef; + // ── 3c. Teach the merge about Catalyst dependency versions ────────── + // Runs before the dry run so the preview shows the dependency change too. + // rawContent predates the 3b backfill, which only touches the `catalyst` + // field — dependency specifiers are identical either way. + const { workspace: workspaceDeps, bumped } = await normalizeWorkspaceDeps( + project.rawContent, + baseDir, + theirsDir, + baseVersions, + targetVersions, + ); + // ── 4. Dry run: show the unified diff and stop ────────────────────── if (options.dryRun) { const diffSpinner = yoctoSpinner().start('Generating diff...'); @@ -957,6 +1267,8 @@ to raise the GitHub API rate limit.`, consola.log('\nDiff preview (--dry-run, not applied):\n'); consola.log(diff.stdout); + printWorkspaceFindings(workspaceDeps); + return; } @@ -988,14 +1300,17 @@ to raise the GitHub API rate limit.`, // ── 6. Stamp catalyst.ref (skip if package.json itself conflicted) ── const patchedRaw = await readFile(pkgPath, 'utf-8'); - let stampedPkg = false; + // Non-null once the stamp has written clean JSON, and then it is exactly + // what sits on disk. Stays null when package.json came out of the merge + // carrying conflict markers. + let stampedText: string | null = null; try { const patchedPkg = z.record(z.string(), z.unknown()).parse(JSON.parse(patchedRaw)); patchedPkg.catalyst = { version: resolvedVersion, ref: newRef }; - await writeFile(pkgPath, `${JSON.stringify(patchedPkg, null, 2)}\n`); - stampedPkg = true; + stampedText = `${JSON.stringify(patchedPkg, null, 2)}\n`; + await writeFile(pkgPath, stampedText); consola.success(`catalyst.ref updated → ${newRef}`); } catch { // package.json has conflict markers (scripts, deps, etc.). The catalyst @@ -1020,7 +1335,7 @@ to raise the GitHub API rate limit.`, if (updated !== patchedRaw) { await writeFile(pkgPath, updated); - // stampedPkg stays false — the file still has conflict markers in other + // stampedText stays null — the file still has conflict markers in other // sections, so it must stay as a UU unmerged entry so the editor shows // the merge UI. Only the catalyst field was resolved in place. consola.success(`catalyst.ref updated → ${newRef}`); @@ -1034,11 +1349,44 @@ to raise the GitHub API rate limit.`, } } + // ── 6b. Offer to migrate workspace-protocol Catalyst dependencies ──── + printWorkspaceFindings(workspaceDeps); + + let migratedDeps = 0; + + if (workspaceDeps.length) { + // Rewriting a package.json that still holds conflict markers could land + // inside a hunk the merchant hasn't reviewed, so the swap is only ever + // applied to a file the stamp already parsed cleanly. Past that, --yes + // means yes (as it does for the inferred base ref), and a non-interactive + // run falls through to printing the versions. + const accepted = + stampedText !== null && + (options.yes === true || + (Boolean(process.stdin.isTTY) && + (await confirm({ + message: 'Replace them with the published versions?', + default: true, + }).catch(() => false)))); + + if (accepted && stampedText !== null) { + await writeFile(pkgPath, migrateWorkspaceDeps(stampedText, workspaceDeps)); + migratedDeps = workspaceDeps.length; + consola.success( + `Migrated ${migratedDeps} dependenc${migratedDeps === 1 ? 'y' : 'ies'} to published versions.`, + ); + } else { + consola.info( + "Keeping the workspace references. Switching abandons any local packages/ copy you've customized; otherwise apply the versions above in package.json and reinstall.", + ); + } + } + // ── 7. Stage the clean changes; mark conflicts as real unmerged entries ─ // Staging is a convenience; the merge already landed on disk, so never let // a git quirk here fail the whole upgrade. try { - await applyIndexState(gitRoot, relDir, baseDir, theirsDir, result, stampedPkg); + await applyIndexState(gitRoot, relDir, baseDir, theirsDir, result, stampedText !== null); } catch (err) { consola.warn( `Couldn't auto-stage the changes (${err instanceof Error ? err.message : String(err)}). Your files are merged on disk — run \`git add ${relDir}\` yourself.`, @@ -1049,6 +1397,12 @@ to raise the GitHub API rate limit.`, .then((r) => r.stdout.trim()) .catch(() => 'unknown'); + // The lockfile lives at the repo root for both layouts and is committed by + // every real project, so it answers this on its own. Catalyst scaffolds with + // pnpm, which makes it the least-surprising answer when there isn't one. + const packageManager = (await detectLockfileManager(gitRoot)) ?? 'pnpm'; + const staleCli = await staleCliCheck; + await getTelemetry().track('upgrade', { strategy, gitVersion, @@ -1058,9 +1412,20 @@ to raise the GitHub API rate limit.`, deleted: result.deleted.length, conflicts: result.conflicted.length, hasConflicts: result.conflicted.length > 0, + workspaceDepsFound: workspaceDeps.length, + workspaceDepsMigrated: migratedDeps, + catalystDepsBumped: bumped.length, + cliOutdated: staleCli !== null, }); - printSummary(result, relDir, stampedPkg); + printSummary({ + result, + relDir, + stampedPkg: stampedText !== null, + migratedDeps, + packageManager, + staleCli, + }); } finally { await rm(tmpDir, { recursive: true, force: true, maxRetries: 10, retryDelay: 500 }); } diff --git a/packages/catalyst/src/cli/lib/detect-package-manager.ts b/packages/catalyst/src/cli/lib/detect-package-manager.ts index 6e4e681dda..c9bad15f4a 100644 --- a/packages/catalyst/src/cli/lib/detect-package-manager.ts +++ b/packages/catalyst/src/cli/lib/detect-package-manager.ts @@ -1,7 +1,36 @@ +import { access } from 'node:fs/promises'; +import { join } from 'node:path'; import { detectPackageManager as detectFromDir } from 'nypm'; export type PackageManager = 'npm' | 'pnpm' | 'yarn' | 'bun'; +// Lockfile -> package manager, in nypm's own check order. +const PROJECT_LOCKFILES = [ + ['pnpm-lock.yaml', 'pnpm'], + ['yarn.lock', 'yarn'], + ['bun.lock', 'bun'], + ['bun.lockb', 'bun'], + ['package-lock.json', 'npm'], +] as const satisfies ReadonlyArray; + +// Detect a project's package manager from its lockfile alone, returning null +// when the directory has none so the caller can decide its own last resort. +// Unlike detectProjectPackageManager() this never throws — nypm parses +// package.json to read `packageManager`, which blows up on a manifest left full +// of merge conflict markers — and it can express "I don't know", which nypm +// can't, since it falls back to npm internally. +export const detectLockfileManager = async (dir: string): Promise => { + const found = await Promise.all( + PROJECT_LOCKFILES.map(async ([file, manager]) => + access(join(dir, file)) + .then(() => manager) + .catch(() => null), + ), + ); + + return found.find((manager) => manager !== null) ?? null; +}; + // Detect the package manager that INVOKED the CLI (via npx / pnpm dlx / yarn dlx // / bunx) by parsing the leading token of npm_config_user_agent, which every // manager sets when it spawns a child process. This is deliberately not diff --git a/packages/catalyst/src/cli/lib/rewrite-core-package.ts b/packages/catalyst/src/cli/lib/rewrite-core-package.ts index acf32a4d37..4fce363e43 100644 --- a/packages/catalyst/src/cli/lib/rewrite-core-package.ts +++ b/packages/catalyst/src/cli/lib/rewrite-core-package.ts @@ -45,9 +45,15 @@ const registryVersionSchema = z.object({ version: z.string() }); // Resolve the published npm version for a workspace dependency. The extracted // tag tree still carries `workspace:^` (core is private, so changesets' publish- // time rewrite never touches the committed tree) — so we resolve the real -// version at extraction time from the npm registry. The two workspace deps -// (`@bigcommerce/catalyst-client`, `@bigcommerce/eslint-config-catalyst`) are -// published in lockstep with core, so `latest` is the compatible version. +// version at extraction time from the npm registry. +// +// NOTE: `latest` answers for today, not for `ref`. These packages version +// independently of core (the changesets config has no `linked`/`fixed` group), +// so scaffolding from an older `--gh-ref` can splice in a newer version than +// that tag shipped. Harmless while they stay in step, wrong at the first major +// bump. `catalyst upgrade` reads the versions out of the tag's own +// `packages/*/package.json`; this path can't yet, because `extractCatalyst` +// filters the tarball down to `core/`. Tracked as a follow-up. const resolvePublishedVersion = async (pkgName: string): Promise => { const response = await fetch(`https://registry.npmjs.org/${pkgName}/latest`); diff --git a/packages/catalyst/tests/mocks/handlers.ts b/packages/catalyst/tests/mocks/handlers.ts index 3a7b91a497..5182d57fbe 100644 --- a/packages/catalyst/tests/mocks/handlers.ts +++ b/packages/catalyst/tests/mocks/handlers.ts @@ -309,4 +309,10 @@ export const handlers = [ data: { id: 1, url: 'https://example.com', channel_id: 1 }, }), ), + + // Default handler for the npm registry — 404 so the stale-CLI check stays + // silent by default. Tests that assert on it override with a version payload. + http.get('https://registry.npmjs.org/:scope/:name/latest', () => + HttpResponse.json({ error: 'Not found' }, { status: 404 }), + ), ];