diff --git a/change/change-e5e83f54-1a36-4545-9dcc-ee8af7201e1e.json b/change/change-e5e83f54-1a36-4545-9dcc-ee8af7201e1e.json new file mode 100644 index 000000000..693a0e434 --- /dev/null +++ b/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" + } + ] +} \ No newline at end of file diff --git a/packages/beachball/src/__tests__/bump/updateRelatedChangeType.test.ts b/packages/beachball/src/__tests__/bump/updateRelatedChangeType.test.ts index 9b0d50ae7..fc4f51759 100644 --- a/packages/beachball/src/__tests__/bump/updateRelatedChangeType.test.ts +++ b/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> & { - changes: Array>; - /** - * 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; - /** - * 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>; + /** + * 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; + /** + * 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 = {}; + 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', + }); + }); }); diff --git a/packages/beachball/src/bump/bumpInMemory.ts b/packages/beachball/src/bump/bumpInMemory.ts index c912b597d..1d7f95704 100644 --- a/packages/beachball/src/bump/bumpInMemory.ts +++ b/packages/beachball/src/bump/bumpInMemory.ts @@ -17,8 +17,8 @@ import { setDependentVersions } from './setDependentVersions'; export function bumpInMemory(options: BeachballOptions, context: Omit): 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,11 +37,15 @@ export function bumpInMemory(options: BeachballOptions, context: Omit = {}; + 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(); for (const packageNameInGroup of group.packageNames) { + packageToGroup[packageNameInGroup] = groupName; + const changeType = calculatedChangeTypes[packageNameInGroup]; if (changeType) { seenTypes.add(changeType); @@ -49,7 +53,8 @@ export function bumpInMemory(options: BeachballOptions, context: Omit; - dependents: PackageDependents | undefined; + dependents: PackageDependents; options: Pick; + /** Cached package name to group name mapping for quick lookup */ + packageToGroup: Record; }): void { - const { change, bumpInfo, dependents } = params; + const { change, bumpInfo, dependents, packageToGroup } = params; const { calculatedChangeTypes, packageGroups, packageInfos } = bumpInfo; // If dependentChangeType is none (or somehow unset), there's nothing to do. - const dependentChangeType = getMaxChangeType([change.dependentChangeType]); - if (dependentChangeType === 'none') { + if (getMaxChangeType([change.dependentChangeType]) === 'none') { return; } - // Enqueue the first package. - // This part of the bump algorithm is a performance bottleneck, so it's important to bail early + // Enqueue the first package. (dependentChangeType in the queue is usually the same as from the + // change file, but for packages in a group with disallowedChangeTypes, it might have been downgraded.) + // + // WARNING: This part of the bump algorithm is a performance bottleneck, so it's important to bail early // whenever possible, and to use `seen` to reduce queue insertion. // https://github.com/microsoft/beachball/pull/1042 - const queue = [change.packageName]; - const seen = new Set(); + type SeenKey = `${string}#${ChangeType}`; + const seen = new Set(); + const seenGroups = new Set(); + const queue: Array<{ subjectPackage: string; dependentChangeType: ChangeType }> = [ + { subjectPackage: change.packageName, dependentChangeType: change.dependentChangeType }, + ]; + const startKey: SeenKey = `${change.packageName}#${change.dependentChangeType}`; while (queue.length > 0) { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - const subjectPackage = queue.shift()!; + const { subjectPackage, dependentChangeType } = queue.shift()!; + const queueKey: SeenKey = `${subjectPackage}#${dependentChangeType}`; // Step 1. Update change type of the subjectPackage according to dependentChangeType if needed. - // (Skip for the initial package.) - if (subjectPackage !== change.packageName) { + // Also update members of its group if applicable. + // (Skip the initial package since dependentChangeType doesn't apply there.) + if (queueKey !== startKey) { + const groupName = packageToGroup[subjectPackage]; + const group = groupName ? packageGroups[groupName] : undefined; + const oldType = calculatedChangeTypes[subjectPackage]; - calculatedChangeTypes[subjectPackage] = getMaxChangeType( + const newType = (calculatedChangeTypes[subjectPackage] = getMaxChangeType( [oldType, dependentChangeType], - getPackageOption('disallowedChangeTypes', packageInfos[subjectPackage], params.options) - ); + // Group disallowedChangeTypes take precedence (actually validation verifies both can't be set) + // TODO: it's probably better to just save the disallowedChangeTypes with each grouped package during setup? + group?.disallowedChangeTypes === null + ? null + : group?.disallowedChangeTypes || + getPackageOption('disallowedChangeTypes', packageInfos[subjectPackage], params.options) + )); - // TODO: what's the interaction with groups here? - if (calculatedChangeTypes[subjectPackage] === oldType) { + if (newType === oldType) { // We didn't change this type, so keep going. continue; } - } - // Step 2. For all dependent packages of the current subjectPackage, place in queue to be updated at least to the "updatedChangeType" - // (dependents will be undefined if bumpDeps was false) - const dependentPackages = dependents?.[subjectPackage]; - - if (dependentPackages?.length) { - for (const dependentPackage of dependentPackages) { - if (seen.has(dependentPackage)) { - continue; + // If this package is in a group, enqueue other packages in the group. + // (Use an extra set tracking groups to avoid iterating over all member packages.) + const groupKey = groupName ? (`${groupName}#${dependentChangeType}` as const) : undefined; + if (group && groupKey && !seenGroups.has(groupKey)) { + seenGroups.add(groupKey); + for (const packageNameInGroup of group.packageNames) { + const key: SeenKey = `${packageNameInGroup}#${dependentChangeType}`; + if (!seen.has(key)) { + seen.add(key); + queue.push({ subjectPackage: packageNameInGroup, dependentChangeType: newType }); + } } - - seen.add(dependentPackage); - queue.push(dependentPackage); } } - // 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) - // TODO: ensure this is consistent with other group handling, and whether it's necessary at all if bumpDeps is false - const group = Object.values(packageGroups).find(g => g.packageNames.includes(subjectPackage)); - - if (group && !group.disallowedChangeTypes?.includes(dependentChangeType)) { - for (const packageNameInGroup of group.packageNames) { - if (!seen.has(packageNameInGroup)) { - seen.add(packageNameInGroup); - queue.push(packageNameInGroup); - } + // Step 2. For all dependent packages of the current subjectPackage, place in queue to be + // updated at least to the dependentChangeType + for (const dependentPackage of dependents[subjectPackage] || []) { + const key: SeenKey = `${dependentPackage}#${dependentChangeType}`; + if (!seen.has(key)) { + seen.add(key); + queue.push({ subjectPackage: dependentPackage, dependentChangeType }); } } }