From 98fa5b398fc46f377dee20053b32c50734695d13 Mon Sep 17 00:00:00 2001 From: Elizabeth Craig Date: Tue, 26 Nov 2024 08:06:55 -0800 Subject: [PATCH] clarify change needed logic --- ...-c96251d0-33c6-465e-b32e-d7f0e1b1aecd.json | 7 ++++++ src/__e2e__/validate.test.ts | 8 +++---- src/cli.ts | 10 ++++---- src/types/BeachballOptions.ts | 8 ++++++- src/validation/validate.ts | 24 +++++++++---------- 5 files changed, 35 insertions(+), 22 deletions(-) create mode 100644 change/beachball-c96251d0-33c6-465e-b32e-d7f0e1b1aecd.json diff --git a/change/beachball-c96251d0-33c6-465e-b32e-d7f0e1b1aecd.json b/change/beachball-c96251d0-33c6-465e-b32e-d7f0e1b1aecd.json new file mode 100644 index 00000000..9eebb37e --- /dev/null +++ b/change/beachball-c96251d0-33c6-465e-b32e-d7f0e1b1aecd.json @@ -0,0 +1,7 @@ +{ + "type": "minor", + "comment": "bump command shouldn't check if change files are needed (+ clarify internal validation options)", + "packageName": "beachball", + "email": "elcraig@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/src/__e2e__/validate.test.ts b/src/__e2e__/validate.test.ts index c7d000b8..b94b9a24 100644 --- a/src/__e2e__/validate.test.ts +++ b/src/__e2e__/validate.test.ts @@ -41,7 +41,7 @@ describe('validate', () => { repo = repositoryFactory.cloneRepository(); repo.checkout('-b', 'test'); - const result = validate(getOptions()); + const result = validate(getOptions(), { checkChangeNeeded: true }); expect(result.isChangeNeeded).toBe(false); expect(logs.mocks.error).not.toHaveBeenCalled(); @@ -53,17 +53,17 @@ describe('validate', () => { repo.checkout('-b', 'test'); repo.stageChange('packages/foo/test.js'); - expect(() => validate(getOptions())).toThrowError(/process\.exit/); + expect(() => validate(getOptions(), { checkChangeNeeded: true })).toThrowError(/process\.exit/); expect(processExit).toHaveBeenCalledWith(1); expect(logs.mocks.error).toHaveBeenCalledWith('ERROR: Change files are needed!'); }); - it('returns an error if change files are needed and allowMissingChangeFiles is true', () => { + it('returns and does not log an error if change files are needed and allowMissingChangeFiles is true', () => { repo = repositoryFactory.cloneRepository(); repo.checkout('-b', 'test'); repo.stageChange('packages/foo/test.js'); - const result = validate(getOptions(), { allowMissingChangeFiles: true }); + const result = validate(getOptions(), { checkChangeNeeded: true, allowMissingChangeFiles: true }); expect(result.isChangeNeeded).toBe(true); expect(logs.mocks.error).not.toHaveBeenCalled(); }); diff --git a/src/cli.ts b/src/cli.ts index 79f0009a..61c60bad 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -25,12 +25,12 @@ import { validate } from './validation/validate'; // Run the commands switch (options.command) { case 'check': - validate(options); + validate(options, { checkChangeNeeded: true, checkDependencies: true }); console.log('No change files are needed'); break; case 'publish': - validate(options, { allowFetching: false }); + validate(options, { checkDependencies: true }); // set a default publish message options.message = options.message || 'applying package updates'; @@ -38,12 +38,12 @@ import { validate } from './validation/validate'; break; case 'bump': - validate(options); + validate(options, { checkDependencies: true }); await bump(options); break; case 'canary': - validate(options, { allowFetching: false }); + validate(options, { checkDependencies: true }); await canary(options); break; @@ -56,7 +56,7 @@ import { validate } from './validation/validate'; break; case 'change': { - const { isChangeNeeded } = validate(options, { allowMissingChangeFiles: true }); + const { isChangeNeeded } = validate(options, { checkChangeNeeded: true, allowMissingChangeFiles: true }); if (!isChangeNeeded && !options.package) { console.log('No change files are needed'); diff --git a/src/types/BeachballOptions.ts b/src/types/BeachballOptions.ts index 2727b114..ce77c1fc 100644 --- a/src/types/BeachballOptions.ts +++ b/src/types/BeachballOptions.ts @@ -14,6 +14,7 @@ export interface CliOptions | 'bumpDeps' | 'changehint' | 'changeDir' + | 'concurrency' | 'depth' | 'disallowedChangeTypes' | 'fetch' @@ -34,7 +35,6 @@ export interface CliOptions canaryName?: string | undefined; command: string; commit?: boolean; - concurrency: number; configPath?: string; dependentChangeType?: ChangeType; disallowDeletedChangeFiles?: boolean; @@ -104,6 +104,12 @@ export interface RepoOptions { changeDir: string; /** Options for customizing changelog rendering */ changelog?: ChangelogOptions; + /** + * Maximum concurrency. + * As of writing, concurrency only applies for calling hooks and publishing to npm. + * @default 1 + */ + concurrency: number; /** * The default dist-tag used for npm publish * @default 'latest' diff --git a/src/validation/validate.ts b/src/validation/validate.ts index 7f0ffd89..4e8fdd44 100644 --- a/src/validation/validate.ts +++ b/src/validation/validate.ts @@ -19,21 +19,22 @@ import { env } from '../env'; type ValidationOptions = { /** - * Defaults to true. If false, skip fetching the latest from the remote and don't check whether - * changes files are needed (or whether change files are deleted). + * If true, check whether change files are needed (and whether change files are deleted). */ - allowFetching?: boolean; + checkChangeNeeded?: boolean; /** - * If true, skip checking whether change files are needed. Ignored if `allowFetching` is false. + * If true, don't error if change files are needed (just return isChangeNeeded true). */ allowMissingChangeFiles?: boolean; + /** + * If true, validate that the dependencies of any packages with change files are valid + * (not private). + */ + checkDependencies?: boolean; }; -export function validate( - options: BeachballOptions, - validateOptions?: Partial -): { isChangeNeeded: boolean } { - const { allowMissingChangeFiles = false, allowFetching = true } = validateOptions || {}; +export function validate(options: BeachballOptions, validateOptions?: ValidationOptions): { isChangeNeeded: boolean } { + const { allowMissingChangeFiles, checkChangeNeeded, checkDependencies } = validateOptions || {}; console.log('\nValidating options and change files...'); @@ -145,8 +146,7 @@ export function validate( let isChangeNeeded = false; - if (allowFetching) { - // This has the side effect of fetching, so call it even if !allowMissingChangeFiles for now + if (checkChangeNeeded) { isChangeNeeded = isChangeFileNeeded(options, packageInfos); if (isChangeNeeded && !allowMissingChangeFiles) { @@ -161,7 +161,7 @@ export function validate( } } - if (!isChangeNeeded) { + if (!isChangeNeeded && checkDependencies && changeSet.length) { console.log('\nValidating package dependencies...'); // TODO: It would be preferable if this could be done without getting the full bump info, // or at least if the bump info could be passed back out to other methods which currently