Simplify scopedPackages short circuit logic

change/beachball-14f0094f-a017-4b4e-856e-726bd0288542.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Simplify internal handling of determining in-scope packages",
+  "packageName": "beachball",
+  "email": "elcraig@microsoft.com",
+  "dependentChangeType": "patch"
+}

src/__functional__/monorepo/getScopedPackages.test.ts

@@ -1,3 +1,4 @@
+/* eslint-disable @typescript-eslint/unbound-method */
 import { describe, expect, it, beforeAll, afterAll } from '@jest/globals';
 import { getScopedPackages } from '../../monorepo/getScopedPackages';
 import type { PackageInfos } from '../../types/PackageInfo';
@@ -18,10 +19,33 @@ describe('getScopedPackages', () => {
     removeTempDir(root);
   });
 
-  it('returns true when no scope is provided', () => {
+  it('short circuits when no scope is provided', () => {
     const scopedPackages = getScopedPackages({ path: root }, packageInfos);
     expect(scopedPackages).toEqual(new Set(Object.keys(packageInfos)));
-    expect(scopedPackages.allInScope).toBe(true);
+
+    // If all are in scope, the returned Set's `has` method is overridden to short circuit.
+    expect(scopedPackages.has).not.toBe(Set.prototype.has);
+    expect(scopedPackages.has('foo')).toBe(true);
+    // But it still returns false if the package doesn't exist
+    expect(scopedPackages.has('nonexistent')).toBe(false);
+  });
+
+  it('short circuits if all in scope', () => {
+    const scopedPackages = getScopedPackages(
+      {
+        path: root,
+        scope: ['packages/**/*'],
+      },
+      packageInfos
+    );
+
+    expect(scopedPackages).toEqual(new Set(Object.keys(packageInfos)));
+
+    // If all are in scope, the returned Set's `has` method is overridden to short circuit.
+    expect(scopedPackages.has).not.toBe(Set.prototype.has);
+    expect(scopedPackages.has('foo')).toBe(true);
+    // But it still returns false if the package doesn't exist
+    expect(scopedPackages.has('nonexistent')).toBe(false);
   });
 
   it('can scope packages', () => {
@@ -34,7 +58,7 @@ describe('getScopedPackages', () => {
     );
 
     expect(scopedPackages).toEqual(new Set(['a', 'b']));
-    expect(scopedPackages.allInScope).toBeUndefined();
+    expect(scopedPackages.has).toBe(Set.prototype.has);
   });
 
   it('can scope with excluded packages', () => {
@@ -47,7 +71,7 @@ describe('getScopedPackages', () => {
     );
 
     expect(scopedPackages).toEqual(new Set(['bar', 'baz', 'foo']));
-    expect(scopedPackages.allInScope).toBeUndefined();
+    expect(scopedPackages.has).toBe(Set.prototype.has);
   });
 
   it('can mix and match with excluded packages', () => {
@@ -60,6 +84,6 @@ describe('getScopedPackages', () => {
     );
 
     expect(scopedPackages).toEqual(new Set(['bar', 'baz']));
-    expect(scopedPackages.allInScope).toBeUndefined();
+    expect(scopedPackages.has).toBe(Set.prototype.has);
   });
 });

src/__tests__/object/cloneObject.test.ts

@@ -1,8 +1,5 @@
 import { describe, it, expect } from '@jest/globals';
-import type { BumpInfo } from '../../types/BumpInfo';
 import { cloneObject } from '../../object/cloneObject';
-import { makePackageInfos } from '../../__fixtures__/packageInfos';
-import { getChange } from '../../__fixtures__/changeFiles';
 
 describe('cloneObject', () => {
   it.each<[string, object | unknown[]]>([
@@ -12,8 +9,6 @@ describe('cloneObject', () => {
     ['object with null prototype', Object.assign(Object.create(null), { a: 1, b: '2', c: true })],
     ['empty array', []],
     ['array', [1, '2', true]],
-    ['set', new Set([1, 2, 3])],
-    ['object of sets', { a: new Set([1, 2, 3]), b: new Set(['a', 'b', 'c']) }],
   ])('clones %s', (desc, val) => {
     const cloned = cloneObject(val);
     expect(cloned).toEqual(val);
@@ -44,35 +39,6 @@ describe('cloneObject', () => {
     expect(() => cloneObject(new Map())).toThrow('Unsupported object type found while cloning bump info: Map');
     class Foo {}
     expect(() => cloneObject(new Foo())).toThrow('Unsupported object type found while cloning bump info: Foo');
-  });
-
-  it('clones bump info structure', () => {
-    const original: BumpInfo = {
-      // There's no attempt at consistency because it doesn't matter here
-      calculatedChangeTypes: { pkgA: 'minor', pkgB: 'patch' },
-      packageInfos: makePackageInfos({ a: { dependencies: { b: '^1.0.0' } }, b: {} }),
-      changeFileChangeInfos: [
-        { change: getChange('a'), changeFile: '' },
-        { change: getChange('b'), changeFile: '' },
-      ],
-      packageGroups: { group1: { packageNames: ['a', 'b'], disallowedChangeTypes: null } },
-      dependentChangedBy: { a: new Set(['b']) },
-      modifiedPackages: new Set(['a']),
-      scopedPackages: new Set(['a', 'b']),
-    };
-
-    const cloned = cloneObject(original);
-    expect(cloned).toEqual(original);
-    expect(cloned).not.toBe(original);
-    expect(cloned.packageInfos).not.toBe(original.packageInfos);
-    expect(cloned.packageInfos.a).not.toBe(original.packageInfos.a);
-    expect(cloned.changeFileChangeInfos).not.toBe(original.changeFileChangeInfos);
-    expect(cloned.changeFileChangeInfos[0]).not.toBe(original.changeFileChangeInfos[0]);
-    expect(cloned.changeFileChangeInfos[0].change).not.toBe(original.changeFileChangeInfos[0].change);
-    expect(cloned.packageGroups).not.toBe(original.packageGroups);
-    expect(cloned.dependentChangedBy).not.toBe(original.dependentChangedBy);
-    expect(cloned.dependentChangedBy.a).not.toBe(original.dependentChangedBy.a);
-    expect(cloned.modifiedPackages).not.toBe(original.modifiedPackages);
-    expect(cloned.scopedPackages).not.toBe(original.scopedPackages);
+    expect(() => cloneObject(new Set())).toThrow('Unsupported object type found while cloning bump info: Set');
   });
 });

src/bump/setDependentVersions.ts

@@ -20,7 +20,7 @@ export function setDependentVersions(
   const dependentChangedBy: BumpInfo['dependentChangedBy'] = {};
 
   for (const [pkgName, info] of Object.entries(packageInfos)) {
-    if (!scopedPackages.allInScope && !scopedPackages.has(pkgName)) {
+    if (!scopedPackages.has(pkgName)) {
       continue; // out of scope
     }
 

src/changefile/getChangedPackages.ts

@@ -48,7 +48,7 @@ function isPackageIncluded(
     : // This is a package-only option (can't be set at repo level or via CLI)
     packageInfo.packageOptions?.shouldPublish === false
     ? `${packageInfo.name} has beachball.shouldPublish=false`
-    : !scopedPackages.allInScope && !scopedPackages.has(packageInfo.name)
+    : !scopedPackages.has(packageInfo.name)
     ? `${packageInfo.name} is out of scope`
     : ''; // not ignored
 

src/changefile/readChangeFiles.ts

@@ -103,7 +103,7 @@ export function readChangeFiles(
       }
 
       // Add the change to the final list if it's valid and in scope
-      if (!warningType && (scopedPackages.allInScope || scopedPackages.has(change.packageName))) {
+      if (!warningType && scopedPackages.has(change.packageName)) {
         changeSet.push({ changeFile, change });
       }
     }

src/commands/sync.ts

@@ -21,11 +21,7 @@ export async function sync(options: BeachballOptions, context?: SyncCommandConte
   const packageInfos = context?.originalPackageInfos ?? getPackageInfos(options.path);
   const scopedPackages = context?.scopedPackages ?? getScopedPackages(options, packageInfos);
 
-  const infos = new Map(
-    Object.entries(packageInfos).filter(
-      ([pkg, info]) => !info.private && (scopedPackages.allInScope || scopedPackages.has(pkg))
-    )
-  );
+  const infos = new Map(Object.entries(packageInfos).filter(([pkg, info]) => !info.private && scopedPackages.has(pkg)));
 
   console.log(`Getting versions from registry for ${infos.size} package(s)...`);
 

src/monorepo/getScopedPackages.ts

@@ -11,22 +11,32 @@ export function getScopedPackages(
   packageInfos: PackageInfos
 ): ScopedPackages {
   const { scope, path: cwd } = options;
-  if (!scope) {
-    const result: ScopedPackages = new Set(Object.keys(packageInfos));
-    result.allInScope = true;
-    return result;
-  }
 
-  let includeScopes: string[] | true = scope.filter(s => !s.startsWith('!'));
-  // If there were no include scopes, include all paths by default
-  includeScopes = includeScopes.length ? includeScopes : true;
+  const packageNames = Object.keys(packageInfos);
+  let result: Set<string>;
+
+  if (scope) {
+    let includeScopes: string[] | true = scope.filter(s => !s.startsWith('!'));
+    // 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('!'));
+    result = new Set(
+      packageNames.filter(pkgName => {
+        const packagePath = path.dirname(packageInfos[pkgName].packageJsonPath);
 
-  const result = Object.keys(packageInfos).filter(pkgName => {
-    const packagePath = path.dirname(packageInfos[pkgName].packageJsonPath);
+        return isPathIncluded(path.relative(cwd, packagePath), includeScopes, excludeScopes);
+      })
+    );
+  } else {
+    result = new Set(packageNames);
+  }
+
+  if (result.size === packageNames.length) {
+    // Override .has() to always return true unless the package doesn't exist
+    result.has = packageName => !!packageInfos[packageName];
+  }
 
-    return isPathIncluded(path.relative(cwd, packagePath), includeScopes, excludeScopes);
-  });
-  return new Set(result);
+  return result;
 }

src/object/cloneObject.ts

@@ -1,8 +1,6 @@
-import type { ScopedPackages } from '../types/PackageInfo';
-
 /**
- * Clone an object, fast.
- * Currently only handles data types expected in `BumpInfo` but could be expanded if needed.
+ * Clone an object, fast. Currently only handles JSON-like objects (not class instances)
+ * but could be expanded if needed.
  *
  * This is decently faster than `structuredClone` or `JSON.parse(JSON.stringify())` on a
  * very large object (bump info can be huge in certain repos). https://jsperf.app/rugosa/5
@@ -29,18 +27,6 @@ export function cloneObject<T extends object>(obj: T): T {
     return clone;
   }
 
-  if (obj instanceof Set) {
-    const cloned = new Set(
-      // eslint-disable-next-line @typescript-eslint/no-unsafe-return
-      Array.from(obj).map(item => (item && typeof item === 'object' ? cloneObject(item) : item))
-    ) as T;
-    // special logic to clone a custom set property
-    if ((obj as ScopedPackages).allInScope) {
-      (cloned as ScopedPackages).allInScope = true;
-    }
-    return cloned;
-  }
-
   if (obj.constructor?.name && obj.constructor.name !== 'Object') {
     throw new Error(`Unsupported object type found while cloning bump info: ${obj.constructor.name}`);
   }

src/publish/getPackagesToPublish.ts

@@ -37,7 +37,7 @@ export function getPackagesToPublish(
       skipReason = 'has change type none';
     } else if (packageInfo.private) {
       skipReason = 'is private';
-    } else if (!scopedPackages.allInScope && !scopedPackages.has(pkg)) {
+    } else if (!scopedPackages.has(pkg)) {
       skipReason = 'is out-of-scope';
     } else if (!changeType && !newPackages?.includes(pkg)) {
       skipReason = 'is not bumped (no calculated change type)';

src/types/PackageInfo.ts

@@ -95,10 +95,7 @@ export const consideredDependencies = [
 ] as const;
 
 /**
- * In-scope package names, with an extra property if all packages are in scope.
+ * In-scope package names. If returned by `getScopedPackages`, this has extra logic to return true
+ * without a full lookup when all packages are in scope. (A plain `Set<string>` works for tests.)
  */
-// This is a Set with an extra property to avoid compatibility issues with code using private APIs
-export type ScopedPackages = ReadonlySet<string> & {
-  /** No `scope` option was specified, so all packages are in scope. */
-  allInScope?: true;
-};
+export type ScopedPackages = ReadonlySet<string>;