From 05966c3cc24b62b831dbd8532e7c277fcb5139eb Mon Sep 17 00:00:00 2001 From: Elizabeth Craig Date: Sun, 24 Nov 2024 07:10:52 -0800 Subject: [PATCH] [breaking] Fix behavior of VersionGroupOptions.exclude (see #916) --- ...-9c18f781-7c73-4b14-adb7-a5fc6930628a.json | 7 +++++ src/__tests__/monorepo/isPathIncluded.test.ts | 31 ++++++++++++------- src/changelog/writeChangelog.ts | 2 +- src/monorepo/getPackageGroups.ts | 4 ++- src/monorepo/getScopedPackages.ts | 8 +++-- src/monorepo/isPathIncluded.ts | 20 +++++------- src/types/BeachballOptions.ts | 8 ++--- src/types/ChangelogOptions.ts | 8 ++--- 8 files changed, 51 insertions(+), 37 deletions(-) create mode 100644 change/beachball-9c18f781-7c73-4b14-adb7-a5fc6930628a.json diff --git a/change/beachball-9c18f781-7c73-4b14-adb7-a5fc6930628a.json b/change/beachball-9c18f781-7c73-4b14-adb7-a5fc6930628a.json new file mode 100644 index 000000000..268e150b4 --- /dev/null +++ b/change/beachball-9c18f781-7c73-4b14-adb7-a5fc6930628a.json @@ -0,0 +1,7 @@ +{ + "type": "major", + "comment": "[BREAKING] Fix behavior of `VersionGroupOptions.exclude`: if a package path **matches** any `exclude` pattern, it's excluded, rather than requiring negation. (To migrate, just remove the leading `!` from your `exclude` patterns.)", + "packageName": "beachball", + "email": "elcraig@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/src/__tests__/monorepo/isPathIncluded.test.ts b/src/__tests__/monorepo/isPathIncluded.test.ts index 4dd37fba4..1e504644e 100644 --- a/src/__tests__/monorepo/isPathIncluded.test.ts +++ b/src/__tests__/monorepo/isPathIncluded.test.ts @@ -3,34 +3,43 @@ import { isPathIncluded } from '../../monorepo/isPathIncluded'; describe('isPathIncluded', () => { it('returns true if path is included (single include path)', () => { - expect(isPathIncluded('packages/a', 'packages/*')).toBeTruthy(); + expect(isPathIncluded({ relativePath: 'packages/a', include: 'packages/*' })).toBeTruthy(); }); - it('returns false if path is excluded (single exclude path)', () => { - expect(isPathIncluded('packages/a', 'packages/*', '!packages/a')).toBeFalsy(); + it('returns false if path is not included, with single include path', () => { + expect(isPathIncluded({ relativePath: 'stuff/b', include: 'packages/*' })).toBeFalsy(); + expect(isPathIncluded({ relativePath: 'packages/b', include: 'packages/!(b)' })).toBeFalsy(); }); - it('returns true if path is included (multiple include paths)', () => { - expect(isPathIncluded('packages/a', ['packages/b', 'packages/a'], ['!packages/b'])).toBeTruthy(); + it('returns false if path is excluded, with single exclude path', () => { + expect(isPathIncluded({ relativePath: 'packages/a', include: 'packages/*', exclude: 'packages/a' })).toBeFalsy(); }); - it('returns false if path is excluded (multiple exclude paths)', () => { - expect(isPathIncluded('packages/a', ['packages/*'], ['!packages/a', '!packages/b'])).toBeFalsy(); + it('returns true if path is included, with multiple include paths', () => { + expect( + isPathIncluded({ relativePath: 'packages/a', include: ['packages/b', 'packages/a'], exclude: ['packages/b'] }) + ).toBeTruthy(); + }); + + it('returns false if path is excluded, with multiple exclude paths', () => { + expect( + isPathIncluded({ relativePath: 'packages/a', include: ['packages/*'], exclude: ['packages/a'] }) + ).toBeFalsy(); }); it('returns true if include is true (no exclude paths)', () => { - expect(isPathIncluded('packages/a', true)).toBeTruthy(); + expect(isPathIncluded({ relativePath: 'packages/a', include: true })).toBeTruthy(); }); it('returns false if include is true and path is excluded', () => { - expect(isPathIncluded('packages/a', true, '!packages/a')).toBeFalsy(); + expect(isPathIncluded({ relativePath: 'packages/a', include: true, exclude: 'packages/a' })).toBeFalsy(); }); it('returns false if include path is empty', () => { - expect(isPathIncluded('packages/a', '')).toBeFalsy(); + expect(isPathIncluded({ relativePath: 'packages/a', include: '' })).toBeFalsy(); }); it('ignores empty exclude path array', () => { - expect(isPathIncluded('packages/a', 'packages/*', [])).toBeTruthy(); + expect(isPathIncluded({ relativePath: 'packages/a', include: 'packages/*', exclude: [] })).toBeTruthy(); }); }); diff --git a/src/changelog/writeChangelog.ts b/src/changelog/writeChangelog.ts index 907fbd161..f538c1365 100644 --- a/src/changelog/writeChangelog.ts +++ b/src/changelog/writeChangelog.ts @@ -85,7 +85,7 @@ async function writeGroupedChangelog( const relativePath = path.relative(options.path, packagePath); for (const group of changelogGroups) { - const isInGroup = isPathIncluded(relativePath, group.include, group.exclude); + const isInGroup = isPathIncluded({ relativePath, include: group.include, exclude: group.exclude }); if (isInGroup) { groupedChangelogs[group.changelogAbsDir].changelogs.push(changelogs[pkg]); } diff --git a/src/monorepo/getPackageGroups.ts b/src/monorepo/getPackageGroups.ts index 149f92466..802a4aed8 100644 --- a/src/monorepo/getPackageGroups.ts +++ b/src/monorepo/getPackageGroups.ts @@ -23,7 +23,9 @@ export function getPackageGroups( const packagePath = path.dirname(info.packageJsonPath); const relativePath = path.relative(root, packagePath); - const groupsForPkg = groups.filter(group => isPathIncluded(relativePath, group.include, group.exclude)); + const groupsForPkg = groups.filter(group => + isPathIncluded({ relativePath, include: group.include, exclude: group.exclude }) + ); if (groupsForPkg.length > 1) { // Keep going after this error to ensure we report all errors errorPackages[pkgName] = groupsForPkg; diff --git a/src/monorepo/getScopedPackages.ts b/src/monorepo/getScopedPackages.ts index e59dbbda2..878fbf262 100644 --- a/src/monorepo/getScopedPackages.ts +++ b/src/monorepo/getScopedPackages.ts @@ -16,11 +16,15 @@ export function getScopedPackages( // If there were no include scopes, include all paths by default includeScopes = includeScopes.length ? includeScopes : true; - const excludeScopes = scope.filter(s => s.startsWith('!')); + const excludeScopes = scope.filter(s => s.startsWith('!')).map(s => s.slice(1)); return Object.keys(packageInfos).filter(pkgName => { const packagePath = path.dirname(packageInfos[pkgName].packageJsonPath); - return isPathIncluded(path.relative(cwd, packagePath), includeScopes, excludeScopes); + return isPathIncluded({ + relativePath: path.relative(cwd, packagePath), + include: includeScopes, + exclude: excludeScopes, + }); }); } diff --git a/src/monorepo/isPathIncluded.ts b/src/monorepo/isPathIncluded.ts index c7e5a7c67..49719fdb1 100644 --- a/src/monorepo/isPathIncluded.ts +++ b/src/monorepo/isPathIncluded.ts @@ -8,11 +8,13 @@ import minimatch from 'minimatch'; * e.g. if you want to exclude `packages/foo`, you must specify `exclude` as `!packages/foo`. * (This will be fixed in a future major version.) */ -export function isPathIncluded( - relativePath: string, - include: string | string[] | true, - exclude?: string | string[] -): boolean { +export function isPathIncluded(params: { + relativePath: string; + include: string | string[] | true; + exclude?: string | string[]; +}): boolean { + const { relativePath, include, exclude } = params; + let shouldInclude: boolean; if (include === true) { shouldInclude = true; @@ -22,14 +24,8 @@ export function isPathIncluded( } if (exclude?.length && shouldInclude) { - // TODO: this is weird/buggy--it assumes that exclude patterns are always negated, - // which intuitively (or comparing to other tools) is not how it should work. - // If this is fixed, updates will be needed in: - // - getScopedPackages() - // - ChangelogGroupOptions - // - VersionGroupOptions const excludePatterns = typeof exclude === 'string' ? [exclude] : exclude; - shouldInclude = excludePatterns.every(pattern => minimatch(relativePath, pattern)); + shouldInclude = !excludePatterns.some(pattern => minimatch(relativePath, pattern)); } return shouldInclude; diff --git a/src/types/BeachballOptions.ts b/src/types/BeachballOptions.ts index 9dd4dd6e8..cbb539e51 100644 --- a/src/types/BeachballOptions.ts +++ b/src/types/BeachballOptions.ts @@ -196,15 +196,13 @@ export interface PackageOptions { */ export interface VersionGroupOptions { /** - * minimatch pattern (or array of minimatch) to detect which packages should be included in this group. - * If `true`, include all packages except those excluded by `exclude`. + * minimatch pattern (or array of minimatch) for packages names to include in this group. + * If `true`, include all packages except those matching `exclude`. */ include: string | string[] | true; /** - * minimatch pattern (or array of minimatch) to detect which packages should be excluded in this group. - * Currently this must use **negated patterns only**: e.g. if you want to exclude `packages/foo`, - * you must specify `exclude` as `!packages/foo`. (This will be fixed in a future major version.) + * minimatch pattern (or array of minimatch) for packages names to exclude from this group. */ exclude?: string | string[]; diff --git a/src/types/ChangelogOptions.ts b/src/types/ChangelogOptions.ts index 3ad07f22e..3735703ce 100644 --- a/src/types/ChangelogOptions.ts +++ b/src/types/ChangelogOptions.ts @@ -66,15 +66,13 @@ export interface ChangelogGroupOptions { masterPackageName: string; /** - * minimatch pattern (or array of minimatch) to detect which packages should be included in this group. - * If `true`, include all packages except those excluded by `exclude`. + * minimatch pattern (or array of minimatch) for packages names to include in this group. + * If `true`, include all packages except those matching `exclude`. */ include: string | string[] | true; /** - * minimatch pattern (or array of minimatch) to detect which packages should be excluded in this group. - * Currently this must use **negated patterns only**: e.g. if you want to exclude `packages/foo`, - * you must specify `exclude` as `!packages/foo`. (This will be fixed in a future major version.) + * minimatch pattern (or array of minimatch) for packages names to exclude from this group. */ exclude?: string | string[];