Fix early bail bug in updateRelatedChangeType

change/beachball-777cc61a-b404-4be0-94f5-bd154643ecaa.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Don't bail early when updating calculated change types if one change file referenced an invalid package",
+  "packageName": "beachball",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}

src/bump/bumpInPlace.ts

@@ -39,6 +39,7 @@ export function bumpInPlace(bumpInfo: BumpInfo, options: BeachballOptions): void
     }
   }
 
+  // Calculate change types for packages and dependencies
   for (const { changeFile } of changeFileChangeInfos) {
     updateRelatedChangeType(changeFile, bumpInfo, bumpDeps);
   }

src/bump/gatherBumpInfo.ts

@@ -6,6 +6,9 @@ import { BeachballOptions } from '../types/BeachballOptions';
 import { getScopedPackages } from '../monorepo/getScopedPackages';
 import { PackageInfos } from '../types/PackageInfo';
 
+/**
+ * Gather bump info and bump versions in memory.
+ */
 export function gatherBumpInfo(options: BeachballOptions, packageInfos: PackageInfos): BumpInfo {
   // Collate the changes per package
   const changes = readChangeFiles(options, packageInfos);

src/bump/setDependentVersions.ts

@@ -1,31 +1,40 @@
 import type { BeachballOptions } from '../types/BeachballOptions';
+import { BumpInfo } from '../types/BumpInfo';
 import type { PackageInfos } from '../types/PackageInfo';
 import { bumpMinSemverRange } from './bumpMinSemverRange';
 
+/**
+ * Go through the deps of each package and bump the version range for in-repo deps if needed.
+ *
+ * **This mutates dep versions in `packageInfos`** as well as returning `dependentChangedBy`.
+ */
 export function setDependentVersions(
   packageInfos: PackageInfos,
   scopedPackages: Set<string>,
-  { verbose }: BeachballOptions
-): { [dependent: string]: Set<string> } {
-  const dependentChangedBy: { [dependent: string]: Set<string> } = {};
+  options: Pick<BeachballOptions, 'verbose'>
+): BumpInfo['dependentChangedBy'] {
+  const { verbose } = options;
+  const dependentChangedBy: BumpInfo['dependentChangedBy'] = {};
 
   for (const [pkgName, info] of Object.entries(packageInfos)) {
     if (!scopedPackages.has(pkgName)) {
-      continue;
+      continue; // out of scope
     }
 
     for (const deps of [info.dependencies, info.devDependencies, info.peerDependencies, info.optionalDependencies]) {
       if (!deps) {
-        continue;
+        continue; // package doesn't have this dep type
       }
 
       for (const [dep, existingVersionRange] of Object.entries(deps)) {
-        const packageInfo = packageInfos[dep];
-        if (!packageInfo) {
-          continue;
+        const depPackage = packageInfos[dep];
+        if (!depPackage) {
+          continue; // external dependency
         }
 
-        const bumpedVersionRange = bumpMinSemverRange(packageInfo.version, existingVersionRange);
+        const bumpedVersionRange = bumpMinSemverRange(depPackage.version, existingVersionRange);
+        // TODO: dependent bumps in workspace:*/^/~ ranges will be missed
+        // https://github.com/microsoft/beachball/issues/981
         if (existingVersionRange !== bumpedVersionRange) {
           deps[dep] = bumpedVersionRange;
 

src/bump/setDependentsInBumpInfo.ts

@@ -1,13 +1,14 @@
 import type { BumpInfo } from '../types/BumpInfo';
 
 /**
- * Gets dependents for all packages
+ * Set dependents for all packages. **This mutates `bumpInfo.dependents`.**
  *
  * Example: "BigApp" deps on "SomeUtil", "BigApp" would be the dependent
  */
-export function setDependentsInBumpInfo(bumpInfo: BumpInfo): void {
-  const { packageInfos, scopedPackages } = bumpInfo;
-  const dependents: BumpInfo['dependents'] = {};
+export function setDependentsInBumpInfo(
+  bumpInfo: Pick<BumpInfo, 'packageInfos' | 'scopedPackages' | 'dependents'>
+): void {
+  const { packageInfos, scopedPackages, dependents } = bumpInfo;
 
   for (const [pkgName, info] of Object.entries(packageInfos)) {
     if (!scopedPackages.has(pkgName)) {
@@ -25,6 +26,4 @@ export function setDependentsInBumpInfo(bumpInfo: BumpInfo): void {
       }
     }
   }
-
-  bumpInfo.dependents = dependents;
 }

src/bump/setGroupsInBumpInfo.ts

@@ -2,7 +2,13 @@ import { BeachballOptions } from '../types/BeachballOptions';
 import { BumpInfo } from '../types/BumpInfo';
 import { getPackageGroups } from '../monorepo/getPackageGroups';
 
-export function setGroupsInBumpInfo(bumpInfo: BumpInfo, options: BeachballOptions): void {
+/**
+ * Set `bumpInfo.packageGroups` and `bumpInfo.groupOptions` based on `options.groups`.
+ */
+export function setGroupsInBumpInfo(
+  bumpInfo: Pick<BumpInfo, 'packageGroups' | 'packageInfos' | 'groupOptions'>,
+  options: Pick<BeachballOptions, 'groups' | 'path'>
+): void {
   if (options.groups) {
     bumpInfo.packageGroups = getPackageGroups(bumpInfo.packageInfos, options.path, options.groups);
 

src/bump/updateRelatedChangeType.ts

@@ -1,6 +1,5 @@
 import { getMaxChangeType, MinChangeType } from '../changefile/changeTypes';
 import { BumpInfo } from '../types/BumpInfo';
-import { ChangeType } from '../types/ChangeInfo';
 
 /**
  * This is the core of the bumpInfo dependency bumping logic - done once per change file
@@ -13,7 +12,7 @@ import { ChangeType } from '../types/ChangeInfo';
  * - this function is the primary way for package groups to get the same dependent change type by queueing up
  *   all packages within a group to be visited by the loop
  *
- * What is mutates:
+ * What it mutates:
  * - bumpInfo.calculatedChangeTypes: updates packages change type modifed by this function
  * - all dependents change types as part of a group update
  *
@@ -26,17 +25,22 @@ export function updateRelatedChangeType(changeFile: string, bumpInfo: BumpInfo,
   /** [^1]: all the information needed from `bumpInfo` */
   const { calculatedChangeTypes, packageGroups, dependents, packageInfos, groupOptions } = bumpInfo;
 
-  const changesForFile = bumpInfo.changeFileChangeInfos.filter(info => info.changeFile === changeFile);
-  for (const { change: changeFileChangeInfo } of changesForFile) {
-    const entryPointPackageName = changeFileChangeInfo.packageName;
-    const dependentChangeType = changeFileChangeInfo.dependentChangeType;
+  for (const info of bumpInfo.changeFileChangeInfos) {
+    if (info.changeFile !== changeFile) {
+      continue;
+    }
+
+    const {
+      change: { packageName: entryPointPackageName, dependentChangeType },
+    } = info;
 
-    // Do not do anything if packageInfo is not present: it means this was an invalid changefile that somehow got checked in
+    // Do not do anything if packageInfo is not present: it means this was an invalid changefile that
+    // somehow got checked in. (This should have already been caught by readChangeFiles, but just in case.)
     if (!packageInfos[entryPointPackageName]) {
-      return;
+      continue;
     }
 
-    let updatedChangeType = getMaxChangeType(dependentChangeType, MinChangeType, []);
+    let updatedChangeType = getMaxChangeType(dependentChangeType, MinChangeType);
 
     const queue = [{ subjectPackage: entryPointPackageName, changeType: MinChangeType }];
 
@@ -46,64 +50,59 @@ export function updateRelatedChangeType(changeFile: string, bumpInfo: BumpInfo,
     while (queue.length > 0) {
       let { subjectPackage, changeType } = queue.shift()!;
 
-      if (!visited.has(subjectPackage)) {
-        visited.add(subjectPackage);
+      if (visited.has(subjectPackage)) {
+        continue;
+      }
 
-        // Step 1. Update change type of the subjectPackage according to the dependent change type propagation
-        const packageInfo = packageInfos[subjectPackage];
+      visited.add(subjectPackage);
 
-        if (!packageInfo) {
-          continue;
-        }
+      // Step 1. Update change type of the subjectPackage according to the dependent change type propagation
+      const packageInfo = packageInfos[subjectPackage];
+      if (!packageInfo) {
+        continue;
+      }
 
-        const disallowedChangeTypes = packageInfo.combinedOptions?.disallowedChangeTypes ?? [];
+      const disallowedChangeTypes = packageInfo.combinedOptions?.disallowedChangeTypes ?? [];
 
-        if (subjectPackage !== entryPointPackageName) {
-          updateChangeType(subjectPackage, changeType, disallowedChangeTypes);
-        }
+      if (subjectPackage !== entryPointPackageName) {
+        calculatedChangeTypes[subjectPackage] = getMaxChangeType(
+          calculatedChangeTypes[subjectPackage],
+          changeType,
+          disallowedChangeTypes
+        );
+      }
 
-        // Step 2. For all dependent packages of the current subjectPackage, place in queue to be updated at least to the "updatedChangeType"
-        const dependentPackages = dependents[subjectPackage];
+      // Step 2. For all dependent packages of the current subjectPackage, place in queue to be updated at least to the "updatedChangeType"
+      const dependentPackages = dependents[subjectPackage];
 
-        if (bumpDeps && dependentPackages && dependentPackages.length > 0) {
-          for (const dependentPackage of dependentPackages) {
-            queue.push({
-              subjectPackage: dependentPackage,
-              changeType: updatedChangeType,
-            });
-          }
+      if (bumpDeps && dependentPackages && dependentPackages.length > 0) {
+        for (const dependentPackage of dependentPackages) {
+          queue.push({
+            subjectPackage: dependentPackage,
+            changeType: updatedChangeType,
+          });
         }
+      }
 
-        // TODO: when we do "locked", or "lock step" versioning, we could simply skip this grouped traversal,
-        //       - set the version for all packages in the group in (bumpPackageInfoVersion())
-        //       - the main concern is how to capture the bump reason in grouped changelog
+      // TODO: when we do "locked", or "lock step" versioning, we could simply skip this grouped traversal,
+      //       - set the version for all packages in the group in (bumpPackageInfoVersion())
+      //       - the main concern is how to capture the bump reason in grouped changelog
 
-        // Step 3. For group-linked packages, update the change type to the max(change file info's change type, propagated update change type)
-        const groupName = Object.keys(packageGroups).find(group =>
-          packageGroups[group].packageNames.includes(packageInfo.name)
-        );
+      // Step 3. For group-linked packages, update the change type to the max(change file info's change type, propagated update change type)
+      const groupName = Object.keys(packageGroups).find(group =>
+        packageGroups[group].packageNames.includes(packageInfo.name)
+      );
 
-        if (groupName) {
-          for (const packageNameInGroup of packageGroups[groupName].packageNames) {
-            if (
-              !groupOptions[groupName] ||
-              !groupOptions[groupName]?.disallowedChangeTypes?.includes(updatedChangeType)
-            ) {
-              queue.push({
-                subjectPackage: packageNameInGroup,
-                changeType: updatedChangeType,
-              });
-            }
+      if (groupName) {
+        for (const packageNameInGroup of packageGroups[groupName].packageNames) {
+          if (!groupOptions[groupName]?.disallowedChangeTypes?.includes(updatedChangeType)) {
+            queue.push({
+              subjectPackage: packageNameInGroup,
+              changeType: updatedChangeType,
+            });
           }
         }
       }
     }
   }
-
-  function updateChangeType(pkg: string, changeType: ChangeType, disallowedChangeTypes: ChangeType[]): ChangeType {
-    const newChangeType = getMaxChangeType(calculatedChangeTypes[pkg], changeType, disallowedChangeTypes);
-    calculatedChangeTypes[pkg] = newChangeType;
-
-    return newChangeType;
-  }
 }

src/changefile/changeTypes.ts

@@ -58,7 +58,14 @@ function getAllowedChangeType(changeType: ChangeType, disallowedChangeTypes: Cha
  * e.g. if `a` is "major" and `b` is "patch", and "major" is disallowed, the result will be "minor"
  * (the greatest allowed change type).
  */
-export function getMaxChangeType(a: ChangeType, b: ChangeType, disallowedChangeTypes: ChangeType[] | null): ChangeType {
+export function getMaxChangeType(
+  a: ChangeType | undefined,
+  b: ChangeType | undefined,
+  disallowedChangeTypes?: ChangeType[] | null
+): ChangeType {
+  a ??= MinChangeType;
+  b ??= MinChangeType;
+
   if (disallowedChangeTypes) {
     a = getAllowedChangeType(a, disallowedChangeTypes);
     b = getAllowedChangeType(b, disallowedChangeTypes);

src/changelog/mergeChangelogs.ts

@@ -25,13 +25,14 @@ export function mergeChangelogs(
     comments: {},
   };
 
-  changelogs.forEach(changelog => {
-    (Object.keys(changelog.comments) as ChangeType[]).forEach(changeType => {
-      if (changelog.comments[changeType]) {
-        result.comments[changeType] = (result.comments[changeType] || []).concat(changelog.comments[changeType]!);
+  for (const changelog of changelogs) {
+    for (const changeType of Object.keys(changelog.comments) as ChangeType[]) {
+      const comments = changelog.comments[changeType];
+      if (comments?.length) {
+        (result.comments[changeType] ??= []).push(...comments);
       }
-    });
-  });
+    }
+  }
 
   return result;
 }

src/types/BumpInfo.ts

@@ -27,6 +27,7 @@ export type BumpInfo = {
   /** Set of new packages detected in this info */
   newPackages: Set<string>;
 
+  /** Map from package name to its internal dependency names that were bumped. */
   dependentChangedBy: { [pkgName: string]: Set<string> };
 
   /** Set of packages that are in scope for this bump */

src/types/ChangelogOptions.ts

@@ -58,6 +58,10 @@ export interface ChangelogGroupOptions {
    */
   exclude?: string | string[];
 
+  /**
+   * Put the grouped changelog file under this directory.
+   * Can be relative to the root, or absolute.
+   */
   changelogPath: string;
 }