For version groups, use the max change type from the group, not the first encountered (#1083)

ecraig12345 · web-flow · commit d0bc43c708ac · 2025-11-18T16:15:09.000-08:00
diff --git a/change/beachball-1a687403-625b-49a9-b681-841901a12186.json b/change/beachball-1a687403-625b-49a9-b681-841901a12186.json
@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "For version groups, use the max change type from the group, not the first one encountered",
+  "packageName": "beachball",
+  "email": "elcraig@microsoft.com",
+  "dependentChangeType": "patch"
+}
diff --git a/src/__e2e__/bump.test.ts b/src/__e2e__/bump.test.ts
@@ -236,6 +236,50 @@ describe('version bumping', () => {
     expect(changeFiles).toHaveLength(0);
   });
 
+  it('bumps all grouped packages to the greatest change type in the group, regardless of change file order', async () => {
+    repositoryFactory = new RepositoryFactory('monorepo');
+    repo = repositoryFactory.cloneRepository();
+
+    repo.commitChange('packages/commonlib/package.json', {
+      // The prefix z- here ensures commonlib's change file is loaded AFTER its dependents.
+      // This makes sure we set the group's version bumps based on ChangeType order and not in
+      // the sort order the filesystem gives us.
+      name: 'z-commonlib',
+      version: '1.0.0',
+    });
+    repo.commitChange('packages/pkg-1/package.json', {
+      name: 'pkg-1',
+      version: '1.0.0',
+      dependencies: {
+        'z-commonlib': '1.0.0',
+      },
+    });
+
+    const options = getOptions({
+      groups: [{ include: 'packages/*', disallowedChangeTypes: null, name: 'grp' }],
+      bumpDeps: true,
+      commit: true,
+    });
+    generateChangeFiles(
+      [
+        { packageName: 'z-commonlib', type: 'none', dependentChangeType: 'none' },
+        { packageName: 'pkg-1', type: 'minor', dependentChangeType: 'minor' },
+      ],
+      options
+    );
+    repo.push();
+
+    await bump(options);
+
+    const packageInfos = getPackageInfos(repo.rootPath);
+
+    expect(packageInfos['pkg-1'].version).toBe('1.1.0');
+    expect(packageInfos['z-commonlib'].version).toBe('1.1.0');
+
+    const changeFiles = getChangeFiles(options);
+    expect(changeFiles).toHaveLength(0);
+  });
+
   it('bumps all grouped AND dependent packages', async () => {
     const monorepo: RepoFixture['folders'] = {
       'packages/grp': {
diff --git a/src/__functional__/changelog/writeChangelog.test.ts b/src/__functional__/changelog/writeChangelog.test.ts
@@ -54,10 +54,10 @@ describe('writeChangelog', () => {
     const calculatedChangeTypes: BumpInfo['calculatedChangeTypes'] = {};
     for (const { change } of changeFileChangeInfos) {
       const { packageName, type } = change;
-      calculatedChangeTypes[packageName] = getMaxChangeType(type, calculatedChangeTypes[packageName]);
+      calculatedChangeTypes[packageName] = getMaxChangeType([type, calculatedChangeTypes[packageName]]);
     }
     for (const pkgName of Object.keys(dependentChangedBy)) {
-      calculatedChangeTypes[pkgName] = getMaxChangeType('patch', calculatedChangeTypes[pkgName]);
+      calculatedChangeTypes[pkgName] = getMaxChangeType(['patch', calculatedChangeTypes[pkgName]]);
     }
 
     // Bump versions in package info and package.json for more realistic changelogs.
diff --git a/src/__tests__/changefile/changeTypes.test.ts b/src/__tests__/changefile/changeTypes.test.ts
@@ -3,48 +3,52 @@ import { getMaxChangeType, initializePackageChangeTypes } from '../../changefile
 import type { ChangeSet } from '../../types/ChangeInfo';
 
 describe('getMaxChangeType', () => {
+  it('handles empty change type array', () => {
+    expect(getMaxChangeType([], null)).toBe('none');
+  });
+
+  it('handles single change type', () => {
+    expect(getMaxChangeType(['patch'], null)).toBe('patch');
+  });
+
   it('handles equal change types', () => {
-    const changeType = getMaxChangeType('patch', 'patch', null);
+    const changeType = getMaxChangeType(['patch', 'patch'], null);
     expect(changeType).toBe('patch');
   });
 
   it('returns greater change type without disallowedChangeTypes', () => {
-    expect(getMaxChangeType('patch', 'minor', null)).toBe('minor');
-    expect(getMaxChangeType('minor', 'patch', null)).toBe('minor');
-    expect(getMaxChangeType('minor', 'major', null)).toBe('major');
-    expect(getMaxChangeType('patch', 'major', null)).toBe('major');
-    expect(getMaxChangeType('patch', 'prerelease', null)).toBe('patch');
-    expect(getMaxChangeType('patch', 'none', null)).toBe('patch');
-    expect(getMaxChangeType('prerelease', 'none', null)).toBe('prerelease');
+    expect(getMaxChangeType(['patch', 'minor'], null)).toBe('minor');
+    expect(getMaxChangeType(['minor', 'patch'], null)).toBe('minor');
+    expect(getMaxChangeType(['minor', 'major'], null)).toBe('major');
+    expect(getMaxChangeType(['patch', 'major'], null)).toBe('major');
+    expect(getMaxChangeType(['patch', 'prerelease'], null)).toBe('patch');
+    expect(getMaxChangeType(['patch', 'none'], null)).toBe('patch');
+    expect(getMaxChangeType(['prerelease', 'none'], null)).toBe('prerelease');
+  });
+
+  it('handles longer array of changeTypes with max in middle', () => {
+    const changeType = getMaxChangeType(['patch', 'minor', 'major', 'patch'], null);
+    expect(changeType).toBe('major');
   });
 
   it('returns none if all given change types are disallowed', () => {
-    const changeType = getMaxChangeType('patch', 'major', [
-      'major',
-      'minor',
-      'patch',
-      'prerelease',
-      'premajor',
-      'preminor',
-      'prepatch',
-    ]);
+    const changeType = getMaxChangeType(
+      ['patch', 'major'],
+      ['major', 'minor', 'patch', 'prerelease', 'premajor', 'preminor', 'prepatch']
+    );
     expect(changeType).toBe('none');
   });
 
   it('returns next greatest change type if max is disallowed', () => {
-    const changeType = getMaxChangeType('patch', 'major', ['major', 'premajor', 'preminor', 'prepatch']);
+    const changeType = getMaxChangeType(['patch', 'major'], ['major', 'premajor', 'preminor', 'prepatch']);
     expect(changeType).toBe('minor');
   });
 
   it('handles prerelease only case', () => {
-    const changeType = getMaxChangeType('patch', 'major', [
-      'major',
-      'minor',
-      'patch',
-      'premajor',
-      'preminor',
-      'prepatch',
-    ]);
+    const changeType = getMaxChangeType(
+      ['patch', 'major'],
+      ['major', 'minor', 'patch', 'premajor', 'preminor', 'prepatch']
+    );
     expect(changeType).toBe('prerelease');
   });
 });
diff --git a/src/bump/bumpInPlace.ts b/src/bump/bumpInPlace.ts
@@ -4,6 +4,8 @@ import { updateRelatedChangeType } from './updateRelatedChangeType';
 import { bumpPackageInfoVersion } from './bumpPackageInfoVersion';
 import type { BeachballOptions } from '../types/BeachballOptions';
 import { setDependentVersions } from './setDependentVersions';
+import { getMaxChangeType } from '../changefile/changeTypes';
+import { ChangeType } from '../types/ChangeInfo';
 
 /**
  * Updates BumpInfo according to change types, bump deps, and version groups
@@ -22,12 +24,21 @@ export function bumpInPlace(bumpInfo: BumpInfo, options: BeachballOptions): void
   //       - the main concern is how to capture the bump reason in grouped changelog
 
   // pass 2: initialize grouped calculatedChangeTypes together
-  for (const { change: changeInfo } of changeFileChangeInfos) {
-    const group = Object.values(bumpInfo.packageGroups).find(g => g.packageNames.includes(changeInfo.packageName));
+  for (const group of Object.values(bumpInfo.packageGroups)) {
+    // If any of the group's packages have a change, find the max change type out of any package in the group.
+    const seenTypes = new Set<ChangeType>();
+    for (const packageNameInGroup of group.packageNames) {
+      const changeType = calculatedChangeTypes[packageNameInGroup];
+      if (changeType) {
+        seenTypes.add(changeType);
+      }
+    }
 
-    if (group) {
+    if (seenTypes.size) {
+      // Set all packages in the group to the max change type.
+      const maxChangeInGroup = getMaxChangeType([...seenTypes], group.disallowedChangeTypes);
       for (const packageNameInGroup of group.packageNames) {
-        calculatedChangeTypes[packageNameInGroup] = changeInfo.type;
+        calculatedChangeTypes[packageNameInGroup] = maxChangeInGroup;
       }
     }
   }
diff --git a/src/bump/updateRelatedChangeType.ts b/src/bump/updateRelatedChangeType.ts
@@ -35,7 +35,7 @@ export function updateRelatedChangeType(params: {
   const { calculatedChangeTypes, packageGroups, packageInfos } = bumpInfo;
 
   // If dependentChangeType is none (or somehow unset), there's nothing to do.
-  const updatedChangeType = getMaxChangeType(change.dependentChangeType);
+  const updatedChangeType = getMaxChangeType([change.dependentChangeType]);
   if (updatedChangeType === 'none') {
     return;
   }
@@ -56,8 +56,7 @@ export function updateRelatedChangeType(params: {
     if (subjectPackage !== entryPointPackageName) {
       const oldType = calculatedChangeTypes[subjectPackage];
       calculatedChangeTypes[subjectPackage] = getMaxChangeType(
-        oldType,
-        changeType,
+        [oldType, changeType],
         packageInfos[subjectPackage]?.combinedOptions?.disallowedChangeTypes
       );
 
diff --git a/src/changefile/changeTypes.ts b/src/changefile/changeTypes.ts
@@ -34,7 +34,7 @@ export function initializePackageChangeTypes(changeSet: ChangeSet): BumpInfo['ca
 
   for (const { change } of changeSet) {
     const { packageName: pkg } = change;
-    const changeType = getMaxChangeType(change.type, pkgChangeTypes[pkg]);
+    const changeType = getMaxChangeType([change.type, pkgChangeTypes[pkg]]);
     // It's best to totally ignore "none" changes to do a bit less processing.
     if (changeType !== 'none') {
       pkgChangeTypes[pkg] = changeType;
@@ -58,19 +58,25 @@ function getAllowedChangeType(changeType: ChangeType, disallowedChangeTypes: Rea
 }
 
 /**
- * Get the max allowed change type based on `a` and `b`, accounting for disallowed change types:
- * e.g. if `a` is "major" and `b` is "patch", and "major" is disallowed, the result will be "minor"
+ * Get the max allowed change type out of `changeTypes`, accounting for disallowed change types:
+ * e.g. if `changeTypes` is `["major", "patch"]`, and `"major"` is disallowed, the result will be `"minor"`
  * (the greatest allowed change type).
  */
 export function getMaxChangeType(
-  a: ChangeType | undefined,
-  b?: ChangeType,
+  changeTypes: (ChangeType | undefined)[],
   disallowedChangeTypes?: ReadonlyArray<ChangeType> | null
 ): ChangeType {
-  if (disallowedChangeTypes?.length) {
-    a = a && getAllowedChangeType(a, disallowedChangeTypes);
-    b = b && getAllowedChangeType(b, disallowedChangeTypes);
+  let max: ChangeType = 'none';
+
+  for (let changeType of changeTypes) {
+    if (!changeType) {
+      continue;
+    }
+    if (disallowedChangeTypes?.length) {
+      changeType = getAllowedChangeType(changeType, disallowedChangeTypes);
+    }
+    max = ChangeTypeWeights[changeType] > ChangeTypeWeights[max] ? changeType : max;
   }
 
-  return a && b ? (ChangeTypeWeights[a] > ChangeTypeWeights[b] ? a : b) : a || b || 'none';
+  return max;
 }
diff --git a/src/changelog/writeChangelog.ts b/src/changelog/writeChangelog.ts
@@ -68,7 +68,7 @@ async function writeGroupedChangelog(
   // Validate groups and initialize groupedChangelogs
   for (const group of changelogGroups) {
     const { changelogAbsDir } = group;
-    const mainPackageName = 'mainPackageName' in group ? group.mainPackageName : group.masterPackageName;
+    const mainPackageName = group.mainPackageName ?? group.masterPackageName!;
     const mainPackage = packageInfos[mainPackageName];
     if (!mainPackage) {
       console.warn(`main package ${mainPackageName} does not exist.`);
diff --git a/src/types/BeachballOptions.ts b/src/types/BeachballOptions.ts
@@ -150,7 +150,10 @@ export interface RepoOptions {
    * - `'json'` to generate only CHANGELOG.json
    */
   generateChangelog: boolean | 'md' | 'json';
-  /** Options for bumping package versions together */
+  /**
+   * Options for bumping package versions together.
+   * (For changelog groups, use `BeachballOptions.changelog.groups`.)
+   */
   groups?: VersionGroupOptions[];
   /**
    * Whether to create git tags for published packages
@@ -255,6 +258,8 @@ export interface PackageOptions {
 
 /**
  * Options for bumping package versions together.
+ *
+ * For changelog groups, use `BeachballOptions.changelog.groups` (`ChangelogGroupOptions`).
  */
 export interface VersionGroupOptions {
   /** name of the version group */
diff --git a/src/types/BumpInfo.ts b/src/types/BumpInfo.ts
@@ -23,7 +23,9 @@ export type BumpInfo = {
    */
   calculatedChangeTypes: { [pkgName: string]: ChangeType };
 
-  /** Package grouping */
+  /**
+   * Package version groups (not changelog groups) derived from `BeachballOptions.groups` (`VersionGroupOptions`).
+   */
   packageGroups: DeepReadonly<PackageGroups>;
 
   /** Set of packages that had been modified */
diff --git a/src/types/ChangelogOptions.ts b/src/types/ChangelogOptions.ts
@@ -9,7 +9,12 @@ import type { ChangeType } from './ChangeInfo';
  * to achieve the desired level of customization.
  */
 export interface ChangelogOptions {
-  /** Options for grouping packages together in a single changelog. */
+  /**
+   * Options for grouping packages together in a single changelog.
+   *
+   * Note that this only impacts changelogs, not version bumping.
+   * For version bumping, use `BeachballOptions.groups` (`VersionGroupOptions`).
+   */
   groups?: ChangelogGroupOptions[];
 
   /**
@@ -62,7 +67,13 @@ export interface ChangelogOptions {
   includeCommitHashes?: boolean;
 }
 
-interface ChangelogGroupOptionsBase {
+/**
+ * Options for generating a changelog for a group of packages.
+ *
+ * Note that this only impacts changelogs, not version bumping.
+ * For version bumping, use `BeachballOptions.groups` (`VersionGroupOptions`).
+ */
+export interface ChangelogGroupOptions {
   /**
    * minimatch pattern(s) for package paths to include in this group.
    * Patterns are relative to the repo root and must use forward slashes.
@@ -84,24 +95,16 @@ interface ChangelogGroupOptionsBase {
    * Can be relative to the root, or absolute.
    */
   changelogPath: string;
-}
 
-/**
- * Options for generating a changelog for a group of packages.
- */
-export type ChangelogGroupOptions =
-  | (ChangelogGroupOptionsBase & {
-      /**
-       * The main package which a group of changes bubbles up to.
-       * All changes within the group are used to describe changes for the main package.
-       */
-      mainPackageName: string;
-    })
-  | (ChangelogGroupOptionsBase & {
-      /** @deprecated Use `mainPackageName` */
-      // Change the type back to a single interface once this is removed
-      masterPackageName: string;
-    });
+  /**
+   * The main package which a group of changes bubbles up to.
+   * All changes within the group are used to describe changes for the main package.
+   */
+  mainPackageName: string;
+
+  /** @deprecated Use `mainPackageName` */
+  masterPackageName?: string;
+}
 
 /**
  * Info used for rendering the changelog markdown for a particular package version.
diff --git a/src/types/PackageInfo.ts b/src/types/PackageInfo.ts
@@ -63,6 +63,9 @@ export interface PackageGroupsInfo {
   disallowedChangeTypes: ChangeType[] | null;
 }
 
+/**
+ * Package version groups (not changelog groups) derived from `BeachballOptions.groups` (`VersionGroupOptions`).
+ */
 export type PackageGroups = { [groupName: string]: PackageGroupsInfo };
 
 /** Types of dependencies to consider when bumping. */

Original file line number	Diff line number	Diff line change
`@@ -54,10 +54,10 @@ describe('writeChangelog', () => {`
`54`	`54`	`const calculatedChangeTypes: BumpInfo['calculatedChangeTypes'] = {};`
`55`	`55`	`for (const { change } of changeFileChangeInfos) {`
`56`	`56`	`const { packageName, type } = change;`
`57`		`- calculatedChangeTypes[packageName] = getMaxChangeType(type, calculatedChangeTypes[packageName]);`
	`57`	`+ calculatedChangeTypes[packageName] = getMaxChangeType([type, calculatedChangeTypes[packageName]]);`
`58`	`58`	`}`
`59`	`59`	`for (const pkgName of Object.keys(dependentChangedBy)) {`
`60`		`- calculatedChangeTypes[pkgName] = getMaxChangeType('patch', calculatedChangeTypes[pkgName]);`
	`60`	`+ calculatedChangeTypes[pkgName] = getMaxChangeType(['patch', calculatedChangeTypes[pkgName]]);`
`61`	`61`	`}`
`62`	`62`
`63`	`63`	`// Bump versions in package info and package.json for more realistic changelogs.`