From 356aaa8d5694476cb5d90a4fd6f62aa57cf7c431 Mon Sep 17 00:00:00 2001 From: Elizabeth Craig Date: Tue, 10 Mar 2026 16:27:48 -0700 Subject: [PATCH 1/3] afterall cleanup --- src/__fixtures__/mockNpm.test.ts | 14 +------------- src/__functional__/options/getCliOptions.test.ts | 6 +----- src/__functional__/options/getOptions.test.ts | 2 -- .../packageManager/getNpmPackageInfo.test.ts | 6 +----- src/__tests__/bump/setDependentVersions.test.ts | 6 +----- src/__tests__/bump/updateLockFile.test.ts | 6 +----- src/__tests__/bump/updatePackageJsons.test.ts | 6 +----- src/__tests__/publish/tagPackages.test.ts | 6 +----- .../validate/isValidChangelogOptions.test.ts | 6 +----- src/__tests__/validate/isValidGroupOptions.test.ts | 10 +--------- 10 files changed, 9 insertions(+), 59 deletions(-) diff --git a/src/__fixtures__/mockNpm.test.ts b/src/__fixtures__/mockNpm.test.ts index 896602b77..faf23996f 100644 --- a/src/__fixtures__/mockNpm.test.ts +++ b/src/__fixtures__/mockNpm.test.ts @@ -2,7 +2,7 @@ // But this added complexity greatly speeds up the other npm-related tests by removing the // dependency on actual npm CLI calls and a fake registry (which are very slow). -import { afterAll, afterEach, beforeAll, describe, expect, it, jest } from '@jest/globals'; +import { afterEach, beforeAll, describe, expect, it, jest } from '@jest/globals'; import fs from 'fs'; // import fetch from 'npm-registry-fetch'; import { type NpmResult, npm } from '../packageManager/npm'; @@ -208,10 +208,6 @@ describe('_mockNpmPublish', () => { packageJson = undefined; }); - afterAll(() => { - jest.restoreAllMocks(); - }); - it('throws if cwd is not specified', async () => { await expect(() => _mockNpmPublish({}, [], { cwd: undefined })).rejects.toThrow( 'cwd is required for mock npm publish' @@ -311,10 +307,6 @@ describe('_mockNpmPack', () => { writtenFiles = []; }); - afterAll(() => { - jest.restoreAllMocks(); - }); - it('throws if cwd is not specified', async () => { await expect(() => _mockNpmPack({}, [], { cwd: undefined })).rejects.toThrow('cwd is required for mock npm pack'); }); @@ -370,10 +362,6 @@ describe('mockNpm', () => { packageJson = undefined; }); - afterAll(() => { - jest.restoreAllMocks(); - }); - // describe('mockFetchJson', () => { // it('mocks registry fetch', async () => { // npmMock.setRegistryData({ foo: { versions: ['1.0.0'] } }); diff --git a/src/__functional__/options/getCliOptions.test.ts b/src/__functional__/options/getCliOptions.test.ts index 99aeceda0..509d3b702 100644 --- a/src/__functional__/options/getCliOptions.test.ts +++ b/src/__functional__/options/getCliOptions.test.ts @@ -1,4 +1,4 @@ -import { afterAll, afterEach, describe, expect, it, jest } from '@jest/globals'; +import { afterEach, describe, expect, it, jest } from '@jest/globals'; import { getCliOptions } from '../../options/getCliOptions'; import { findProjectRoot, getDefaultRemoteBranch } from 'workspace-tools'; @@ -34,10 +34,6 @@ describe('getCliOptions', () => { jest.clearAllMocks(); }); - afterAll(() => { - jest.restoreAllMocks(); - }); - // start by making sure nothing went wrong with the mock it('uses fake project root', () => { expect(findProjectRoot(process.cwd())).toEqual(projectRoot); diff --git a/src/__functional__/options/getOptions.test.ts b/src/__functional__/options/getOptions.test.ts index 1118fa568..483e53524 100644 --- a/src/__functional__/options/getOptions.test.ts +++ b/src/__functional__/options/getOptions.test.ts @@ -30,7 +30,6 @@ describe('getOptions (deprecated)', () => { afterAll(() => { repositoryFactory.cleanUp(); - jest.restoreAllMocks(); }); it('uses the branch name defined in beachball.config.js', () => { @@ -115,7 +114,6 @@ describe('getParsedOptions', () => { afterAll(() => { repositoryFactory.cleanUp(); - jest.restoreAllMocks(); }); it('uses the branch name defined in beachball.config.js', () => { diff --git a/src/__functional__/packageManager/getNpmPackageInfo.test.ts b/src/__functional__/packageManager/getNpmPackageInfo.test.ts index 38f1ea818..1d153c43c 100644 --- a/src/__functional__/packageManager/getNpmPackageInfo.test.ts +++ b/src/__functional__/packageManager/getNpmPackageInfo.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, afterAll, beforeEach, jest } from '@jest/globals'; +import { describe, expect, it, beforeEach, jest } from '@jest/globals'; // import fetch from 'npm-registry-fetch'; import { _npmShowProperties, @@ -24,10 +24,6 @@ describe('getNpmPackageInfo', () => { // fetchJsonSpy.mockClear(); }); - afterAll(() => { - jest.restoreAllMocks(); - }); - it.each<{ desc: string; name: string; knownVersion: string }>([ { desc: 'unscoped', name: 'beachball', knownVersion: '2.60.1' }, { desc: 'scoped', name: '@lage-run/cli', knownVersion: '0.33.0' }, diff --git a/src/__tests__/bump/setDependentVersions.test.ts b/src/__tests__/bump/setDependentVersions.test.ts index 4b2bcff57..a14238ab8 100644 --- a/src/__tests__/bump/setDependentVersions.test.ts +++ b/src/__tests__/bump/setDependentVersions.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, afterEach, jest, beforeAll, afterAll } from '@jest/globals'; +import { describe, it, expect, afterEach, jest, beforeAll } from '@jest/globals'; import { setDependentVersions } from '../../bump/setDependentVersions'; import { makePackageInfos, type PartialPackageInfos } from '../../__fixtures__/packageInfos'; import { consideredDependencies } from '../../types/PackageInfo'; @@ -35,10 +35,6 @@ describe('setDependentVersions', () => { jest.clearAllMocks(); }); - afterAll(() => { - jest.restoreAllMocks(); - }); - it('returns empty object when no packages are in scope', () => { const bumpInfo = makeBumpInfo({ packageInfos: { 'pkg-a': {} }, diff --git a/src/__tests__/bump/updateLockFile.test.ts b/src/__tests__/bump/updateLockFile.test.ts index 31793a244..5c543f5b9 100644 --- a/src/__tests__/bump/updateLockFile.test.ts +++ b/src/__tests__/bump/updateLockFile.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, jest, afterAll, afterEach, beforeAll, beforeEach } from '@jest/globals'; +import { describe, it, expect, jest, afterEach, beforeAll, beforeEach } from '@jest/globals'; import fs from 'fs'; import path from 'path'; import { updateLockFile } from '../../bump/updateLockFile'; @@ -35,10 +35,6 @@ describe('updateLockFile', () => { jest.clearAllMocks(); }); - afterAll(() => { - jest.restoreAllMocks(); - }); - it('updates package-lock.json when it exists', async () => { mockFs.existsSync.mockImplementation(p => p === path.join(mockRoot, 'package-lock.json')); diff --git a/src/__tests__/bump/updatePackageJsons.test.ts b/src/__tests__/bump/updatePackageJsons.test.ts index d985e22a3..5c1f101b7 100644 --- a/src/__tests__/bump/updatePackageJsons.test.ts +++ b/src/__tests__/bump/updatePackageJsons.test.ts @@ -1,4 +1,4 @@ -import { jest, describe, it, expect, beforeEach, afterEach, beforeAll, afterAll } from '@jest/globals'; +import { jest, describe, it, expect, beforeEach, afterEach, beforeAll } from '@jest/globals'; import fs from 'fs'; import { updatePackageJsons } from '../../bump/updatePackageJsons'; import { makePackageInfos } from '../../__fixtures__/packageInfos'; @@ -41,10 +41,6 @@ describe('updatePackageJsons', () => { jest.resetAllMocks(); }); - afterAll(() => { - jest.restoreAllMocks(); - }); - it('updates version for non-private packages', () => { const modifiedPackages = new Set(['pkg-a']); const packageInfos = makePackageInfos({ diff --git a/src/__tests__/publish/tagPackages.test.ts b/src/__tests__/publish/tagPackages.test.ts index c24d433f4..88689e266 100644 --- a/src/__tests__/publish/tagPackages.test.ts +++ b/src/__tests__/publish/tagPackages.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, jest, beforeEach, afterAll } from '@jest/globals'; +import { describe, expect, it, jest, beforeEach } from '@jest/globals'; import { gitFailFast } from 'workspace-tools'; import { initMockLogs } from '../../__fixtures__/mockLogs'; import { tagPackages } from '../../publish/tagPackages'; @@ -63,10 +63,6 @@ describe('tagPackages', () => { (gitFailFast as jest.Mock).mockReset(); }); - afterAll(() => { - jest.restoreAllMocks(); - }); - it('does not create package tag for packages with gitTags=false', () => { // Also verifies that if `gitTags` is false overall, it doesn't create a git tag for the dist tag (`tag`) tagPackages(noTagBumpInfo, { path: '', gitTags: false, tag: '' }); diff --git a/src/__tests__/validate/isValidChangelogOptions.test.ts b/src/__tests__/validate/isValidChangelogOptions.test.ts index 2cb209781..ccdadb41d 100644 --- a/src/__tests__/validate/isValidChangelogOptions.test.ts +++ b/src/__tests__/validate/isValidChangelogOptions.test.ts @@ -1,4 +1,4 @@ -import { afterAll, afterEach, beforeAll, describe, expect, it, jest } from '@jest/globals'; +import { afterEach, beforeAll, describe, expect, it, jest } from '@jest/globals'; import { isValidChangelogOptions } from '../../validation/isValidChangelogOptions'; import type { ChangelogGroupOptions, ChangelogOptions } from '../../types/ChangelogOptions'; @@ -13,10 +13,6 @@ describe('isValidChangelogOptions', () => { jest.clearAllMocks(); }); - afterAll(() => { - jest.restoreAllMocks(); - }); - it('returns true when options have no groups', () => { const options: ChangelogOptions = {}; expect(isValidChangelogOptions(options)).toBe(true); diff --git a/src/__tests__/validate/isValidGroupOptions.test.ts b/src/__tests__/validate/isValidGroupOptions.test.ts index 1ffd197cc..3b3a4bcb2 100644 --- a/src/__tests__/validate/isValidGroupOptions.test.ts +++ b/src/__tests__/validate/isValidGroupOptions.test.ts @@ -1,4 +1,4 @@ -import { afterAll, afterEach, beforeAll, describe, expect, it, jest } from '@jest/globals'; +import { afterEach, beforeAll, describe, expect, it, jest } from '@jest/globals'; import { isValidGroupOptions, isValidGroupedPackageOptions } from '../../validation/isValidGroupOptions'; import type { VersionGroupOptions } from '../../types/BeachballOptions'; import type { PackageGroups } from '../../types/PackageInfo'; @@ -15,10 +15,6 @@ describe('isValidGroupOptions', () => { jest.clearAllMocks(); }); - afterAll(() => { - jest.restoreAllMocks(); - }); - it('returns true for valid groups', () => { const groups: VersionGroupOptions[] = [ { name: 'group1', include: ['pkg1', 'pkg2'], disallowedChangeTypes: null }, @@ -83,10 +79,6 @@ describe('isValidGroupedPackageOptions', () => { jest.clearAllMocks(); }); - afterAll(() => { - jest.restoreAllMocks(); - }); - it('returns true when no packages have disallowedChangeTypes', () => { const packageInfos = makePackageInfos({ pkg1: {}, pkg2: {} }); const packageGroups: PackageGroups = { From 2298e3060cbaeabe05ee7eaa1c79bbe3ce017530 Mon Sep 17 00:00:00 2001 From: Elizabeth Craig Date: Tue, 10 Mar 2026 16:34:53 -0700 Subject: [PATCH 2/3] Centralize error handling --- scripts/jestSetup.js | 4 ++++ src/__e2e__/validate.test.ts | 7 ++----- src/__fixtures__/mockProcessExit.ts | 11 ---------- .../publish/publishToRegistry.test.ts | 3 +-- src/__tests__/bump/bumpInMemory.test.ts | 14 ++----------- .../monorepo/getPackageGroups.test.ts | 14 +++---------- src/cli.ts | 21 ++++++++++++------- src/commands/init.ts | 13 ++++++------ src/monorepo/getPackageGroups.ts | 5 ++--- src/options/getRepoOptions.ts | 6 ++---- src/publish/bumpAndPush.ts | 7 ++++--- src/publish/publishToRegistry.ts | 5 ++--- src/types/BeachballError.ts | 17 +++++++++++++++ src/validation/validate.ts | 19 ++++++----------- 14 files changed, 65 insertions(+), 81 deletions(-) delete mode 100644 src/__fixtures__/mockProcessExit.ts create mode 100644 src/types/BeachballError.ts diff --git a/scripts/jestSetup.js b/scripts/jestSetup.js index 6f40fb490..68d862374 100644 --- a/scripts/jestSetup.js +++ b/scripts/jestSetup.js @@ -1,6 +1,10 @@ // @ts-check const { jest, afterAll } = require('@jest/globals'); +// Safety net: production code should never call process.exit() directly +// (errors should be thrown and caught at the top level in cli.ts). +// This mock ensures any accidental process.exit() call in tests immediately +// throws rather than silently exiting the test runner. jest.spyOn(process, 'exit').mockImplementation(code => { throw new Error(`process.exit called with code ${code}`); }); diff --git a/src/__e2e__/validate.test.ts b/src/__e2e__/validate.test.ts index bc2371935..c5be8103f 100644 --- a/src/__e2e__/validate.test.ts +++ b/src/__e2e__/validate.test.ts @@ -5,13 +5,12 @@ import { initMockLogs } from '../__fixtures__/mockLogs'; import { validate, type ValidateOptions } from '../validation/validate'; import type { Repository } from '../__fixtures__/repository'; import { getParsedOptions } from '../options/getOptions'; -import { mockProcessExit } from '../__fixtures__/mockProcessExit'; +import { BeachballError } from '../types/BeachballError'; describe('validate', () => { let repositoryFactory: RepositoryFactory; let repo: Repository | undefined; const logs = initMockLogs(); - const processExit = mockProcessExit(logs); function validateWrapper(validateOptions?: ValidateOptions) { const parsedOptions = getParsedOptions({ @@ -30,7 +29,6 @@ describe('validate', () => { }); afterEach(() => { - processExit.mockClear(); repo = undefined; }); @@ -54,8 +52,7 @@ describe('validate', () => { repo.checkout('-b', 'test'); repo.stageChange('packages/foo/test.js'); - expect(() => validateWrapper({ checkChangeNeeded: true })).toThrowError(/process\.exit/); - expect(processExit).toHaveBeenCalledWith(1); + expect(() => validateWrapper({ checkChangeNeeded: true })).toThrow(BeachballError); expect(logs.mocks.error).toHaveBeenCalledWith('ERROR: Change files are needed!'); }); diff --git a/src/__fixtures__/mockProcessExit.ts b/src/__fixtures__/mockProcessExit.ts deleted file mode 100644 index 622a84fe2..000000000 --- a/src/__fixtures__/mockProcessExit.ts +++ /dev/null @@ -1,11 +0,0 @@ -import { jest } from '@jest/globals'; -import type { MockLogs } from './mockLogs'; - -/** - * Throw an error when `process.exit()` is called. The message will include any error logs. - */ -export function mockProcessExit(logs: MockLogs): jest.SpiedFunction { - return jest.spyOn(process, 'exit').mockImplementation(code => { - throw new Error(`process.exit(${code ?? ''}) called. Logged errors:\n${logs.getMockLines('error')}`); - }); -} diff --git a/src/__functional__/publish/publishToRegistry.test.ts b/src/__functional__/publish/publishToRegistry.test.ts index 39a233fa5..2e0d15eec 100644 --- a/src/__functional__/publish/publishToRegistry.test.ts +++ b/src/__functional__/publish/publishToRegistry.test.ts @@ -158,8 +158,7 @@ describe('publishToRegistry', () => { // Pre-populate registry so version 1.0.0 already exists npmMock.setRegistryData({ foo: { versions: ['1.0.0'] } }); - // process.exit is mocked in jestSetup.js to throw - await expect(publishToRegistry(bumpInfo, defaultOptions)).rejects.toThrow('process.exit called with code 1'); + await expect(publishToRegistry(bumpInfo, defaultOptions)).rejects.toThrow('Pre-publish validation failed'); expect(logs.getMockLines('all')).toMatchInlineSnapshot(` "[log] Validating new package versions... diff --git a/src/__tests__/bump/bumpInMemory.test.ts b/src/__tests__/bump/bumpInMemory.test.ts index 296923caf..8637c7880 100644 --- a/src/__tests__/bump/bumpInMemory.test.ts +++ b/src/__tests__/bump/bumpInMemory.test.ts @@ -1,8 +1,7 @@ -import { afterAll, beforeAll, describe, expect, it, jest } from '@jest/globals'; +import { describe, expect, it } from '@jest/globals'; import path from 'path'; import { generateChanges, type PartialChangeFile } from '../../__fixtures__/changeFiles'; import { initMockLogs } from '../../__fixtures__/mockLogs'; -import { mockProcessExit } from '../../__fixtures__/mockProcessExit'; import { makePackageInfosByFolder, type PartialPackageInfo } from '../../__fixtures__/packageInfos'; import { bumpInMemory } from '../../bump/bumpInMemory'; import { getParsedOptions } from '../../options/getOptions'; @@ -12,7 +11,7 @@ import { getScopedPackages } from '../../monorepo/getScopedPackages'; import { getPackageGroups } from '../../monorepo/getPackageGroups'; describe('bumpInMemory', () => { - const logs = initMockLogs(); + initMockLogs(); const cwd = path.resolve('/fake-root'); function gatherBumpInfoWrapper(params: { @@ -42,15 +41,6 @@ describe('bumpInMemory', () => { return { bumpInfo, options, originalPackageInfos }; } - beforeAll(() => { - // getPackageGroups currently can call process.exit - mockProcessExit(logs); - }); - - afterAll(() => { - jest.restoreAllMocks(); - }); - it('bumps only packages with change files with bumpDeps: false', () => { const { bumpInfo, originalPackageInfos } = gatherBumpInfoWrapper({ packageFolders: { diff --git a/src/__tests__/monorepo/getPackageGroups.test.ts b/src/__tests__/monorepo/getPackageGroups.test.ts index 839f8b777..0f4ee3cc9 100644 --- a/src/__tests__/monorepo/getPackageGroups.test.ts +++ b/src/__tests__/monorepo/getPackageGroups.test.ts @@ -1,10 +1,10 @@ -import { afterAll, beforeAll, describe, expect, it, jest } from '@jest/globals'; +import { describe, expect, it } from '@jest/globals'; import path from 'path'; import { initMockLogs } from '../../__fixtures__/mockLogs'; -import { mockProcessExit } from '../../__fixtures__/mockProcessExit'; import { makePackageInfosByFolder, type PartialPackageInfos } from '../../__fixtures__/packageInfos'; import { getPackageGroups } from '../../monorepo/getPackageGroups'; import type { RepoOptions } from '../../types/BeachballOptions'; +import { BeachballError } from '../../types/BeachballError'; describe('getPackageGroups', () => { const root = path.resolve('/fake-root'); @@ -21,14 +21,6 @@ describe('getPackageGroups', () => { return getPackageGroups(packageInfos, root, params.repoOptions?.groups); } - beforeAll(() => { - mockProcessExit(logs); - }); - - afterAll(() => { - jest.restoreAllMocks(); - }); - it('returns empty object if no groups are defined', () => { const groups = getPackageGroupsWrapper({ packageFolders: { 'packages/foo': {}, 'packages/bar': {} }, @@ -173,7 +165,7 @@ describe('getPackageGroups', () => { ], }, }) - ).toThrow('process.exit(1) called'); + ).toThrow(BeachballError); expect(logs.getMockLines('error')).toMatchInlineSnapshot(` "ERROR: Found package(s) belonging to multiple groups: diff --git a/src/cli.ts b/src/cli.ts index 8800edd5e..fee996f97 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -11,15 +11,14 @@ import { getPackageInfos } from './monorepo/getPackageInfos'; import { getParsedOptions } from './options/getOptions'; import { validate } from './validation/validate'; import { getScopedPackages } from './monorepo/getScopedPackages'; +import { BeachballError } from './types/BeachballError'; (async () => { try { // eslint-disable-next-line no-restricted-properties -- top-level call findGitRoot(process.cwd()); } catch { - console.error('beachball only works in a git repository. Please initialize git and try again.'); - // eslint-disable-next-line no-restricted-properties - process.exit(1); + throw new BeachballError('beachball only works in a git repository. Please initialize git and try again.'); } // eslint-disable-next-line no-restricted-properties -- this is the top level @@ -98,10 +97,18 @@ import { getScopedPackages } from './monorepo/getScopedPackages'; throw new Error('Invalid command: ' + options.command); } })().catch(e => { - showVersion(); - console.error('An error has been detected while running beachball!'); - console.error((e as Error)?.stack || e); + if (e instanceof BeachballError && e.alreadyLogged) { + // Error details were already printed -- just exit + } else if (e instanceof BeachballError) { + // Expected error, not yet logged -- print the message (no stack trace) + console.error(e.message); + } else { + // Unexpected error -- print full details including stack + showVersion(); + console.error('Unexpected error while running beachball!'); + console.error((e as Error)?.stack || e); + } - // eslint-disable-next-line no-restricted-properties -- this is the top level + // eslint-disable-next-line no-restricted-properties -- this is the only place that should call process.exit process.exit(1); }); diff --git a/src/commands/init.ts b/src/commands/init.ts index 93056afae..4a0a8e385 100644 --- a/src/commands/init.ts +++ b/src/commands/init.ts @@ -4,27 +4,26 @@ import * as path from 'path'; import type { PackageJson } from '../types/PackageInfo'; import { readJson } from '../object/readJson'; import { getNpmPackageInfo } from '../packageManager/getNpmPackageInfo'; +import { BeachballError } from '../types/BeachballError'; -// TODO: consider modifying this to propagate up -function errorExit(message: string): never { +function throwInitError(message: string): never { console.error(message); console.log( 'You can still set up beachball manually by following the instructions here: https://microsoft.github.io/beachball/overview/getting-started.html' ); - // eslint-disable-next-line no-restricted-properties - process.exit(1); + throw new BeachballError(message, { alreadyLogged: true }); } export async function init(options: Pick): Promise { const packageJsonFilePath = path.join(options.path, 'package.json'); if (!fs.existsSync(packageJsonFilePath)) { - errorExit(`Cannot find package.json at ${packageJsonFilePath}`); + throwInitError(`Cannot find package.json at ${packageJsonFilePath}`); } const beachballInfo = await getNpmPackageInfo('beachball', options); if (!beachballInfo) { - errorExit('Failed to retrieve beachball version from npm'); + throwInitError('Failed to retrieve beachball version from npm'); } const beachballVersion = beachballInfo['dist-tags'].latest; @@ -32,7 +31,7 @@ export async function init(options: Pick) try { packageJson = readJson(packageJsonFilePath); } catch { - errorExit(`Failed to read package.json at ${packageJsonFilePath}`); + throwInitError(`Failed to read package.json at ${packageJsonFilePath}`); } packageJson.devDependencies ??= {}; diff --git a/src/monorepo/getPackageGroups.ts b/src/monorepo/getPackageGroups.ts index 041d0cddd..f146e35ed 100644 --- a/src/monorepo/getPackageGroups.ts +++ b/src/monorepo/getPackageGroups.ts @@ -3,6 +3,7 @@ import path from 'path'; import type { PackageInfos, PackageGroups } from '../types/PackageInfo'; import { isPathIncluded } from './isPathIncluded'; import { bulletedList } from '../logging/bulletedList'; +import { BeachballError } from '../types/BeachballError'; export function getPackageGroups( packageInfos: PackageInfos, @@ -48,9 +49,7 @@ export function getPackageGroups( errorEntries.map(([pkgName, pkgGroups]) => `${pkgName}: ${pkgGroups.map(g => g.name).join(', ')}`).sort() ) ); - // TODO: probably more appropriate to throw here and let the caller handle it? - // eslint-disable-next-line no-restricted-properties - process.exit(1); + throw new BeachballError('Found package(s) belonging to multiple groups', { alreadyLogged: true }); } return packageGroups; diff --git a/src/options/getRepoOptions.ts b/src/options/getRepoOptions.ts index bee3c2a8f..d5451a626 100644 --- a/src/options/getRepoOptions.ts +++ b/src/options/getRepoOptions.ts @@ -2,6 +2,7 @@ import { cosmiconfigSync } from 'cosmiconfig'; import { findGitRoot, getDefaultRemoteBranch } from 'workspace-tools'; import type { RepoOptions, BeachballOptions, ParsedOptions } from '../types/BeachballOptions'; import path from 'path'; +import { BeachballError } from '../types/BeachballError'; /** * Find the beachball config file and return the repo options. @@ -39,10 +40,7 @@ export function getRepoOptions(cliOptions: ParsedOptions['cliOptions']): Partial if (configPath) { repoOptions = configExplorer.load(path.resolve(cwd, configPath))?.config as Partial | undefined; if (!repoOptions) { - console.error(`Config file "${configPath}" could not be loaded`); - // TODO: consider throwing instead - // eslint-disable-next-line no-restricted-properties - process.exit(1); + throw new BeachballError(`Config file "${configPath}" could not be loaded`); } } else { repoOptions = (configExplorer.search(cwd)?.config as Partial | undefined) || {}; diff --git a/src/publish/bumpAndPush.ts b/src/publish/bumpAndPush.ts index 5ec2728f6..f3d8980bd 100644 --- a/src/publish/bumpAndPush.ts +++ b/src/publish/bumpAndPush.ts @@ -6,6 +6,7 @@ import { tagPackages } from './tagPackages'; import { displayManualRecovery } from './displayManualRecovery'; import { gitFetch } from '../git/fetch'; import { gitAsync } from '../git/gitAsync'; +import { BeachballError } from '../types/BeachballError'; const bumpPushRetries = 5; /** Use verbose logging for these steps to make it easier to debug if something goes wrong */ @@ -87,9 +88,9 @@ export async function bumpAndPush( if (!completed) { displayManualRecovery(bumpInfo); - // TODO: consider throwing instead - // eslint-disable-next-line no-restricted-properties - process.exit(1); + throw new BeachballError(`Failed to bump and push after ${bumpPushRetries} attempts`, { + alreadyLogged: true, + }); } } diff --git a/src/publish/publishToRegistry.ts b/src/publish/publishToRegistry.ts index 5b2895f99..4c17d3cfb 100644 --- a/src/publish/publishToRegistry.ts +++ b/src/publish/publishToRegistry.ts @@ -12,6 +12,7 @@ import { getPackageGraph } from '../monorepo/getPackageGraph'; import type { PackageInfo } from '../types/PackageInfo'; import { packPackage } from '../packageManager/packPackage'; import { getCatalogs } from 'workspace-tools'; +import { BeachballError } from '../types/BeachballError'; /** * Publish all the bumped packages to the registry, OR if `packToPath` is specified, @@ -48,9 +49,7 @@ export async function publishToRegistry(bumpInfo: PublishBumpInfo, options: Beac if (invalid) { // Don't log anything since the validate functions already did it - // TODO: consider throwing instead - // eslint-disable-next-line no-restricted-properties - process.exit(1); + throw new BeachballError('Pre-publish validation failed', { alreadyLogged: true }); } // performing publishConfig and workspace version overrides requires this procedure to diff --git a/src/types/BeachballError.ts b/src/types/BeachballError.ts new file mode 100644 index 000000000..2565a2210 --- /dev/null +++ b/src/types/BeachballError.ts @@ -0,0 +1,17 @@ +/** + * Custom error class for expected/handled beachball errors. + * + * When `alreadyLogged` is true, it means the detailed error information has + * already been printed to stderr before the error was thrown. The top-level + * catch handler in cli.ts should NOT re-log the error details in that case. + */ +export class BeachballError extends Error { + /** If true, detailed error info was already logged via console.error before throwing. */ + readonly alreadyLogged: boolean; + + constructor(message: string, options?: { alreadyLogged?: boolean }) { + super(message); + this.name = 'BeachballError'; + this.alreadyLogged = !!options?.alreadyLogged; + } +} diff --git a/src/validation/validate.ts b/src/validation/validate.ts index a604cb294..2a3ba7e30 100644 --- a/src/validation/validate.ts +++ b/src/validation/validate.ts @@ -15,6 +15,7 @@ import { isValidDependentChangeType } from './isValidDependentChangeType'; import { getPackagesToPublish } from '../publish/getPackagesToPublish'; import { env } from '../env'; import { bulletedList } from '../logging/bulletedList'; +import { BeachballError } from '../types/BeachballError'; import type { BumpInfo } from '../types/BumpInfo'; import type { ChangeCommandContext, CommandContext } from '../types/CommandContext'; import { getScopedPackages } from '../monorepo/getScopedPackages'; @@ -49,7 +50,7 @@ type ValidationResult = { }; /** - * Run validation of options, change files, and packages. Exit 1 if it's invalid. + * Run validation of options, change files, and packages. Throws `BeachballError` if invalid. * If `validateOptions.checkChangeNeeded` is true, also check whether change files are needed. * @returns Various info retrieved during validation which is also needed by other functions. */ @@ -177,9 +178,7 @@ export function validate( if (hasError) { // If any of the above basic checks failed, it doesn't make sense to check if change files are needed - // TODO: consider throwing instead - // eslint-disable-next-line no-restricted-properties - process.exit(1); + throw new BeachballError('Validation failed', { alreadyLogged: true }); } let isChangeNeeded = false; @@ -200,16 +199,12 @@ export function validate( if (isChangeNeeded && !allowMissingChangeFiles) { logValidationError('Change files are needed!'); console.log(options.changehint); - // TODO: consider throwing instead - // eslint-disable-next-line no-restricted-properties - process.exit(1); // exit here (this is the main poin) + throw new BeachballError('Validation failed: change files are needed', { alreadyLogged: true }); } if (options.disallowDeletedChangeFiles && areChangeFilesDeleted(options)) { logValidationError('Change files must not be deleted!'); - // TODO: consider throwing instead - // eslint-disable-next-line no-restricted-properties - process.exit(1); + throw new BeachballError('Validation failed: change files were deleted', { alreadyLogged: true }); } } @@ -228,9 +223,7 @@ Consider one of the following solutions: - If the unpublished package should be published, remove \`"private": true\` from its package.json. - If it should NOT be published, verify that it is only listed under devDependencies of published packages. `); - // TODO: consider throwing instead - // eslint-disable-next-line no-restricted-properties - process.exit(1); + throw new BeachballError('Validation failed: invalid package dependencies', { alreadyLogged: true }); } } From 20bdc3869a4fb344a5491447493971706b276a05 Mon Sep 17 00:00:00 2001 From: Elizabeth Craig Date: Tue, 10 Mar 2026 16:35:13 -0700 Subject: [PATCH 3/3] Change files --- change/beachball-4c8bb372-aff5-438c-9706-ca20aaac11bb.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/beachball-4c8bb372-aff5-438c-9706-ca20aaac11bb.json diff --git a/change/beachball-4c8bb372-aff5-438c-9706-ca20aaac11bb.json b/change/beachball-4c8bb372-aff5-438c-9706-ca20aaac11bb.json new file mode 100644 index 000000000..033a203a8 --- /dev/null +++ b/change/beachball-4c8bb372-aff5-438c-9706-ca20aaac11bb.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "Centralize error handling instead of calling process.exit() throughout the code", + "packageName": "beachball", + "email": "elcraig@microsoft.com", + "dependentChangeType": "patch" +}