From 36c7b078b9a8ca2e6f24f8f1af85f053befb2bc9 Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Tue, 11 Mar 2025 16:04:40 +0100 Subject: [PATCH 1/6] fix: include unchanged dependents in release validation --- src/release-specification.test.ts | 230 +++++++++++++++++++++++++++++- src/release-specification.ts | 11 +- 2 files changed, 228 insertions(+), 13 deletions(-) diff --git a/src/release-specification.test.ts b/src/release-specification.test.ts index 82742162..2677d1dc 100644 --- a/src/release-specification.test.ts +++ b/src/release-specification.test.ts @@ -602,7 +602,7 @@ Your release spec could not be processed due to the following issues: }); }); - it('throws if there are any packages in the release with a major version bump using the word "major", but any of their dependents defined as "peerDependencies" are not listed in the release', async () => { + it('throws if there are any packages in the release with a major version bump using the word "major", but any of their dependents defined as "peerDependencies" have changes since their latest release and are not listed in the release', async () => { await withSandbox(async (sandbox) => { const project = buildMockProject({ workspacePackages: { @@ -657,7 +657,62 @@ ${releaseSpecificationPath} }); }); - it('throws if there are any packages in the release with a major version bump using a literal version, but any of their dependents defined as "peerDependencies" are not listed in the release', async () => { + it('throws if there are any packages in the release with a major version bump using the word "major", but any of their dependents defined as "peerDependencies" have no changes since their latest release and are not listed in the release', async () => { + await withSandbox(async (sandbox) => { + const project = buildMockProject({ + workspacePackages: { + a: buildMockPackage('a', { + hasChangesSinceLatestRelease: true, + }), + b: buildMockPackage('b', { + hasChangesSinceLatestRelease: false, + validatedManifest: { + peerDependencies: { + a: '1.0.0', + }, + }, + }), + }, + }); + const releaseSpecificationPath = path.join( + sandbox.directoryPath, + 'release-spec', + ); + await fs.promises.writeFile( + releaseSpecificationPath, + YAML.stringify({ + packages: { + a: 'major', + }, + }), + ); + + await expect( + validateReleaseSpecification(project, releaseSpecificationPath), + ).rejects.toThrow( + ` +Your release spec could not be processed due to the following issues: + +* The following dependents of package 'a', which is being released with a major version bump, are missing from the release spec. + + - b + + Consider including them in the release spec so that they are compatible with the new 'a' version. + + If you are ABSOLUTELY SURE these packages are safe to omit, however, and want to postpone the release of a package, then list it with a directive of "intentionally-skip". For example: + + packages: + b: intentionally-skip + +The release spec file has been retained for you to edit again and make the necessary fixes. Once you've done this, re-run this tool. + +${releaseSpecificationPath} +`.trim(), + ); + }); + }); + + it('throws if there are any packages in the release with a major version bump using a literal version, but any of their dependents defined as "peerDependencies" have changes since their latest release and are not listed in the release', async () => { await withSandbox(async (sandbox) => { const project = buildMockProject({ workspacePackages: { @@ -712,7 +767,62 @@ ${releaseSpecificationPath} }); }); - it('throws if there are any packages in the release with a major version bump using the word "major", but their dependents via "peerDependencies" have their version specified as null in the release spec', async () => { + it('throws if there are any packages in the release with a major version bump using a literal version, but any of their dependents defined as "peerDependencies" have no changes since their latest release and are not listed in the release', async () => { + await withSandbox(async (sandbox) => { + const project = buildMockProject({ + workspacePackages: { + a: buildMockPackage('a', '2.1.4', { + hasChangesSinceLatestRelease: true, + }), + b: buildMockPackage('b', { + hasChangesSinceLatestRelease: true, + validatedManifest: { + peerDependencies: { + a: '2.1.4', + }, + }, + }), + }, + }); + const releaseSpecificationPath = path.join( + sandbox.directoryPath, + 'release-spec', + ); + await fs.promises.writeFile( + releaseSpecificationPath, + YAML.stringify({ + packages: { + a: '3.0.0', + }, + }), + ); + + await expect( + validateReleaseSpecification(project, releaseSpecificationPath), + ).rejects.toThrow( + ` +Your release spec could not be processed due to the following issues: + +* The following dependents of package 'a', which is being released with a major version bump, are missing from the release spec. + + - b + + Consider including them in the release spec so that they are compatible with the new 'a' version. + + If you are ABSOLUTELY SURE these packages are safe to omit, however, and want to postpone the release of a package, then list it with a directive of "intentionally-skip". For example: + + packages: + b: intentionally-skip + +The release spec file has been retained for you to edit again and make the necessary fixes. Once you've done this, re-run this tool. + +${releaseSpecificationPath} +`.trim(), + ); + }); + }); + + it('throws if there are any packages in the release with a major version bump using the word "major", but their dependents via "peerDependencies" have changes since their latest release and have their version specified as null in the release spec', async () => { await withSandbox(async (sandbox) => { const project = buildMockProject({ workspacePackages: { @@ -768,7 +878,63 @@ ${releaseSpecificationPath} }); }); - it('throws if there are any packages in the release with a major version bump using a literal version, but their dependents via "peerDependencies" have their version specified as null in the release spec', async () => { + it('throws if there are any packages in the release with a major version bump using the word "major", but their dependents via "peerDependencies" have no changes since their latest release and have their version specified as null in the release spec', async () => { + await withSandbox(async (sandbox) => { + const project = buildMockProject({ + workspacePackages: { + a: buildMockPackage('a', { + hasChangesSinceLatestRelease: true, + }), + b: buildMockPackage('b', { + hasChangesSinceLatestRelease: false, + validatedManifest: { + peerDependencies: { + a: '1.0.0', + }, + }, + }), + }, + }); + const releaseSpecificationPath = path.join( + sandbox.directoryPath, + 'release-spec', + ); + await fs.promises.writeFile( + releaseSpecificationPath, + YAML.stringify({ + packages: { + a: 'major', + b: null, + }, + }), + ); + + await expect( + validateReleaseSpecification(project, releaseSpecificationPath), + ).rejects.toThrow( + ` +Your release spec could not be processed due to the following issues: + +* The following dependents of package 'a', which is being released with a major version bump, are missing from the release spec. + + - b + + Consider including them in the release spec so that they are compatible with the new 'a' version. + + If you are ABSOLUTELY SURE these packages are safe to omit, however, and want to postpone the release of a package, then list it with a directive of "intentionally-skip". For example: + + packages: + b: intentionally-skip + +The release spec file has been retained for you to edit again and make the necessary fixes. Once you've done this, re-run this tool. + +${releaseSpecificationPath} +`.trim(), + ); + }); + }); + + it('throws if there are any packages in the release with a major version bump using a literal version, but their dependents via "peerDependencies" have changes since their latest release and have their version specified as null in the release spec', async () => { await withSandbox(async (sandbox) => { const project = buildMockProject({ workspacePackages: { @@ -818,6 +984,62 @@ Your release spec could not be processed due to the following issues: The release spec file has been retained for you to edit again and make the necessary fixes. Once you've done this, re-run this tool. +${releaseSpecificationPath} +`.trim(), + ); + }); + }); + + it('throws if there are any packages in the release with a major version bump using a literal version, but their dependents via "peerDependencies" have no changes since their latest release and have their version specified as null in the release spec', async () => { + await withSandbox(async (sandbox) => { + const project = buildMockProject({ + workspacePackages: { + a: buildMockPackage('a', '2.1.4', { + hasChangesSinceLatestRelease: true, + }), + b: buildMockPackage('b', { + hasChangesSinceLatestRelease: false, + validatedManifest: { + peerDependencies: { + a: '2.1.4', + }, + }, + }), + }, + }); + const releaseSpecificationPath = path.join( + sandbox.directoryPath, + 'release-spec', + ); + await fs.promises.writeFile( + releaseSpecificationPath, + YAML.stringify({ + packages: { + a: '3.0.0', + b: null, + }, + }), + ); + + await expect( + validateReleaseSpecification(project, releaseSpecificationPath), + ).rejects.toThrow( + ` +Your release spec could not be processed due to the following issues: + +* The following dependents of package 'a', which is being released with a major version bump, are missing from the release spec. + + - b + + Consider including them in the release spec so that they are compatible with the new 'a' version. + + If you are ABSOLUTELY SURE these packages are safe to omit, however, and want to postpone the release of a package, then list it with a directive of "intentionally-skip". For example: + + packages: + b: intentionally-skip + +The release spec file has been retained for you to edit again and make the necessary fixes. Once you've done this, re-run this tool. + ${releaseSpecificationPath} `.trim(), ); diff --git a/src/release-specification.ts b/src/release-specification.ts index 151a9ad8..aac0b855 100644 --- a/src/release-specification.ts +++ b/src/release-specification.ts @@ -168,7 +168,7 @@ export async function waitForUserToEditReleaseSpecification( } /** - * Finds all workspace packages that depend on the given package and have changes since their latest release. + * Finds all workspace packages that depend on the given package. * * @param project - The project containing workspace packages. * @param packageName - The name of the package to find dependents for. @@ -189,14 +189,7 @@ export function findMissingUnreleasedDependents( }, ); - const changedDependentNames = dependentNames.filter( - (possibleDependentName) => { - return project.workspacePackages[possibleDependentName] - .hasChangesSinceLatestRelease; - }, - ); - - return changedDependentNames.filter((dependentName) => { + return dependentNames.filter((dependentName) => { return !unvalidatedReleaseSpecificationPackages[dependentName]; }); } From 50306cc69786321a520f1a413802c70e450efb11 Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Thu, 13 Mar 2025 17:51:36 +0100 Subject: [PATCH 2/6] fix: apply peer dependency validation without changes to ui --- src/release-specification.ts | 27 +++++++++++++++++++++++---- src/ui.ts | 23 ++++++++++++++++++++--- src/ui/App.tsx | 31 ++++++++++++++++++++++++++++--- 3 files changed, 71 insertions(+), 10 deletions(-) diff --git a/src/release-specification.ts b/src/release-specification.ts index aac0b855..e551a2ef 100644 --- a/src/release-specification.ts +++ b/src/release-specification.ts @@ -172,13 +172,11 @@ export async function waitForUserToEditReleaseSpecification( * * @param project - The project containing workspace packages. * @param packageName - The name of the package to find dependents for. - * @param unvalidatedReleaseSpecificationPackages - The packages in the release specification. - * @returns An array of package names that depend on the given package and are missing from the release spec. + * @returns An array of package names that depend on the given package. */ -export function findMissingUnreleasedDependents( +export function findAllWorkspacePackagesThatDependOnPackage( project: Project, packageName: string, - unvalidatedReleaseSpecificationPackages: Record, ): string[] { const dependentNames = Object.keys(project.workspacePackages).filter( (possibleDependentName) => { @@ -189,6 +187,27 @@ export function findMissingUnreleasedDependents( }, ); + return dependentNames; +} + +/** + * Finds all workspace packages that depend on the given package. + * + * @param project - The project containing workspace packages. + * @param packageName - The name of the package to find dependents for. + * @param unvalidatedReleaseSpecificationPackages - The packages in the release specification. + * @returns An array of package names that depend on the given package and are missing from the release spec. + */ +export function findMissingUnreleasedDependents( + project: Project, + packageName: string, + unvalidatedReleaseSpecificationPackages: Record, +): string[] { + const dependentNames = findAllWorkspacePackagesThatDependOnPackage( + project, + packageName, + ); + return dependentNames.filter((dependentName) => { return !unvalidatedReleaseSpecificationPackages[dependentName]; }); diff --git a/src/ui.ts b/src/ui.ts index 46ff94e7..0757bf38 100644 --- a/src/ui.ts +++ b/src/ui.ts @@ -10,6 +10,7 @@ import { } from './project.js'; import { Package } from './package.js'; import { + findAllWorkspacePackagesThatDependOnPackage, findMissingUnreleasedDependenciesForRelease, findMissingUnreleasedDependentsForBreakingChanges, IncrementableVersionParts, @@ -130,15 +131,31 @@ function createApp({ app.use(express.static(UI_BUILD_DIR)); app.use(express.json()); - app.get('/api/packages', (_req, res) => { + app.get('/api/packages', (req, res) => { + const { majorBumps } = req.query; + + const majorBumpsArray = + typeof majorBumps === 'string' + ? majorBumps.split(',').filter(Boolean) + : (req.query.majorBumps as string[] | undefined) || []; + + const requiredDependents = [ + ...new Set( + majorBumpsArray.flatMap((majorBump) => + findAllWorkspacePackagesThatDependOnPackage(project, majorBump), + ), + ), + ]; + const pkgs = Object.values(project.workspacePackages).filter( - (pkg) => pkg.hasChangesSinceLatestRelease, + (pkg) => + pkg.hasChangesSinceLatestRelease || + requiredDependents.includes(pkg.validatedManifest.name), ); const packages = pkgs.map((pkg) => ({ name: pkg.validatedManifest.name, version: pkg.validatedManifest.version.version, - location: pkg.directoryPath, })); res.json(packages); diff --git a/src/ui/App.tsx b/src/ui/App.tsx index 2282a4b6..f2b2d2ce 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -1,11 +1,17 @@ import './style.css'; -import React, { useState, useEffect } from 'react'; +import React, { useState, useEffect, useRef } from 'react'; import { createRoot } from 'react-dom/client'; import { SemVer } from 'semver'; import { ErrorMessage } from './ErrorMessage.js'; import { PackageItem } from './PackageItem.js'; import { Package, RELEASE_TYPE_OPTIONS, ReleaseType } from './types.js'; +// Helper function to compare sets +const setsAreEqual = (a: Set, b: Set) => { + if (a.size !== b.size) return false; + return [...a].every((value) => b.has(value)); +}; + type SubmitButtonProps = { selections: Record; packageDependencyErrors: Record< @@ -67,9 +73,14 @@ function App() { new Set(), ); const [showCheckboxes, setShowCheckboxes] = useState(false); + const previousPackages = useRef>(new Set()); useEffect(() => { - fetch('/api/packages') + const majorBumps = Object.entries(selections) + .filter(([_, type]) => type === 'major') + .map(([pkgName]) => pkgName); + + fetch(`/api/packages?majorBumps=${majorBumps.join(',')}`) .then((res) => { if (!res.ok) { throw new Error(`Received ${res.status}`); @@ -77,6 +88,20 @@ function App() { return res.json(); }) .then((data: Package[]) => { + const newPackageNames = new Set(data.map((pkg) => pkg.name)); + + // Only clean up selections if the package list actually changed + if (!setsAreEqual(previousPackages.current, newPackageNames)) { + setSelections((prev) => + Object.fromEntries( + Object.entries(prev).filter(([pkgName]) => + newPackageNames.has(pkgName), + ), + ), + ); + previousPackages.current = newPackageNames; + } + setPackages(data); setLoadingChangelogs( data.reduce((acc, pkg) => ({ ...acc, [pkg.name]: false }), {}), @@ -86,7 +111,7 @@ function App() { setError(err.message); console.error('Error fetching packages:', err); }); - }, []); + }, [selections]); const checkDependencies = async (selectionData: Record) => { if (Object.keys(selectionData).length === 0) return; From 7bc1221bb2c7e045901b8c4335e3f28e45caedce Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Thu, 13 Mar 2025 17:53:28 +0100 Subject: [PATCH 3/6] fix: use set for major bumps check --- src/ui.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/ui.ts b/src/ui.ts index 0757bf38..a646f9a7 100644 --- a/src/ui.ts +++ b/src/ui.ts @@ -139,18 +139,16 @@ function createApp({ ? majorBumps.split(',').filter(Boolean) : (req.query.majorBumps as string[] | undefined) || []; - const requiredDependents = [ - ...new Set( - majorBumpsArray.flatMap((majorBump) => - findAllWorkspacePackagesThatDependOnPackage(project, majorBump), - ), + const requiredDependents = new Set( + majorBumpsArray.flatMap((majorBump) => + findAllWorkspacePackagesThatDependOnPackage(project, majorBump), ), - ]; + ); const pkgs = Object.values(project.workspacePackages).filter( (pkg) => pkg.hasChangesSinceLatestRelease || - requiredDependents.includes(pkg.validatedManifest.name), + requiredDependents.has(pkg.validatedManifest.name), ); const packages = pkgs.map((pkg) => ({ From 7c90c8339e14424322ed33eea4c0c2fa3ca7099a Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Thu, 13 Mar 2025 21:35:05 +0100 Subject: [PATCH 4/6] Address PR feedback --- src/release-specification.test.ts | 8 ++--- src/ui/App.tsx | 54 +++++++++++++++++-------------- src/ui/PackageItem.tsx | 33 +++++++++++-------- 3 files changed, 52 insertions(+), 43 deletions(-) diff --git a/src/release-specification.test.ts b/src/release-specification.test.ts index 2677d1dc..9c4b2c6e 100644 --- a/src/release-specification.test.ts +++ b/src/release-specification.test.ts @@ -657,7 +657,7 @@ ${releaseSpecificationPath} }); }); - it('throws if there are any packages in the release with a major version bump using the word "major", but any of their dependents defined as "peerDependencies" have no changes since their latest release and are not listed in the release', async () => { + it('throws if there are any packages in the release with a major version bump using the word "major", but any of their dependents defined as "peerDependencies" are not listed in the release, even if they have no changes', async () => { await withSandbox(async (sandbox) => { const project = buildMockProject({ workspacePackages: { @@ -767,7 +767,7 @@ ${releaseSpecificationPath} }); }); - it('throws if there are any packages in the release with a major version bump using a literal version, but any of their dependents defined as "peerDependencies" have no changes since their latest release and are not listed in the release', async () => { + it('throws if there are any packages in the release with a major version bump using a literal version, but any of their dependents defined as "peerDependencies" are not listed in the release, even if they have no changes', async () => { await withSandbox(async (sandbox) => { const project = buildMockProject({ workspacePackages: { @@ -878,7 +878,7 @@ ${releaseSpecificationPath} }); }); - it('throws if there are any packages in the release with a major version bump using the word "major", but their dependents via "peerDependencies" have no changes since their latest release and have their version specified as null in the release spec', async () => { + it('throws if there are any packages in the release with a major version bump using the word "major", but their dependents via "peerDependencies" have their version specified as null in the release spec, even if they have no changes', async () => { await withSandbox(async (sandbox) => { const project = buildMockProject({ workspacePackages: { @@ -990,7 +990,7 @@ ${releaseSpecificationPath} }); }); - it('throws if there are any packages in the release with a major version bump using a literal version, but their dependents via "peerDependencies" have no changes since their latest release and have their version specified as null in the release spec', async () => { + it('throws if there are any packages in the release with a major version bump using a literal version, but their dependents via "peerDependencies" have their version specified as null in the release spec, even if they have no changes', async () => { await withSandbox(async (sandbox) => { const project = buildMockProject({ workspacePackages: { diff --git a/src/ui/App.tsx b/src/ui/App.tsx index f2b2d2ce..e8f0826b 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -13,7 +13,7 @@ const setsAreEqual = (a: Set, b: Set) => { }; type SubmitButtonProps = { - selections: Record; + releaseSelections: Record; packageDependencyErrors: Record< string, { missingDependentNames: string[]; missingDependencies: string[] } @@ -22,14 +22,16 @@ type SubmitButtonProps = { }; function SubmitButton({ - selections, + releaseSelections, packageDependencyErrors, onSubmit, }: SubmitButtonProps) { const isDisabled = - Object.keys(selections).length === 0 || + Object.keys(releaseSelections).length === 0 || Object.keys(packageDependencyErrors).length > 0 || - Object.values(selections).every((value) => value === 'intentionally-skip'); + Object.values(releaseSelections).every( + (value) => value === 'intentionally-skip', + ); return (