[breaking] Fix behavior of VersionGroupOptions.exclude (see #916)

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": "[email protected]",
+  "dependentChangeType": "patch"
+}

docs/concepts/groups.md

@@ -28,12 +28,12 @@ For cases where it's necessary to bump packages together, `beachball` also provi
 
 Groups can be added to the [configuration file](../overview/configuration). See the [`VersionGroupOptions` source](https://github.com/microsoft/beachball/blob/master/src/types/ChangelogOptions.ts) for full details.
 
-| Name                    | Type                         | Description                                                                                                                                                                                                                        |
-| ----------------------- | ---------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| `name`                  | `string`                     | Name of the version group                                                                                                                                                                                                          |
-| `include`               | `string \| string[] \| true` | minimatch pattern(s) for package paths to include in this group. Patterns are relative to the repo root and must use forward slashes. If `true`, include all packages except those matching `exclude`.                             |
-| `exclude`               | `string \| string[]`         | minimatch pattern(s) for package paths to include in this group. Patterns are relative to the repo root and must use forward slashes. Currently this must use **negated patterns only** (will be fixed in the next major version). |
-| `disallowedChangeTypes` | `ChangeType[] \| null`       | Disallow these change types for the group.                                                                                                                                                                                         |
+| Name                    | Type                         | Description                                                                                                                                                                                            |
+| ----------------------- | ---------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
+| `name`                  | `string`                     | Name of the version group                                                                                                                                                                              |
+| `include`               | `string \| string[] \| true` | minimatch pattern(s) for package paths to include in this group. Patterns are relative to the repo root and must use forward slashes. If `true`, include all packages except those matching `exclude`. |
+| `exclude`               | `string \| string[]`         | minimatch pattern(s) for package paths to include in this group. Patterns are relative to the repo root and must use forward slashes.                                                                  |
+| `disallowedChangeTypes` | `ChangeType[] \| null`       | Disallow these change types for the group.                                                                                                                                                             |
 
 Example:
 
@@ -43,10 +43,10 @@ Example:
     {
       "name": "group name",
       "include": ["packages/groupfoo/*"],
-      "exclude": ["!packages/groupfoo/bar"],
-      "disallowedChangeTypes": ["major"]
-    }
-  ]
+      "exclude": ["packages/groupfoo/bar"],
+      "disallowedChangeTypes": ["major"],
+    },
+  ],
 }
 ```
 
@@ -58,12 +58,12 @@ If you only want to publish or record changes for certain packages, you should u
 
 To show changes for multiple packages in one change file, use the `changelog.groups` option. See the [`ChangelogGroupOptions` source](https://github.com/microsoft/beachball/blob/master/src/types/ChangelogOptions.ts) for full details.
 
-| Name                | Type                         | Description                                                                                                                                                                                                                          |
-| ------------------- | ---------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
-| `masterPackageName` | `string`                     | The main package which a group of changes bubbles up to.                                                                                                                                                                             |
-| `include`           | `string \| string[] \| true` | minimatch pattern(s) for package paths to include in this group. Patterns are relative to the repo root and must use forward slashes. If `true`, include all packages except those matching `exclude`.                               |
-| `exclude`           | `string \| string[]`         | minimatch pattern(s) for package paths to exclude from this group. Patterns are relative to the repo root and must use forward slashes. Currently this must use **negated patterns only** (will be fixed in the next major version). |
-| `changelogPath`     | `string`                     | Put the grouped changelog file under this directory. Can be relative to the root, or absolute.                                                                                                                                       |
+| Name                | Type                         | Description                                                                                                                                                                                            |
+| ------------------- | ---------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
+| `masterPackageName` | `string`                     | The main package which a group of changes bubbles up to.                                                                                                                                               |
+| `include`           | `string \| string[] \| true` | minimatch pattern(s) for package paths to include in this group. Patterns are relative to the repo root and must use forward slashes. If `true`, include all packages except those matching `exclude`. |
+| `exclude`           | `string \| string[]`         | minimatch pattern(s) for package paths to exclude from this group. Patterns are relative to the repo root and must use forward slashes.                                                                |
+| `changelogPath`     | `string`                     | Put the grouped changelog file under this directory. Can be relative to the root, or absolute.                                                                                                         |
 
 In this example, changelogs for all packages under `packages/*` (except `packages/baz`) are written to a `CHANGELOG.md` at the repo root (`.`), with `foo` as the master package. (To replace `foo`'s usual changelog with a grouped one, you'd specify `changelogPath` as the path to `foo` instead, e.g. `packages/foo`.)
 
@@ -75,7 +75,7 @@ In this example, changelogs for all packages under `packages/*` (except `package
         "masterPackageName": "foo",
         "changelogPath": ".",
         "include": ["packages/*"],
-        "exclude": ["!packages/baz"]
+        "exclude": ["packages/baz"]
       }
     ]
   }

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();
   });
 });

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]);
       }

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;

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,
+    });
   });
 }

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;

src/types/BeachballOptions.ts

@@ -225,9 +225,6 @@ export interface VersionGroupOptions {
   /**
    * minimatch pattern(s) for package paths to exclude from this group.
    * Patterns are relative to the repo root and must use forward slashes.
-   *
-   * 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.)
    */
   exclude?: string | string[];
 

src/types/ChangelogOptions.ts

@@ -75,9 +75,6 @@ export interface ChangelogGroupOptions {
   /**
    * minimatch pattern(s) for package paths to exclude from this group.
    * Patterns are relative to the repo root and must use forward slashes.
-   *
-   * 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.)
    */
   exclude?: string | string[];