diff --git a/.changeset/removed-packages-ux.md b/.changeset/removed-packages-ux.md new file mode 100644 index 0000000000..d846f70b99 --- /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 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 new file mode 100644 index 0000000000..c5dd622141 --- /dev/null +++ b/packages/cli/src/commands/hydrogen/changelog-schema.test.ts @@ -0,0 +1,270 @@ +import {readFile} from 'node:fs/promises'; +import {join, dirname} from 'node:path'; +import {fileURLToPath} from 'node:url'; +import semver from 'semver'; +import {beforeAll, 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. + */ + +function assertDefined(value: T): asserts value is NonNullable { + expect(value).toBeDefined(); +} + +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', + ); + + let changelog: Awaited>; + + beforeAll(async () => { + changelog = await upgradeModule.getChangelog(); + }); + + 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}`, + ); + } + + expect(changelog).toEqual(parsedChangelog); + }); + + it('has only allowed top-level fields', () => { + 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', () => { + for (const release of changelog.releases) { + assertDefined(release); + + 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', () => { + for (const release of changelog.releases) { + assertDefined(release); + + const rogueReleaseFields = Object.keys(release).filter( + (key) => !allowedReleaseFields.has(key), + ); + expect(rogueReleaseFields).toEqual([]); + } + }); + + it('has valid feature/fix items with no rogue fields', () => { + const allItems = changelog.releases.flatMap((r) => [ + ...(r.features ?? []), + ...(r.fixes ?? []), + ]); + expect(allItems.length).toBeGreaterThan(0); + + for (const release of changelog.releases) { + assertDefined(release); + + for (const item of [ + ...(release.features ?? []), + ...(release.fixes ?? []), + ]) { + assertDefined(item); + + 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', () => { + 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) { + assertDefined(release); + + 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) { + assertDefined(step); + + 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', () => { + 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) { + assertDefined(release); + + 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', () => { + 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', () => { + for (const release of changelog.releases) { + assertDefined(release); + + 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..c363874696 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,199 @@ 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, + }, + ); + }); + + 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', () => { + 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, + }, + ); + }); + + 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`'); + expect(mdContent).not.toContain('----\n\n----'); + }, + { + cleanGitRepo: true, + packageJson: OUTDATED_HYDROGEN_PACKAGE_JSON, + }, + ); + }); }); describe('displayDevUpgradeNotice', () => { @@ -1447,7 +1654,25 @@ 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 + expect(renderTasks).toHaveBeenCalledWith( + expect.not.arrayContaining([ + expect.objectContaining({ + title: 'Removing deprecated dependencies', + }), + ]), + expect.anything(), + ); }, { cleanGitRepo: true, diff --git a/packages/cli/src/commands/hydrogen/upgrade.ts b/packages/cli/src/commands/hydrogen/upgrade.ts index 229a7046e8..397e377744 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,13 @@ async function generateUpgradeInstructionsFile({ )}`; } + if (allRemovedPackages.length) { + 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'; + } + const filePath = joinPath(instructionsFolderPath, filename); try {