Clarify some of the bumping logic (#1101)

Author: ecraig12345

change/beachball-89f85253-2fbd-47da-923c-d3aad1d5f836.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Clarify some of the bumping logic",
+  "packageName": "beachball",
+  "email": "elcraig@microsoft.com",
+  "dependentChangeType": "patch"
+}

src/__e2e__/bump.test.ts

@@ -37,7 +37,7 @@ describe('version bumping', () => {
     repo = undefined;
   });
 
-  it('bumps only packages with change files', async () => {
+  it('bumps only packages with change files with bumpDeps: false', async () => {
     const monorepo: RepoFixture['folders'] = {
       packages: {
         'pkg-1': { version: '1.0.0' },

src/__tests__/bump/setDependentVersions.test.ts

@@ -0,0 +1,175 @@
+import { describe, it, expect, afterEach, jest, beforeAll, afterAll } from '@jest/globals';
+import { setDependentVersions } from '../../bump/setDependentVersions';
+import { makePackageInfos, type PartialPackageInfos } from '../../__fixtures__/packageInfos';
+import { consideredDependencies } from '../../types/PackageInfo';
+
+type PartialBumpInfo = Parameters<typeof setDependentVersions>[0];
+
+describe('setDependentVersions', () => {
+  let consoleLogSpy: jest.SpiedFunction<typeof console.log>;
+
+  /**
+   * Make the bump info. Package versions should reflect any bumps applied.
+   *
+   * Unless otherwise specified, assumes all packages are in scope and have been modified.
+   * Realistically this would have been determined based on change types and `dependentChangeTypes`.
+   */
+  function makeBumpInfo(
+    params: { packageInfos: PartialPackageInfos } & Partial<Omit<PartialBumpInfo, 'packageInfos'>>
+  ): PartialBumpInfo {
+    const { packageInfos, ...rest } = params;
+    return {
+      packageInfos: makePackageInfos(packageInfos),
+      scopedPackages: new Set(Object.keys(packageInfos)),
+      modifiedPackages: new Set(Object.keys(packageInfos)),
+      ...rest,
+    };
+  }
+
+  beforeAll(() => {
+    consoleLogSpy = jest.spyOn(console, 'log').mockImplementation(() => undefined);
+    jest.spyOn(console, 'info').mockImplementation(() => undefined);
+  });
+
+  afterEach(() => {
+    jest.clearAllMocks();
+  });
+
+  afterAll(() => {
+    jest.restoreAllMocks();
+  });
+
+  it('returns empty object when no packages are in scope', () => {
+    const bumpInfo = makeBumpInfo({
+      packageInfos: { 'pkg-a': {} },
+      scopedPackages: new Set(),
+      // not sure if this would be included if out of scope, but check in case
+      modifiedPackages: new Set(['pkg-a']),
+    });
+
+    const result = setDependentVersions(bumpInfo, {});
+
+    expect(result).toEqual({});
+  });
+
+  it('returns empty object when no packages are modified', () => {
+    const bumpInfo = makeBumpInfo({
+      packageInfos: { 'pkg-a': {} },
+      modifiedPackages: new Set(),
+    });
+
+    const result = setDependentVersions(bumpInfo, {});
+
+    expect(result).toEqual({});
+  });
+
+  it('bumps dependency version range when dependency is bumped to unsatisfied range', () => {
+    const bumpInfo = makeBumpInfo({
+      packageInfos: {
+        // assume this had change type major
+        'pkg-a': { version: '2.0.0' },
+        // and this would be bumped for dependentChangeType patch
+        'pkg-b': { version: '1.0.1', dependencies: { 'pkg-a': '^1.0.0' } },
+      },
+    });
+
+    const result = setDependentVersions(bumpInfo, {});
+
+    expect(bumpInfo.packageInfos['pkg-b'].dependencies!['pkg-a']).toBe('^2.0.0');
+    expect(result).toEqual({ 'pkg-b': new Set(['pkg-a']) });
+  });
+
+  it('bumps dependency range even if already satisfied', () => {
+    const bumpInfo = makeBumpInfo({
+      packageInfos: {
+        // assume this had change type minor
+        'pkg-a': { version: '1.1.0' },
+        // and this would be bumped for dependentChangeType patch
+        'pkg-b': { version: '1.0.1', dependencies: { 'pkg-a': '^1.0.0' } },
+        'pkg-c': { version: '1.0.1', dependencies: { 'pkg-a': '~1.0.0' } },
+      },
+    });
+
+    const result = setDependentVersions(bumpInfo, {});
+
+    expect(bumpInfo.packageInfos['pkg-b'].dependencies!['pkg-a']).toBe('^1.1.0');
+    expect(bumpInfo.packageInfos['pkg-c'].dependencies!['pkg-a']).toBe('~1.1.0');
+    expect(result).toEqual({ 'pkg-b': new Set(['pkg-a']), 'pkg-c': new Set(['pkg-a']) });
+  });
+
+  it.each(consideredDependencies)('handles %s', depType => {
+    const bumpInfo = makeBumpInfo({
+      packageInfos: {
+        'pkg-a': { version: '1.1.0' },
+        'pkg-b': { version: '1.0.1', [depType]: { 'pkg-a': '^1.0.0' } },
+      },
+    });
+
+    const result = setDependentVersions(bumpInfo, {});
+
+    expect(bumpInfo.packageInfos['pkg-b'][depType]!['pkg-a']).toBe('^1.1.0');
+    expect(result).toEqual({ 'pkg-b': new Set(['pkg-a']) });
+
+    // doesn't log by default
+    expect(consoleLogSpy).not.toHaveBeenCalled();
+  });
+
+  it('handles (ignores) external dependencies', () => {
+    const bumpInfo = makeBumpInfo({
+      packageInfos: {
+        'pkg-a': { version: '1.0.0', dependencies: { external: '^1.0.0' } },
+      },
+    });
+
+    const result = setDependentVersions(bumpInfo, {});
+
+    expect(bumpInfo.packageInfos['pkg-a'].dependencies!['external']).toBe('^1.0.0');
+    expect(result).toEqual({});
+  });
+
+  it('handles multiple dependencies being bumped', () => {
+    const bumpInfo = makeBumpInfo({
+      packageInfos: {
+        'pkg-a': { version: '1.0.1' },
+        'pkg-b': { version: '1.1.0' },
+        'pkg-c': { version: '1.0.1', dependencies: { 'pkg-a': '^1.0.0' }, peerDependencies: { 'pkg-b': '^1.0.0' } },
+      },
+    });
+
+    const result = setDependentVersions(bumpInfo, {});
+
+    expect(bumpInfo.packageInfos['pkg-c'].dependencies!['pkg-a']).toBe('^1.0.1');
+    expect(bumpInfo.packageInfos['pkg-c'].peerDependencies!['pkg-b']).toBe('^1.1.0');
+    expect(result).toEqual({ 'pkg-c': new Set(['pkg-a', 'pkg-b']) });
+  });
+
+  it('logs when verbose is true', () => {
+    const bumpInfo = makeBumpInfo({
+      packageInfos: {
+        'pkg-a': { version: '2.0.0' },
+        'pkg-b': { version: '1.0.1', dependencies: { 'pkg-a': '^1.0.0' } },
+      },
+    });
+
+    setDependentVersions(bumpInfo, { verbose: true });
+
+    expect(consoleLogSpy).toHaveBeenCalledWith('pkg-b needs to be bumped because pkg-a ^1.0.0 -> ^2.0.0');
+  });
+
+  // Documenting this issue
+  // https://github.com/microsoft/beachball/issues/981
+  it('currently misses bumps of workspace: ranges', () => {
+    const bumpInfo = makeBumpInfo({
+      packageInfos: {
+        'pkg-a': { version: '1.1.0' },
+        // * means an exact dependency and should definitely cause a bump
+        'pkg-b': { version: '1.0.1', dependencies: { 'pkg-a': 'workspace:*' } },
+      },
+    });
+
+    const result = setDependentVersions(bumpInfo, {});
+
+    expect(bumpInfo.packageInfos['pkg-b'].dependencies!['pkg-a']).toBe('workspace:*');
+    expect(result).toEqual({}); // should have pkg-b depending on pkg-a
+  });
+});

src/__tests__/bump/updateRelatedChangeType.test.ts

@@ -4,6 +4,7 @@ import type { ChangeFileInfo, ChangeInfo, ChangeType } from '../../types/ChangeI
 import { PartialPackageInfos, makePackageInfos } from '../../__fixtures__/packageInfos';
 import { PackageGroups } from '../../types/PackageInfo';
 import { getDependentsForPackages } from '../../bump/getDependentsForPackages';
+import type { BeachballOptions } from '../../types/BeachballOptions';
 
 type RelatedChangeTypeParams = Omit<Parameters<typeof updateRelatedChangeType>[0], 'change'>;
 
@@ -12,22 +13,24 @@ describe('updateRelatedChangeType', () => {
    * Call `updateRelatedChangeType` once for each of `changes`.
    * Returns the updated bump info.
    */
-  function callUpdateRelatedChangeType(options: {
-    changes: Array<Pick<ChangeInfo, 'packageName' | 'type' | 'dependentChangeType'>>;
-    /**
-     * All the packages used in this fixture.
-     * Must include any dependencies (all versions are 1.0.0).
-     */
-    packages: PartialPackageInfos;
-    /**
-     * 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;
+  function callUpdateRelatedChangeType(
+    options: Partial<Pick<BeachballOptions, 'bumpDeps'>> & {
+      changes: Array<Pick<ChangeInfo, 'packageName' | 'type' | 'dependentChangeType'>>;
+      /**
+       * All the packages used in this fixture.
+       * Must include any dependencies (all versions are 1.0.0).
+       */
+      packages: PartialPackageInfos;
+      /**
+       * 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;
 
     if (packageGroups && !options.calculatedChangeTypes) {
       throw new Error('calculatedChangeTypes must be specified if packageGroups is used');
@@ -51,8 +54,9 @@ describe('updateRelatedChangeType', () => {
       },
       // 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: getDependentsForPackages({ packageInfos, scopedPackages: new Set(Object.keys(packageInfos)) }),
-      bumpDeps: true,
+      dependents: bumpDeps
+        ? getDependentsForPackages({ packageInfos, scopedPackages: new Set(Object.keys(packageInfos)) })
+        : undefined,
     };
 
     for (const change of changes) {
@@ -77,6 +81,19 @@ 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: [

src/bump/bumpInPlace.ts

@@ -14,10 +14,9 @@ import { ChangeType } from '../types/ChangeInfo';
  */
 export function bumpInPlace(bumpInfo: BumpInfo, options: BeachballOptions): void {
   const { bumpDeps } = options;
-  const { calculatedChangeTypes, changeFileChangeInfos, modifiedPackages } = bumpInfo;
-
-  // pass 1: figure out all the change types for all the packages taking into account the bumpDeps option and version groups
-  const dependents = bumpDeps ? getDependentsForPackages(bumpInfo) : {};
+  // Precondition (pass 1): calculatedChangeTypes includes ONLY changes direct from the change files
+  // (no dependents or groups)
+  const { calculatedChangeTypes, changeFileChangeInfos } = bumpInfo;
 
   // TODO: when we do "locked", or "lock step" versioning, we could simply skip setting grouped change types
   //       - set the version for all packages in the group in (bumpPackageInfoVersion())
@@ -43,17 +42,19 @@ export function bumpInPlace(bumpInfo: BumpInfo, options: BeachballOptions): void
     }
   }
 
-  // Calculate change types for packages and dependencies
+  // Pass 3: Calculate change types for dependents and groups.
+  // TODO: fix weird behavior - https://github.com/microsoft/beachball/issues/620
+  const dependents = bumpDeps ? getDependentsForPackages(bumpInfo) : undefined;
   for (const { change } of changeFileChangeInfos) {
-    updateRelatedChangeType({ change, bumpInfo, dependents, bumpDeps });
+    updateRelatedChangeType({ change, bumpInfo, dependents });
   }
 
   // pass 3: actually bump the packages in the bumpInfo in memory (no disk writes at this point)
-  Object.keys(calculatedChangeTypes).forEach(pkgName => {
+  for (const pkgName of Object.keys(calculatedChangeTypes)) {
     bumpPackageInfoVersion(pkgName, bumpInfo, options);
-  });
+  }
 
-  // step 4: Bump all the dependencies packages
+  // step 4: Bump all the dependency version ranges and collect dependentChangedBy for the changelog
+  // (also add any modifiedPackages not previously detected--this should only happen if bumpDeps was false)
   bumpInfo.dependentChangedBy = setDependentVersions(bumpInfo, options);
-  Object.keys(bumpInfo.dependentChangedBy).forEach(pkg => modifiedPackages.add(pkg));
 }

src/bump/setDependentVersions.ts

@@ -5,14 +5,19 @@ import { bumpMinSemverRange } from './bumpMinSemverRange';
 
 /**
  * Go through the deps of each package and bump the version range for in-repo deps if needed.
+ * Prior to calling this, it's expected that:
+ * - package versions in `bumpInfo.packageInfos` have been bumped per change files and dependentChangeTypes
+ * - `bumpInfo.modifiedPackages` contains all packages whose version has been bumped
  *
- * **This mutates dep versions in `packageInfos`** as well as returning `dependentChangedBy`.
+ * **This mutates dependency versions in `packageInfos`** and might add to `bumpInfo.modifiedPackages`.
+ * Probably the only case where it will change `modifiedPackages` is if `BeachballOptions.bumpDeps` is false
+ * (or if this is being called by `sync` which didn't previously bump dependents).
  */
 export function setDependentVersions(
-  bumpInfo: Pick<BumpInfo, 'packageInfos' | 'scopedPackages'>,
+  bumpInfo: Pick<BumpInfo, 'packageInfos' | 'scopedPackages' | 'modifiedPackages'>,
   options: Pick<BeachballOptions, 'verbose'>
 ): BumpInfo['dependentChangedBy'] {
-  const { packageInfos, scopedPackages } = bumpInfo;
+  const { packageInfos, scopedPackages, modifiedPackages } = bumpInfo;
   const { verbose } = options;
   const dependentChangedBy: BumpInfo['dependentChangedBy'] = {};
 
@@ -26,8 +31,11 @@ export function setDependentVersions(
 
       for (const [dep, existingVersionRange] of Object.entries(deps)) {
         const depPackage = packageInfos[dep];
-        if (!depPackage) {
-          continue; // external dependency
+        // TODO: should this use the initial modifiedPackages rather than the possibly-updated one?
+        // (considering updates could introduce order sensitivity, though the old logic that didn't
+        // check modifiedPackages at all also had that issue)
+        if (!depPackage || !modifiedPackages.has(dep)) {
+          continue; // external dependency or not modified
         }
 
         const bumpedVersionRange = bumpMinSemverRange(depPackage.version, existingVersionRange);
@@ -38,6 +46,12 @@ export function setDependentVersions(
 
           dependentChangedBy[pkgName] ??= new Set<string>();
           dependentChangedBy[pkgName].add(dep);
+
+          // Unless bumpDeps was false, the package should have been added to modifiedPackages
+          // by updateRelatedChangeType plus bumpPackageInfoVersion, but to be safe we add it here too.
+          // TODO: fix behavior - https://github.com/microsoft/beachball/issues/620
+          modifiedPackages.add(pkgName);
+
           if (verbose) {
             console.log(
               `${pkgName} needs to be bumped because ${dep} ${existingVersionRange} -> ${bumpedVersionRange}`