Fix handling of group disallowedChangeTypes with dependent bumps

change/change-e5e83f54-1a36-4545-9dcc-ee8af7201e1e.json

@@ -0,0 +1,11 @@
+{
+  "changes": [
+    {
+      "type": "minor",
+      "comment": "Fix handling of group `disallowedChangeTypes` in dependent bumps (`updateRelatedChangeType`). This should be non-breaking but has a chance of unexpected side effects.",
+      "packageName": "beachball",
+      "email": "elcraig@microsoft.com",
+      "dependentChangeType": "patch"
+    }
+  ]
+}

packages/beachball/src/__tests__/bump/updateRelatedChangeType.test.ts

@@ -13,26 +13,24 @@ describe('updateRelatedChangeType', () => {
    * Call `updateRelatedChangeType` once for each of `changes`.
    * Returns the updated bump info.
    */
-  function callUpdateRelatedChangeType(
-    options: Partial<Pick<BeachballOptions, 'bumpDeps'>> & {
-      changes: Array<Pick<ChangeInfo, 'packageName' | 'type' | 'dependentChangeType'>>;
-      /**
-       * All the packages used in this fixture, including any per-package beachball options.
-       * Must include any dependencies (all versions are 1.0.0).
-       */
-      packages: PartialPackageInfos;
-      /** Repo disallowed change types */
-      options?: Pick<BeachballOptions, 'disallowedChangeTypes'>;
-      /**
-       * Initial calculated change types before updates. This is **required** if `packageGroups`
-       * is specified (since the initial calculation is complex) but otherwise a default can be
-       * calculated from `changes`.
-       */
-      calculatedChangeTypes?: { [packageName: string]: ChangeType };
-      packageGroups?: PackageGroups;
-    }
-  ) {
-    const { packages, packageGroups, bumpDeps = true } = options;
+  function callUpdateRelatedChangeType(options: {
+    changes: Array<Pick<ChangeInfo, 'packageName' | 'type' | 'dependentChangeType'>>;
+    /**
+     * All the packages used in this fixture, including any per-package beachball options.
+     * Must include any dependencies (all versions are 1.0.0).
+     */
+    packages: PartialPackageInfos;
+    /** Repo disallowed change types */
+    options?: Pick<BeachballOptions, 'disallowedChangeTypes'>;
+    /**
+     * Initial calculated change types before updates. This is **required** if `packageGroups`
+     * is specified (since the initial calculation is complex) but otherwise a default can be
+     * calculated from `changes`.
+     */
+    calculatedChangeTypes?: { [packageName: string]: ChangeType };
+    packageGroups?: PackageGroups;
+  }) {
+    const { packages, packageGroups } = options;
 
     if (packageGroups && !options.calculatedChangeTypes) {
       throw new Error('calculatedChangeTypes must be specified if packageGroups is used');
@@ -46,6 +44,14 @@ describe('updateRelatedChangeType', () => {
     }));
 
     const packageInfos = makePackageInfos(packages);
+    const packageToGroup: Record<string, string> = {};
+    if (packageGroups) {
+      for (const [groupName, group] of Object.entries(packageGroups)) {
+        for (const packageName of group.packageNames) {
+          packageToGroup[packageName] = groupName;
+        }
+      }
+    }
 
     const params: RelatedChangeTypeParams = {
       bumpInfo: {
@@ -57,9 +63,8 @@ describe('updateRelatedChangeType', () => {
       options: { disallowedChangeTypes: null, ...options.options },
       // Dependents are confusing to reason about directly (or specify in fixtures) since they're
       // backwards from dependencies, so just reuse the actual helper that calculates them
-      dependents: bumpDeps
-        ? getDependentsForPackages({ packageInfos, scopedPackages: new Set(Object.keys(packageInfos)) })
-        : undefined,
+      dependents: getDependentsForPackages({ packageInfos, scopedPackages: new Set(Object.keys(packageInfos)) }),
+      packageToGroup,
     };
 
     for (const change of changes) {
@@ -84,19 +89,6 @@ describe('updateRelatedChangeType', () => {
     });
   });
 
-  it('does not bump dependents with bumpDeps: false', () => {
-    const bumpInfo = callUpdateRelatedChangeType({
-      bumpDeps: false,
-      changes: [{ packageName: 'foo', type: 'patch', dependentChangeType: 'minor' }],
-      packages: {
-        bar: { dependencies: { foo: '1.0.0' } },
-        foo: {},
-      },
-    });
-
-    expect(bumpInfo.calculatedChangeTypes).toEqual({ foo: 'patch' });
-  });
-
   it("respects bumped dependent package's own change type if higher than dependentChangeType", () => {
     const bumpInfo = callUpdateRelatedChangeType({
       changes: [
@@ -165,10 +157,29 @@ describe('updateRelatedChangeType', () => {
     });
   });
 
+  it('bumps all grouped packages, if a dependency was bumped and group was not previously bumped', () => {
+    const bumpInfo = callUpdateRelatedChangeType({
+      changes: [{ packageName: 'dep', type: 'major', dependentChangeType: 'minor' }],
+      calculatedChangeTypes: { dep: 'major' },
+      packages: {
+        foo: { dependencies: { dep: '1.0.0' } },
+        bar: {},
+        dep: {},
+      },
+      packageGroups: { grp: { packageNames: ['foo', 'bar'], disallowedChangeTypes: null } },
+    });
+
+    expect(bumpInfo.calculatedChangeTypes).toEqual({
+      dep: 'major',
+      foo: 'minor',
+      bar: 'minor',
+    });
+  });
+
   // bumpInPlace sets calculatedChangeTypes for all packages in a group to the same type.
   // So the meaningful thing to test is that a bump of a dependency *outside* the group
   // propagates in to bump the group.
-  it('bumps all grouped packages, if a dependency was bumped', () => {
+  it('bumps all grouped packages, if a dependency was bumped and group had lower bump type', () => {
     const bumpInfo = callUpdateRelatedChangeType({
       packageGroups: { grp: { packageNames: ['foo', 'bar'], disallowedChangeTypes: null } },
       // Suppose there was some additional change file that already patch bumped the group packages
@@ -282,25 +293,24 @@ describe('updateRelatedChangeType', () => {
     });
   });
 
-  // currently fails
-  // it('respects disallowed change type from group', () => {
-  //   const bumpInfo = callUpdateRelatedChangeType({
-  //     changes: [{ packageName: 'dep', type: 'major', dependentChangeType: 'minor' }],
-  //     calculatedChangeTypes: { dep: 'major' },
-  //     packages: {
-  //       foo: { dependencies: { dep: '1.0.0' } },
-  //       bar: {},
-  //       dep: {},
-  //     },
-  //     packageGroups: { grp: { packageNames: ['foo', 'bar'], disallowedChangeTypes: ['major', 'minor'] } },
-  //   });
-  //
-  //   expect(bumpInfo.calculatedChangeTypes).toEqual({
-  //     dep: 'major',
-  //     foo: 'preminor',
-  //     bar: 'preminor',
-  //   });
-  // });
+  it('respects disallowed change type from group', () => {
+    const bumpInfo = callUpdateRelatedChangeType({
+      changes: [{ packageName: 'dep', type: 'major', dependentChangeType: 'minor' }],
+      calculatedChangeTypes: { dep: 'major' },
+      packages: {
+        foo: { dependencies: { dep: '1.0.0' } },
+        bar: {},
+        dep: {},
+      },
+      packageGroups: { grp: { packageNames: ['foo', 'bar'], disallowedChangeTypes: ['major', 'minor'] } },
+    });
+
+    expect(bumpInfo.calculatedChangeTypes).toEqual({
+      dep: 'major',
+      foo: 'preminor',
+      bar: 'preminor',
+    });
+  });
 
   it('does not bump any dependents when dependentChangeType is none', () => {
     const bumpInfo = callUpdateRelatedChangeType({
@@ -336,33 +346,20 @@ describe('updateRelatedChangeType', () => {
     });
   });
 
-  it('bumps package with devDependency on the changed package', () => {
+  it('bumps package with devDependency or peerDependency on the changed package', () => {
     const bumpInfo = callUpdateRelatedChangeType({
-      changes: [{ packageName: 'dep', type: 'patch', dependentChangeType: 'minor' }],
+      changes: [{ packageName: 'dep', type: 'minor', dependentChangeType: 'patch' }],
       packages: {
         dep: {},
         consumer: { devDependencies: { dep: '1.0.0' } },
+        peerConsumer: { peerDependencies: { dep: '1.0.0' } },
       },
     });
 
     expect(bumpInfo.calculatedChangeTypes).toEqual({
-      dep: 'patch',
-      consumer: 'minor',
-    });
-  });
-
-  it('bumps package with peerDependency on the changed package', () => {
-    const bumpInfo = callUpdateRelatedChangeType({
-      changes: [{ packageName: 'dep', type: 'patch', dependentChangeType: 'minor' }],
-      packages: {
-        dep: {},
-        consumer: { peerDependencies: { dep: '1.0.0' } },
-      },
-    });
-
-    expect(bumpInfo.calculatedChangeTypes).toEqual({
-      dep: 'patch',
-      consumer: 'minor',
+      dep: 'minor',
+      consumer: 'patch',
+      peerConsumer: 'patch',
     });
   });
 
@@ -477,4 +474,25 @@ describe('updateRelatedChangeType', () => {
       g2b: 'minor',
     });
   });
+
+  it('does not re-bump group members of the changed package via dependentChangeType', () => {
+    const bumpInfo = callUpdateRelatedChangeType({
+      changes: [{ packageName: 'foo', type: 'patch', dependentChangeType: 'minor' }],
+      calculatedChangeTypes: { foo: 'patch', bar: 'patch' },
+      packages: {
+        foo: {},
+        bar: {},
+        external: { dependencies: { foo: '1.0.0' } },
+      },
+      packageGroups: { grp: { packageNames: ['foo', 'bar'], disallowedChangeTypes: null } },
+    });
+
+    // 'bar' stays at 'patch' — the startKey check skips group expansion for the initial package.
+    // 'external' gets 'minor' because it depends on 'foo'.
+    expect(bumpInfo.calculatedChangeTypes).toEqual({
+      foo: 'patch',
+      bar: 'patch',
+      external: 'minor',
+    });
+  });
 });

packages/beachball/src/bump/bumpInMemory.ts

@@ -17,8 +17,8 @@ import { setDependentVersions } from './setDependentVersions';
 export function bumpInMemory(options: BeachballOptions, context: Omit<CommandContext, 'bumpInfo'>): BumpInfo {
   const { bumpDeps } = options;
 
-  // Pass 1: calculatedChangeTypes includes ONLY changes direct from the change files
-  // (no dependents, groups, or disallowedChangeTypes)
+  // Pass 1: calculatedChangeTypes includes ONLY changes direct from the change files (no dependents,
+  // groups, or disallowedChangeTypes). "none" changes are removed.
   const calculatedChangeTypes = initializePackageChangeTypes(context.changeSet);
 
   // (Splitting out a couple properties that aren't modified as initial step of reducing mutation approach)
@@ -37,31 +37,38 @@ export function bumpInMemory(options: BeachballOptions, context: Omit<CommandCon
   //       - set the version for all packages in the group in (bumpPackageInfoVersion())
   //       - the main concern is how to capture the bump reason in grouped changelog
 
-  // pass 2: initialize grouped calculatedChangeTypes together
-  for (const group of Object.values(bumpInfo.packageGroups)) {
+  // pass 2: initialize grouped calculatedChangeTypes together (and cache the package to group mapping)
+  const packageToGroup: Record<string, string> = {};
+  for (const [groupName, group] of Object.entries(bumpInfo.packageGroups)) {
     // If any of the group's packages have a change, find the max change type out of any package in the group.
+    // ("none" types were removed by initializePackageChangeTypes).
     const seenTypes = new Set<ChangeType>();
     for (const packageNameInGroup of group.packageNames) {
+      packageToGroup[packageNameInGroup] = groupName;
+
       const changeType = calculatedChangeTypes[packageNameInGroup];
       if (changeType) {
         seenTypes.add(changeType);
       }
     }
 
     if (seenTypes.size) {
-      // Set all packages in the group to the max change type.
+      // Set all packages in the group to the max valid type found, overwriting any previous type
+      // (validate() should have prevented an invalid group type at this step, but check to be safe)
       const maxChangeInGroup = getMaxChangeType([...seenTypes], group.disallowedChangeTypes);
       for (const packageNameInGroup of group.packageNames) {
         calculatedChangeTypes[packageNameInGroup] = maxChangeInGroup;
       }
     }
   }
 
-  // Pass 3: Calculate change types for dependents and groups.
+  // Pass 3: Calculate change types for dependents (propagating dependent bumps through groups).
   // TODO: fix weird behavior - https://github.com/microsoft/beachball/issues/620
-  const dependents = bumpDeps ? getDependentsForPackages(bumpInfo) : undefined;
-  for (const { change } of context.changeSet) {
-    updateRelatedChangeType({ change, bumpInfo, dependents, options });
+  if (bumpDeps) {
+    const dependents = getDependentsForPackages(bumpInfo);
+    for (const { change } of context.changeSet) {
+      updateRelatedChangeType({ change, bumpInfo, dependents, options, packageToGroup });
+    }
   }
 
   // pass 4: actually bump the packages in the bumpInfo in memory (no disk writes at this point)