From 7b981cfeeb47b656937e4bc84abf3a68693beb60 Mon Sep 17 00:00:00 2001 From: Riley Nowak Date: Fri, 27 Mar 2026 09:57:54 -0400 Subject: [PATCH 1/4] feat(cli): show removed packages in upgrade UX MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merchants upgrading across versions that remove packages (e.g. the Remix → React Router migration) had no visibility into what was being cleaned up. Now they see a "Removed packages" section in both the upgrade confirmation prompt and the generated instructions markdown. - Extract getAllRemovedPackages() helper to avoid duplicating the removeDependencies + removeDevDependencies concatenation across displayConfirmation and generateUpgradeInstructionsFile - Extend displayConfirmation to show removed packages list when present - Extend generateUpgradeInstructionsFile to include a "## Removed packages" section with backtick-wrapped package names - Export generateUpgradeInstructionsFile for testability Co-Authored-By: Claude Haiku 4.5 --- .changeset/removed-packages-ux.md | 5 +++ packages/cli/src/commands/hydrogen/upgrade.ts | 35 +++++++++++++++++-- 2 files changed, 37 insertions(+), 3 deletions(-) create mode 100644 .changeset/removed-packages-ux.md diff --git a/.changeset/removed-packages-ux.md b/.changeset/removed-packages-ux.md new file mode 100644 index 0000000000..3588938bd9 --- /dev/null +++ b/.changeset/removed-packages-ux.md @@ -0,0 +1,5 @@ +--- +'@shopify/cli-hydrogen': patch +--- + +Show removed packages in the upgrade confirmation prompt and upgrade instructions file so merchants can see which dependencies will be cleaned up during the upgrade. diff --git a/packages/cli/src/commands/hydrogen/upgrade.ts b/packages/cli/src/commands/hydrogen/upgrade.ts index 229a7046e8..e1eac3806f 100644 --- a/packages/cli/src/commands/hydrogen/upgrade.ts +++ b/packages/cli/src/commands/hydrogen/upgrade.ts @@ -83,6 +83,10 @@ export type CumulativeRelease = { devDependencies: Record; }; +function getAllRemovedPackages(release: CumulativeRelease): string[] { + return [...release.removeDependencies, ...release.removeDevDependencies]; +} + const INSTRUCTIONS_FOLDER = '.hydrogen'; export default class Upgrade extends Command { @@ -722,7 +726,9 @@ export function displayConfirmation({ targetVersion?: string; }) { const {features, fixes} = cumulativeRelease; - if (features.length || fixes.length) { + const allRemovedPackages = getAllRemovedPackages(cumulativeRelease); + + if (features.length || fixes.length || allRemovedPackages.length) { renderInfo({ headline: `Included in this upgrade:`, // @ts-expect-error - filter(Boolean) removes falsy values, leaving only objects @@ -747,6 +753,16 @@ export function displayConfirmation({ }, ], }, + allRemovedPackages.length && { + title: 'Removed packages', + body: [ + { + list: { + items: allRemovedPackages, + }, + }, + ], + }, ].filter(Boolean), }); } @@ -1394,7 +1410,7 @@ function generateStepMd(item: ReleaseItem) { /** * Generates a markdown file with upgrade instructions */ -async function generateUpgradeInstructionsFile({ +export async function generateUpgradeInstructionsFile({ appPath, cumulativeRelease, currentVersion, @@ -1428,7 +1444,14 @@ async function generateUpgradeInstructionsFile({ .filter((fixes) => fixes.steps) .map(generateStepMd); - if (!featuresMd.length && !fixesMd.length && !breakingChangesMd.length) { + const allRemovedPackages = getAllRemovedPackages(cumulativeRelease); + + if ( + !featuresMd.length && + !fixesMd.length && + !breakingChangesMd.length && + !allRemovedPackages.length + ) { renderInfo({ headline: `No upgrade instructions generated`, body: `There are no additional upgrade instructions for this version.`, @@ -1460,6 +1483,12 @@ async function generateUpgradeInstructionsFile({ )}`; } + if (allRemovedPackages.length) { + md += `\n----\n\n## Removed packages\n\nThe following packages have been removed as part of this upgrade:\n\n`; + md += allRemovedPackages.map((dep) => `- \`${dep}\``).join('\n'); + md += '\n'; + } + const filePath = joinPath(instructionsFolderPath, filename); try { From 2cd1b9f16e080b0c70eea51204d81df9377cccba Mon Sep 17 00:00:00 2001 From: Riley Nowak Date: Fri, 27 Mar 2026 09:58:22 -0400 Subject: [PATCH 2/4] test(cli): strengthen upgrade and changelog schema tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The upgrade command tests had weak assertions that could pass vacuously, and changelog validation coverage was lost when tests were deleted from upgrade-flow.test.ts. Upgrade test improvements: - Verify renderTasks receives specific task titles instead of just toHaveBeenCalled() - Assert removal task is absent when no deps to remove (direct mock inspection pattern) - Add tests for removed packages display in confirmation prompt and generated markdown instructions New changelog-schema.test.ts (~290 lines): - Validates changelog.json schema: allowed fields, required fields, URL/version/date formats, semver deps, base64 steps, dependenciesMeta - Uses semver.validRange() instead of a permissive regex for version validation — the regex accepted trailing garbage like 1.0.0!@#$% - Precondition assertions on required-field loops prevent vacuous passes (e.g. if all releases lost their features/fixes arrays, the test would still pass green without the precondition) - Optional fields (dependenciesMeta, removeDependencies) intentionally have no preconditions — a valid changelog may have zero instances Co-Authored-By: Claude Haiku 4.5 --- .../hydrogen/changelog-schema.test.ts | 287 ++++++++++++++++++ .../cli/src/commands/hydrogen/upgrade.test.ts | 111 ++++++- 2 files changed, 395 insertions(+), 3 deletions(-) create mode 100644 packages/cli/src/commands/hydrogen/changelog-schema.test.ts diff --git a/packages/cli/src/commands/hydrogen/changelog-schema.test.ts b/packages/cli/src/commands/hydrogen/changelog-schema.test.ts new file mode 100644 index 0000000000..6994429d0c --- /dev/null +++ b/packages/cli/src/commands/hydrogen/changelog-schema.test.ts @@ -0,0 +1,287 @@ +import {readFile} from 'node:fs/promises'; +import {join, dirname} from 'node:path'; +import {fileURLToPath} from 'node:url'; +import semver from 'semver'; +import {describe, expect, it} from 'vitest'; +import * as upgradeModule from './upgrade.js'; + +/** + * Validates the changelog.json schema and data integrity. + * + * Ported from the deleted `describe('Changelog validation')` block in + * upgrade-flow.test.ts (~270 lines). These tests guard against malformed + * changelog entries that could break the upgrade command at runtime. + */ +describe('Changelog validation', () => { + const allowedReleaseFields = new Set([ + 'title', + 'version', + 'date', + 'hash', + 'commit', + 'pr', + 'dependencies', + 'devDependencies', + 'dependenciesMeta', + 'removeDependencies', + 'removeDevDependencies', + 'features', + 'fixes', + ]); + + const allowedItemFields = new Set([ + 'title', + 'info', + 'pr', + 'id', + 'breaking', + 'docs', + 'steps', + 'desc', + 'code', + 'description', + ]); + + const allowedStepFields = new Set([ + 'title', + 'info', + 'code', + 'file', + 'reel', + 'desc', + 'docs', + ]); + + const urlRegex = /^https:\/\/.+/; + const versionRegex = /^\d{4}\.\d+\.\d+$/; + + // Resolve from the file's own location to avoid dependence on process.cwd() + const currentDir = dirname(fileURLToPath(import.meta.url)); + // This file lives at packages/cli/src/commands/hydrogen/ — navigate up to repo root + const changelogPath = join( + currentDir, + '..', + '..', + '..', + '..', + '..', + 'docs', + 'changelog.json', + ); + + it('is valid JSON and matches getChangelog() output', async () => { + const changelogContent = await readFile(changelogPath, 'utf8'); + + let parsedChangelog; + try { + parsedChangelog = JSON.parse(changelogContent); + } catch (error) { + throw new Error( + `Invalid JSON in changelog.json: ${(error as Error).message}`, + ); + } + + const changelog = await upgradeModule.getChangelog(); + expect(changelog).toEqual(parsedChangelog); + }); + + it('has only allowed top-level fields', async () => { + const changelog = await upgradeModule.getChangelog(); + const allowedChangelogFields = ['url', 'version', 'releases']; + const rogueChangelogFields = Object.keys(changelog).filter( + (key) => !allowedChangelogFields.includes(key), + ); + expect(rogueChangelogFields).toEqual([]); + }); + + it('has required fields and valid formats in every release', async () => { + const changelog = await upgradeModule.getChangelog(); + + for (const release of changelog.releases) { + // Fail fast on malformed entries rather than silently skipping them + expect(release).toBeDefined(); + if (!release) continue; + + expect(release.title).toBeDefined(); + expect(release.version).toBeDefined(); + expect(release.hash).toBeDefined(); + expect(release.commit).toBeDefined(); + expect(release.dependencies).toBeDefined(); + expect(release.devDependencies).toBeDefined(); + expect(release.features).toBeDefined(); + expect(release.fixes).toBeDefined(); + + if (release.pr) { + expect(typeof release.pr).toBe('string'); + } + expect(release.commit).toMatch(urlRegex); + expect(release.version).toMatch(versionRegex); + + if (release.date) { + expect(typeof release.date).toBe('string'); + expect(release.date.length).toBeGreaterThan(0); + } + } + }); + + it('has no rogue fields in any release', async () => { + const changelog = await upgradeModule.getChangelog(); + + for (const release of changelog.releases) { + // Fail fast on malformed entries rather than silently skipping them + expect(release).toBeDefined(); + if (!release) continue; + + const rogueReleaseFields = Object.keys(release).filter( + (key) => !allowedReleaseFields.has(key), + ); + expect(rogueReleaseFields).toEqual([]); + } + }); + + it('has valid feature/fix items with no rogue fields', async () => { + const changelog = await upgradeModule.getChangelog(); + + const allItems = changelog.releases.flatMap((r) => [ + ...(r.features ?? []), + ...(r.fixes ?? []), + ]); + expect(allItems.length).toBeGreaterThan(0); + + for (const release of changelog.releases) { + // Fail fast on malformed entries rather than silently skipping them + expect(release).toBeDefined(); + if (!release) continue; + + for (const item of [ + ...(release.features ?? []), + ...(release.fixes ?? []), + ]) { + if (!item) continue; + + const rogueItemFields = Object.keys(item).filter( + (key) => !allowedItemFields.has(key), + ); + expect(rogueItemFields).toEqual([]); + expect(item.title).toBeDefined(); + + if (item.pr) { + expect(typeof item.pr).toBe('string'); + } + } + } + }); + + it('has valid steps with decodable base64 code', async () => { + const changelog = await upgradeModule.getChangelog(); + + const allSteps = changelog.releases.flatMap((r) => + [...(r.features ?? []), ...(r.fixes ?? [])].flatMap( + (item) => item?.steps ?? [], + ), + ); + expect(allSteps.length).toBeGreaterThan(0); + + for (const release of changelog.releases) { + // Fail fast on malformed entries rather than silently skipping them + expect(release).toBeDefined(); + if (!release) continue; + + for (const item of [ + ...(release.features ?? []), + ...(release.fixes ?? []), + ]) { + if (!item?.steps) continue; + + expect(Array.isArray(item.steps)).toBe(true); + for (const step of item.steps) { + if (!step) continue; + + const rogueStepFields = Object.keys(step).filter( + (key) => !allowedStepFields.has(key), + ); + expect(rogueStepFields).toEqual([]); + expect(step.title).toBeDefined(); + + if (step.code) { + expect(() => + Buffer.from(step.code, 'base64').toString(), + ).not.toThrow(); + } + } + } + } + }); + + it('has valid semver versions for all dependencies', async () => { + const changelog = await upgradeModule.getChangelog(); + + const allDeps = changelog.releases.flatMap((r) => [ + ...Object.entries(r.dependencies ?? {}), + ...Object.entries(r.devDependencies ?? {}), + ]); + expect(allDeps.length).toBeGreaterThan(0); + + for (const release of changelog.releases) { + // Fail fast on malformed entries rather than silently skipping them + expect(release).toBeDefined(); + if (!release) continue; + + for (const [pkg, version] of Object.entries(release.dependencies ?? {})) { + expect(typeof pkg).toBe('string'); + expect(typeof version).toBe('string'); + expect(semver.validRange(version)).not.toBeNull(); + } + + for (const [pkg, version] of Object.entries( + release.devDependencies ?? {}, + )) { + expect(typeof pkg).toBe('string'); + expect(typeof version).toBe('string'); + expect(semver.validRange(version)).not.toBeNull(); + } + } + }); + + it('has valid dependenciesMeta structure', async () => { + const changelog = await upgradeModule.getChangelog(); + + for (const release of changelog.releases) { + if (!release?.dependenciesMeta) continue; + + for (const [pkg, meta] of Object.entries(release.dependenciesMeta)) { + expect(typeof pkg).toBe('string'); + expect(typeof meta).toBe('object'); + expect(typeof meta.required).toBe('boolean'); + const rogueMetaFields = Object.keys(meta).filter( + (key) => key !== 'required', + ); + expect(rogueMetaFields).toEqual([]); + } + } + }); + + it('has valid removeDependencies and removeDevDependencies arrays', async () => { + const changelog = await upgradeModule.getChangelog(); + + for (const release of changelog.releases) { + // Fail fast on malformed entries rather than silently skipping them + expect(release).toBeDefined(); + if (!release) continue; + + if (release.removeDependencies) { + expect(Array.isArray(release.removeDependencies)).toBe(true); + for (const dep of release.removeDependencies) { + expect(typeof dep).toBe('string'); + } + } + + if (release.removeDevDependencies) { + expect(Array.isArray(release.removeDevDependencies)).toBe(true); + for (const dep of release.removeDevDependencies) { + expect(typeof dep).toBe('string'); + } + } + } + }); +}); diff --git a/packages/cli/src/commands/hydrogen/upgrade.test.ts b/packages/cli/src/commands/hydrogen/upgrade.test.ts index 6053148d7d..467de5a49c 100644 --- a/packages/cli/src/commands/hydrogen/upgrade.test.ts +++ b/packages/cli/src/commands/hydrogen/upgrade.test.ts @@ -1,6 +1,6 @@ import {createRequire} from 'node:module'; import {tmpdir} from 'node:os'; -import {mkdtemp, rm} from 'node:fs/promises'; +import {mkdtemp, readFile, rm} from 'node:fs/promises'; import {describe, it, expect, vi, beforeEach} from 'vitest'; import {writeFile, fileExists} from '@shopify/cli-kit/node/fs'; import {joinPath} from '@shopify/cli-kit/node/path'; @@ -16,6 +16,7 @@ import {exec} from '@shopify/cli-kit/node/system'; import { buildUpgradeCommandArgs, displayConfirmation, + generateUpgradeInstructionsFile, getAbsoluteVersion, getAvailableUpgrades, getCumulativeRelease, @@ -1168,7 +1169,17 @@ describe('upgrade', async () => { cumulativeRelease.removeDevDependencies, }); - expect(renderTasks).toHaveBeenCalled(); + expect(renderTasks).toHaveBeenCalledWith( + expect.arrayContaining([ + expect.objectContaining({ + title: 'Removing deprecated dependencies', + }), + expect.objectContaining({ + title: 'Upgrading dependencies', + }), + ]), + expect.anything(), + ); }, { cleanGitRepo: true, @@ -1209,6 +1220,9 @@ describe('upgrade', async () => { expect(info).toMatch(feat.title.slice(0, 15)), ); + // With empty removeDependencies, the "Removed packages" section should NOT appear + expect(info).not.toMatch('Removed packages'); + expect(renderConfirmationPrompt).toHaveBeenCalledWith({ message: `Are you sure you want to upgrade to ${selectedRelease.version}?`, cancellationMessage: `No, choose another version`, @@ -1221,6 +1235,81 @@ describe('upgrade', async () => { }, ); }); + it('displays removed packages when cumulativeRelease has removals', async () => { + await inTemporaryHydrogenRepo( + async () => { + const {releases} = await getChangelog(); + + const selectedRelease = releases.find( + (release) => release.version === '2023.10.0', + ) as (typeof releases)[0]; + + const releaseWithRemovals = { + ...CUMULATIVE_RELEASE, + removeDependencies: ['@remix-run/react'], + removeDevDependencies: ['@remix-run/dev'], + }; + + await expect( + displayConfirmation({ + cumulativeRelease: releaseWithRemovals, + selectedRelease, + }), + ).resolves.toEqual(false); + + const info = outputMock.info(); + expect(info).toMatch('Removed packages'); + expect(info).toMatch('@remix-run/react'); + expect(info).toMatch('@remix-run/dev'); + }, + { + cleanGitRepo: true, + packageJson: OUTDATED_HYDROGEN_PACKAGE_JSON, + }, + ); + }); + }); + + describe('generateUpgradeInstructionsFile', () => { + it('includes backtick-wrapped removed packages in markdown output', async () => { + await inTemporaryHydrogenRepo( + async (appPath) => { + const {releases} = await getChangelog(); + + const selectedRelease = releases.find( + (release) => release.version === '2023.10.0', + ) as (typeof releases)[0]; + + const releaseWithRemovals: CumulativeRelease = { + ...CUMULATIVE_RELEASE, + removeDependencies: ['@remix-run/react'], + removeDevDependencies: ['@remix-run/dev'], + }; + + const resultPath = await generateUpgradeInstructionsFile({ + appPath, + cumulativeRelease: releaseWithRemovals, + currentVersion: '2023.1.6', + selectedRelease, + }); + + expect(resultPath).toBeDefined(); + + const mdContent = await readFile( + joinPath(appPath, resultPath!), + 'utf8', + ); + + expect(mdContent).toContain('## Removed packages'); + expect(mdContent).toContain('- `@remix-run/react`'); + expect(mdContent).toContain('- `@remix-run/dev`'); + }, + { + cleanGitRepo: true, + packageJson: OUTDATED_HYDROGEN_PACKAGE_JSON, + }, + ); + }); }); describe('displayDevUpgradeNotice', () => { @@ -1447,7 +1536,23 @@ describe('upgrade', async () => { cumulativeRemoveDevDependencies: [], }); - expect(renderTasks).toHaveBeenCalled(); + // With empty removal lists, only the upgrade task should be present + expect(renderTasks).toHaveBeenCalledWith( + expect.arrayContaining([ + expect.objectContaining({ + title: 'Upgrading dependencies', + }), + ]), + expect.anything(), + ); + + // The removal task should NOT be present when no deps to remove + const tasks = vi.mocked(renderTasks).mock.calls[0]?.[0] ?? []; + const removalTask = tasks.find( + (t: {title: string}) => + t.title === 'Removing deprecated dependencies', + ); + expect(removalTask).toBeUndefined(); }, { cleanGitRepo: true, From 1ab4c167ed68b63f43e62db2904668f2692a4c2d Mon Sep 17 00:00:00 2001 From: Riley Nowak Date: Fri, 10 Apr 2026 09:40:11 -0400 Subject: [PATCH 3/4] test: address PR review feedback on upgrade tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace fragile `mock.calls[0]?.[0]` traversal with `expect.not.arrayContaining` per kdaviduik's suggestion; the built-in matcher is resilient to call-order changes and removes the need for the inline `{title: string}` cast - Add negative test for `generateUpgradeInstructionsFile` asserting the "Removed packages" section is omitted when there are no removals - Add edge-case test for both `displayConfirmation` and `generateUpgradeInstructionsFile` when a release has only removals and no features/fixes — covering the new UX path noted by fredericoo - Extract `assertDefined` type assertion helper in `changelog-schema.test.ts` to replace the 6x repeated `expect(release).toBeDefined(); if (!release) continue;` pattern; after calling `assertDefined`, TypeScript narrows the type so the manual guard is no longer needed - Hoist `getChangelog()` into `beforeAll` in `changelog-schema.test.ts` to parse the static JSON once instead of once per test (~9 calls) - Update changeset wording per kdaviduik's nit Co-Authored-By: Claude Sonnet 4.6 --- .changeset/removed-packages-ux.md | 2 +- .../hydrogen/changelog-schema.test.ts | 69 ++++------ .../cli/src/commands/hydrogen/upgrade.test.ts | 129 +++++++++++++++++- 3 files changed, 151 insertions(+), 49 deletions(-) diff --git a/.changeset/removed-packages-ux.md b/.changeset/removed-packages-ux.md index 3588938bd9..d846f70b99 100644 --- a/.changeset/removed-packages-ux.md +++ b/.changeset/removed-packages-ux.md @@ -2,4 +2,4 @@ '@shopify/cli-hydrogen': patch --- -Show removed packages in the upgrade confirmation prompt and upgrade instructions file so merchants can see which dependencies will be cleaned up during the upgrade. +Show removed packages in the upgrade confirmation prompt and upgrade instructions file so that it's visible which dependencies will be cleaned up during the upgrade. diff --git a/packages/cli/src/commands/hydrogen/changelog-schema.test.ts b/packages/cli/src/commands/hydrogen/changelog-schema.test.ts index 6994429d0c..4301150ec0 100644 --- a/packages/cli/src/commands/hydrogen/changelog-schema.test.ts +++ b/packages/cli/src/commands/hydrogen/changelog-schema.test.ts @@ -2,7 +2,7 @@ import {readFile} from 'node:fs/promises'; import {join, dirname} from 'node:path'; import {fileURLToPath} from 'node:url'; import semver from 'semver'; -import {describe, expect, it} from 'vitest'; +import {beforeAll, describe, expect, it} from 'vitest'; import * as upgradeModule from './upgrade.js'; /** @@ -12,6 +12,11 @@ import * as upgradeModule from './upgrade.js'; * upgrade-flow.test.ts (~270 lines). These tests guard against malformed * changelog entries that could break the upgrade command at runtime. */ + +function assertDefined(value: T): asserts value is NonNullable { + expect(value).toBeDefined(); +} + describe('Changelog validation', () => { const allowedReleaseFields = new Set([ 'title', @@ -69,6 +74,12 @@ describe('Changelog validation', () => { 'changelog.json', ); + let changelog: Awaited>; + + beforeAll(async () => { + changelog = await upgradeModule.getChangelog(); + }); + it('is valid JSON and matches getChangelog() output', async () => { const changelogContent = await readFile(changelogPath, 'utf8'); @@ -81,12 +92,10 @@ describe('Changelog validation', () => { ); } - const changelog = await upgradeModule.getChangelog(); expect(changelog).toEqual(parsedChangelog); }); - it('has only allowed top-level fields', async () => { - const changelog = await upgradeModule.getChangelog(); + it('has only allowed top-level fields', () => { const allowedChangelogFields = ['url', 'version', 'releases']; const rogueChangelogFields = Object.keys(changelog).filter( (key) => !allowedChangelogFields.includes(key), @@ -94,13 +103,9 @@ describe('Changelog validation', () => { expect(rogueChangelogFields).toEqual([]); }); - it('has required fields and valid formats in every release', async () => { - const changelog = await upgradeModule.getChangelog(); - + it('has required fields and valid formats in every release', () => { for (const release of changelog.releases) { - // Fail fast on malformed entries rather than silently skipping them - expect(release).toBeDefined(); - if (!release) continue; + assertDefined(release); expect(release.title).toBeDefined(); expect(release.version).toBeDefined(); @@ -124,13 +129,9 @@ describe('Changelog validation', () => { } }); - it('has no rogue fields in any release', async () => { - const changelog = await upgradeModule.getChangelog(); - + it('has no rogue fields in any release', () => { for (const release of changelog.releases) { - // Fail fast on malformed entries rather than silently skipping them - expect(release).toBeDefined(); - if (!release) continue; + assertDefined(release); const rogueReleaseFields = Object.keys(release).filter( (key) => !allowedReleaseFields.has(key), @@ -139,9 +140,7 @@ describe('Changelog validation', () => { } }); - it('has valid feature/fix items with no rogue fields', async () => { - const changelog = await upgradeModule.getChangelog(); - + it('has valid feature/fix items with no rogue fields', () => { const allItems = changelog.releases.flatMap((r) => [ ...(r.features ?? []), ...(r.fixes ?? []), @@ -149,9 +148,7 @@ describe('Changelog validation', () => { expect(allItems.length).toBeGreaterThan(0); for (const release of changelog.releases) { - // Fail fast on malformed entries rather than silently skipping them - expect(release).toBeDefined(); - if (!release) continue; + assertDefined(release); for (const item of [ ...(release.features ?? []), @@ -172,9 +169,7 @@ describe('Changelog validation', () => { } }); - it('has valid steps with decodable base64 code', async () => { - const changelog = await upgradeModule.getChangelog(); - + it('has valid steps with decodable base64 code', () => { const allSteps = changelog.releases.flatMap((r) => [...(r.features ?? []), ...(r.fixes ?? [])].flatMap( (item) => item?.steps ?? [], @@ -183,9 +178,7 @@ describe('Changelog validation', () => { expect(allSteps.length).toBeGreaterThan(0); for (const release of changelog.releases) { - // Fail fast on malformed entries rather than silently skipping them - expect(release).toBeDefined(); - if (!release) continue; + assertDefined(release); for (const item of [ ...(release.features ?? []), @@ -213,9 +206,7 @@ describe('Changelog validation', () => { } }); - it('has valid semver versions for all dependencies', async () => { - const changelog = await upgradeModule.getChangelog(); - + it('has valid semver versions for all dependencies', () => { const allDeps = changelog.releases.flatMap((r) => [ ...Object.entries(r.dependencies ?? {}), ...Object.entries(r.devDependencies ?? {}), @@ -223,9 +214,7 @@ describe('Changelog validation', () => { expect(allDeps.length).toBeGreaterThan(0); for (const release of changelog.releases) { - // Fail fast on malformed entries rather than silently skipping them - expect(release).toBeDefined(); - if (!release) continue; + assertDefined(release); for (const [pkg, version] of Object.entries(release.dependencies ?? {})) { expect(typeof pkg).toBe('string'); @@ -243,9 +232,7 @@ describe('Changelog validation', () => { } }); - it('has valid dependenciesMeta structure', async () => { - const changelog = await upgradeModule.getChangelog(); - + it('has valid dependenciesMeta structure', () => { for (const release of changelog.releases) { if (!release?.dependenciesMeta) continue; @@ -261,13 +248,9 @@ describe('Changelog validation', () => { } }); - it('has valid removeDependencies and removeDevDependencies arrays', async () => { - const changelog = await upgradeModule.getChangelog(); - + it('has valid removeDependencies and removeDevDependencies arrays', () => { for (const release of changelog.releases) { - // Fail fast on malformed entries rather than silently skipping them - expect(release).toBeDefined(); - if (!release) continue; + assertDefined(release); if (release.removeDependencies) { expect(Array.isArray(release.removeDependencies)).toBe(true); diff --git a/packages/cli/src/commands/hydrogen/upgrade.test.ts b/packages/cli/src/commands/hydrogen/upgrade.test.ts index 467de5a49c..90b0834d4d 100644 --- a/packages/cli/src/commands/hydrogen/upgrade.test.ts +++ b/packages/cli/src/commands/hydrogen/upgrade.test.ts @@ -1268,6 +1268,47 @@ describe('upgrade', async () => { }, ); }); + + it('displays removed packages when release has only removals and no features or fixes', async () => { + await inTemporaryHydrogenRepo( + async () => { + const {releases} = await getChangelog(); + + const selectedRelease = releases.find( + (release) => release.version === '2023.10.0', + ) as (typeof releases)[0]; + + const removalsOnlyRelease: CumulativeRelease = { + features: [], + fixes: [], + removeDependencies: ['@remix-run/react'], + removeDevDependencies: [], + dependencies: {}, + devDependencies: {}, + }; + + await expect( + displayConfirmation({ + cumulativeRelease: removalsOnlyRelease, + selectedRelease, + }), + ).resolves.toEqual(false); + + expect(renderInfo).toHaveBeenCalledWith( + expect.objectContaining({ + headline: 'Included in this upgrade:', + customSections: expect.arrayContaining([ + expect.objectContaining({title: 'Removed packages'}), + ]), + }), + ); + }, + { + cleanGitRepo: true, + packageJson: OUTDATED_HYDROGEN_PACKAGE_JSON, + }, + ); + }); }); describe('generateUpgradeInstructionsFile', () => { @@ -1310,6 +1351,82 @@ describe('upgrade', async () => { }, ); }); + + it('omits Removed packages section when cumulativeRelease has no removals', async () => { + await inTemporaryHydrogenRepo( + async (appPath) => { + const {releases} = await getChangelog(); + + const selectedRelease = releases.find( + (release) => release.version === '2023.10.0', + ) as (typeof releases)[0]; + + // CUMULATIVE_RELEASE has features/fixes (so a file is generated) + // but empty removeDependencies and removeDevDependencies + const resultPath = await generateUpgradeInstructionsFile({ + appPath, + cumulativeRelease: CUMULATIVE_RELEASE, + currentVersion: '2023.1.6', + selectedRelease, + }); + + expect(resultPath).toBeDefined(); + + const mdContent = await readFile( + joinPath(appPath, resultPath!), + 'utf8', + ); + + expect(mdContent).not.toContain('## Removed packages'); + }, + { + cleanGitRepo: true, + packageJson: OUTDATED_HYDROGEN_PACKAGE_JSON, + }, + ); + }); + + it('generates instructions file when release has only removals and no features or fixes', async () => { + await inTemporaryHydrogenRepo( + async (appPath) => { + const {releases} = await getChangelog(); + + const selectedRelease = releases.find( + (release) => release.version === '2023.10.0', + ) as (typeof releases)[0]; + + const removalsOnlyRelease: CumulativeRelease = { + features: [], + fixes: [], + removeDependencies: ['@remix-run/react'], + removeDevDependencies: [], + dependencies: {}, + devDependencies: {}, + }; + + const resultPath = await generateUpgradeInstructionsFile({ + appPath, + cumulativeRelease: removalsOnlyRelease, + currentVersion: '2023.1.6', + selectedRelease, + }); + + expect(resultPath).toBeDefined(); + + const mdContent = await readFile( + joinPath(appPath, resultPath!), + 'utf8', + ); + + expect(mdContent).toContain('## Removed packages'); + expect(mdContent).toContain('- `@remix-run/react`'); + }, + { + cleanGitRepo: true, + packageJson: OUTDATED_HYDROGEN_PACKAGE_JSON, + }, + ); + }); }); describe('displayDevUpgradeNotice', () => { @@ -1547,12 +1664,14 @@ describe('upgrade', async () => { ); // The removal task should NOT be present when no deps to remove - const tasks = vi.mocked(renderTasks).mock.calls[0]?.[0] ?? []; - const removalTask = tasks.find( - (t: {title: string}) => - t.title === 'Removing deprecated dependencies', + expect(renderTasks).toHaveBeenCalledWith( + expect.not.arrayContaining([ + expect.objectContaining({ + title: 'Removing deprecated dependencies', + }), + ]), + expect.anything(), ); - expect(removalTask).toBeUndefined(); }, { cleanGitRepo: true, From 1198f9c9ec9f09def5d060d188c2821dbb04698f Mon Sep 17 00:00:00 2001 From: Riley Nowak Date: Wed, 15 Apr 2026 11:20:19 -0300 Subject: [PATCH 4/4] fix(cli): fix double-separator bug and strengthen schema test guards When `allRemovedPackages` is present but `fixes` is absent, the prior section (h1/breaking/features) already ends with `\n----\n`. The old code unconditionally prepended `\n----\n\n` before "## Removed packages", producing a `----\n\n----` double horizontal rule in the output. Fix: conditionally add the separator only when `fixesMd.length > 0` (fixes emits no trailing separator, so we supply one) vs. `'\n'` when fixes is absent (prior section already terminated with `----\n`). Also replace silent `if (!item) continue` / `if (!step) continue` guards in changelog-schema.test.ts with `assertDefined()` so null/ undefined entries in changelog.json cause an explicit test failure rather than a silent skip that masks malformed data. --- packages/cli/src/commands/hydrogen/changelog-schema.test.ts | 4 ++-- packages/cli/src/commands/hydrogen/upgrade.test.ts | 1 + packages/cli/src/commands/hydrogen/upgrade.ts | 3 ++- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/commands/hydrogen/changelog-schema.test.ts b/packages/cli/src/commands/hydrogen/changelog-schema.test.ts index 4301150ec0..c5dd622141 100644 --- a/packages/cli/src/commands/hydrogen/changelog-schema.test.ts +++ b/packages/cli/src/commands/hydrogen/changelog-schema.test.ts @@ -154,7 +154,7 @@ describe('Changelog validation', () => { ...(release.features ?? []), ...(release.fixes ?? []), ]) { - if (!item) continue; + assertDefined(item); const rogueItemFields = Object.keys(item).filter( (key) => !allowedItemFields.has(key), @@ -188,7 +188,7 @@ describe('Changelog validation', () => { expect(Array.isArray(item.steps)).toBe(true); for (const step of item.steps) { - if (!step) continue; + assertDefined(step); const rogueStepFields = Object.keys(step).filter( (key) => !allowedStepFields.has(key), diff --git a/packages/cli/src/commands/hydrogen/upgrade.test.ts b/packages/cli/src/commands/hydrogen/upgrade.test.ts index 90b0834d4d..c363874696 100644 --- a/packages/cli/src/commands/hydrogen/upgrade.test.ts +++ b/packages/cli/src/commands/hydrogen/upgrade.test.ts @@ -1420,6 +1420,7 @@ describe('upgrade', async () => { expect(mdContent).toContain('## Removed packages'); expect(mdContent).toContain('- `@remix-run/react`'); + expect(mdContent).not.toContain('----\n\n----'); }, { cleanGitRepo: true, diff --git a/packages/cli/src/commands/hydrogen/upgrade.ts b/packages/cli/src/commands/hydrogen/upgrade.ts index e1eac3806f..397e377744 100644 --- a/packages/cli/src/commands/hydrogen/upgrade.ts +++ b/packages/cli/src/commands/hydrogen/upgrade.ts @@ -1484,7 +1484,8 @@ export async function generateUpgradeInstructionsFile({ } if (allRemovedPackages.length) { - md += `\n----\n\n## Removed packages\n\nThe following packages have been removed as part of this upgrade:\n\n`; + md += fixesMd.length ? '\n\n----\n\n' : '\n'; + md += `## Removed packages\n\nThe following packages have been removed as part of this upgrade:\n\n`; md += allRemovedPackages.map((dep) => `- \`${dep}\``).join('\n'); md += '\n'; }