Clarify internal validation logic + don't check for change needed in bump command (#1023)

clarify change needed logic

Author: ecraig12345

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": "[email protected]",
+  "dependentChangeType": "patch"
+}

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();
   });

src/cli.ts

@@ -25,25 +25,25 @@ 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';
       await publish(options);
       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');

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'

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<ValidationOptions>
-): { 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