Implement "layer" pack style 🎂 (#1158)

Author: ecraig12345

change/beachball-4c2b57d3-cd0b-40d7-8dbb-6a583c645b38.json

@@ -0,0 +1,7 @@
+{
+  "type": "minor",
+  "comment": "Implement \"layer\" pack style. Also handle all topological package sorting internally, and update the logic to ignore devDependencies since they can't cause breakages.",
+  "packageName": "beachball",
+  "email": "elcraig@microsoft.com",
+  "dependentChangeType": "patch"
+}

docs/overview/configuration.md

@@ -94,6 +94,8 @@ For the latest full list of supported options, see `RepoOptions` [in this file](
 | `npmReadConcurrency` | number | 5 | repo | Maximum concurrency for fetching package versions from the registry (see `concurrency` for write operations) |
 | `package` | `string` | | repo | Specifies which package the command relates to (overrides change detection based on `git diff`) |
 | `prereleasePrefix` | `string` | | repo | Prerelease prefix, e.g. `"beta"`. Note that if this is specified, packages with change type major/minor/patch will be bumped as prerelease instead. |
+| `packStyle` | `'sequential' \| 'layer'` | `'sequential'` | repo | With `packToPath`, how to organize the tgz files. `'sequential'` uses numeric prefixes to ensure topological ordering. `'layer'` groups the packages into numbered subfolders based on dependency tree layers. |
+| `packToPath` | `string` | | repo | Instead of publishing to npm, pack packages to tgz files under the specified path. |
 | `publish` | `boolean` | `true` | repo | Whether to publish to npm registry |
 | `push` | `boolean` | `true` | repo | Whether to push to the remote git branch |
 | `registry` | `string` | | repo | Publish to this npm registry |

package.json

@@ -58,7 +58,6 @@
     "p-limit": "^3.0.2",
     "prompts": "^2.4.2",
     "semver": "^7.0.0",
-    "toposort": "^2.0.2",
     "workspace-tools": "^0.41.0",
     "yargs-parser": "^21.0.0"
   },
@@ -69,7 +68,6 @@
     "@types/prompts": "^2.4.2",
     "@types/semver": "^7.3.13",
     "@types/tmp": "^0.2.3",
-    "@types/toposort": "^2.0.3",
     "@types/yargs-parser": "^21.0.0",
     "@typescript-eslint/eslint-plugin": "^5.0.0",
     "@typescript-eslint/parser": "^5.0.0",

src/__fixtures__/changeFiles.ts

@@ -2,7 +2,7 @@ import fs from 'fs';
 import path from 'path';
 import { writeChangeFiles } from '../changefile/writeChangeFiles';
 import { getChangePath } from '../paths';
-import type { ChangeFileInfo, ChangeType } from '../types/ChangeInfo';
+import type { ChangeFileInfo, ChangeSet, ChangeType } from '../types/ChangeInfo';
 import type { BeachballOptions } from '../types/BeachballOptions';
 
 /** Change file with `packageName` required and other props optional */
@@ -13,6 +13,11 @@ export const fakeEmail = 'test@test.com';
 
 /**
  * Generate a change file for the given package.
+ * Default values:
+ * - `type: 'minor'`
+ * - `dependentChangeType: 'patch'`
+ * - `comment: '<packageName> comment'`
+ * - `email: 'test@test.com'`
  */
 export function getChange(
   packageName: string,
@@ -29,18 +34,11 @@ export function getChange(
 }
 
 /**
- * Generates and writes change files for the given packages.
- * Also commits if `options.commit` is true (the default with full options) and the context is a git repo.
- * @param changes Array of package names or partial change files (which must include `packageName`).
- * Default values:
- * - `type: 'minor'`
- * - `dependentChangeType: 'patch'`
- * - `comment: '<packageName> comment'`
- * - `email: 'test@test.com'`
- *
+ * Generates change files for the given packages.
  * @param changes Array of package names or partial change files (which must include `packageName`).
+ * See {@link getChange} for default values.
  */
-export function generateChanges(changes: (string | PartialChangeFile)[]): ChangeFileInfo[] {
+function generateChanges(changes: (string | PartialChangeFile)[]): ChangeFileInfo[] {
   return changes.map(change => {
     change = typeof change === 'string' ? { packageName: change } : change;
     return {
@@ -50,6 +48,18 @@ export function generateChanges(changes: (string | PartialChangeFile)[]): Change
   });
 }
 
+/**
+ * Generates a change set for the given packages (the file names will not be realistic).
+ * @param changes Array of package names or partial change files (which must include `packageName`).
+ * See {@link getChange} for default values.
+ */
+export function generateChangeSet(changes: (string | PartialChangeFile)[]): ChangeSet {
+  return generateChanges(changes).map((change, i) => ({
+    change,
+    changeFile: `change${i}.json`,
+  }));
+}
+
 /**
  * Generates and writes change files for the given packages.
  * Also commits if `options.commit` is true.

src/__functional__/options/getOptions.test.ts

@@ -102,6 +102,8 @@ describe('getOptions (deprecated)', () => {
 });
 
 describe('getParsedOptions', () => {
+  initMockLogs({ alsoLog: ['error', 'warn'] });
+
   let repositoryFactory: RepositoryFactory;
   // Don't reuse a repo in these tests! If multiple tests load beachball.config.js from the same path,
   // it will use the version from the require cache, which will have outdated contents.

src/__functional__/packageManager/packPackage.test.ts

@@ -56,7 +56,10 @@ describe('packPackage', () => {
     const testPkg = getTestPackage('testpkg');
     writeJson(tempPackageJsonPath, testPkg.json);
 
-    const packResult = await packPackage(testPkg.info, { packToPath: tempPackPath, index: 0, total: 1 });
+    const packResult = await packPackage(testPkg.info, {
+      packToPath: tempPackPath,
+      packInfo: { index: 0, total: 1 },
+    });
     expect(packResult).toEqual(true);
     expect(npmMock.mock).toHaveBeenCalledTimes(1);
     expect(npmMock.mock).toHaveBeenCalledWith(
@@ -76,7 +79,10 @@ describe('packPackage', () => {
     const testPkg = getTestPackage('@foo/bar');
     writeJson(tempPackageJsonPath, testPkg.json);
 
-    const packResult = await packPackage(testPkg.info, { packToPath: tempPackPath, index: 0, total: 1 });
+    const packResult = await packPackage(testPkg.info, {
+      packToPath: tempPackPath,
+      packInfo: { index: 0, total: 1 },
+    });
     expect(packResult).toEqual(true);
     expect(npmMock.mock).toHaveBeenCalledTimes(1);
     expect(npmMock.mock).toHaveBeenCalledWith(
@@ -97,7 +103,10 @@ describe('packPackage', () => {
     writeJson(tempPackageJsonPath, testPkg.json);
 
     // There are 100 packages to pack, so index 1 should be prefixed with "002-"
-    const packResult = await packPackage(testPkg.info, { packToPath: tempPackPath, index: 1, total: 100 });
+    const packResult = await packPackage(testPkg.info, {
+      packToPath: tempPackPath,
+      packInfo: { index: 1, total: 100 },
+    });
     expect(packResult).toEqual(true);
     expect(npmMock.mock).toHaveBeenCalledTimes(1);
     expect(npmMock.mock).toHaveBeenCalledWith(
@@ -112,14 +121,43 @@ describe('packPackage', () => {
     expect(allLogs).toMatch(`Packed ${testPkg.spec} to ${path.join(tempPackPath, `002-${testPkg.packName}`)}`);
   });
 
+  it('packs package with packMode: "layers"', async () => {
+    const testPkg = getTestPackage('testpkg');
+    writeJson(tempPackageJsonPath, testPkg.json);
+
+    const layers = Array.from({ length: 10 }, () => [] as string[]);
+    layers[2].push(testPkg.name);
+
+    const packResult = await packPackage(testPkg.info, {
+      packToPath: tempPackPath,
+      packInfo: { layers },
+    });
+    expect(packResult).toEqual(true);
+    expect(npmMock.mock).toHaveBeenCalledTimes(1);
+    expect(npmMock.mock).toHaveBeenCalledWith(
+      ['pack', '--loglevel', 'warn'],
+      expect.objectContaining({ cwd: tempRoot })
+    );
+    const outFile = path.join(tempPackPath, '03', testPkg.packName);
+    expect(fs.existsSync(outFile)).toBe(true);
+    expect(fs.existsSync(path.join(tempRoot, testPkg.packName))).toBe(false);
+
+    const allLogs = logs.getMockLines('all');
+    expect(allLogs).toMatch(`Packing - ${testPkg.spec}`);
+    expect(allLogs).toMatch(`Packed ${testPkg.spec} to ${outFile}`);
+  });
+
   it('handles failure packing', async () => {
     const testPkg = getTestPackage('testpkg');
     // It's difficult to simulate actual error conditions, so mock an npm call failure.
     npmMock.setCommandOverride('pack', () =>
       Promise.resolve({ success: false, stdout: 'oh no', all: 'oh no' } as NpmResult)
     );
 
-    const packResult = await packPackage(testPkg.info, { packToPath: tempPackPath, index: 0, total: 1 });
+    const packResult = await packPackage(testPkg.info, {
+      packToPath: tempPackPath,
+      packInfo: { index: 0, total: 1 },
+    });
     expect(packResult).toEqual(false);
     expect(npmMock.mock).toHaveBeenCalledTimes(1);
     expect(fs.readdirSync(tempPackPath)).toEqual([]);
@@ -136,7 +174,10 @@ describe('packPackage', () => {
       Promise.resolve({ success: true, stdout: 'not a file', all: 'not a file' } as NpmResult)
     );
 
-    const packResult = await packPackage(testPkg.info, { packToPath: tempPackPath, index: 0, total: 1 });
+    const packResult = await packPackage(testPkg.info, {
+      packToPath: tempPackPath,
+      packInfo: { index: 0, total: 1 },
+    });
     expect(packResult).toEqual(false);
     expect(npmMock.mock).toHaveBeenCalledTimes(1);
     expect(fs.existsSync(path.join(tempRoot, testPkg.packName))).toBe(false);
@@ -153,7 +194,10 @@ describe('packPackage', () => {
       Promise.resolve({ success: true, stdout: 'nope.tgz', all: 'nope.tgz' } as NpmResult)
     );
 
-    const packResult = await packPackage(testPkg.info, { packToPath: tempPackPath, index: 0, total: 1 });
+    const packResult = await packPackage(testPkg.info, {
+      packToPath: tempPackPath,
+      packInfo: { index: 0, total: 1 },
+    });
     expect(packResult).toEqual(false);
     expect(npmMock.mock).toHaveBeenCalledTimes(1);
     expect(fs.existsSync(path.join(tempRoot, testPkg.packName))).toBe(false);
@@ -172,7 +216,10 @@ describe('packPackage', () => {
     fs.writeFileSync(destPath, 'other content');
     const origPath = path.join(tempRoot, testPkg.packName);
 
-    const packResult = await packPackage(testPkg.info, { packToPath: tempPackPath, index: 0, total: 1 });
+    const packResult = await packPackage(testPkg.info, {
+      packToPath: tempPackPath,
+      packInfo: { index: 0, total: 1 },
+    });
     expect(packResult).toEqual(false);
     expect(npmMock.mock).toHaveBeenCalledTimes(1);
 
@@ -196,7 +243,10 @@ describe('packPackage', () => {
       const testPkg = getTestPackage('@foo/bar');
       writeJson(tempPackageJsonPath, testPkg.json);
 
-      const packResult = await packPackage(testPkg.info, { packToPath: tempPackPath, index: 0, total: 1 });
+      const packResult = await packPackage(testPkg.info, {
+        packToPath: tempPackPath,
+        packInfo: { index: 0, total: 1 },
+      });
       expect(packResult).toEqual(true);
       expect(npmMock.mock).toHaveBeenCalledTimes(1);
       expect(npmMock.mock).toHaveBeenCalledWith(

src/__functional__/publish/__snapshots__/publishToRegistry.test.ts.snap

@@ -4,8 +4,8 @@ exports[`publishToRegistry publishes multiple packages in dependency order 1`] =
 "[log] Validating new package versions...
 
 [log] Package versions are OK to publish:
-  • lib@1.0.0
   • app@1.0.0
+  • lib@1.0.0
 
 [log] Validating no private package among package dependencies
 [log]   OK!
@@ -129,9 +129,9 @@ exports[`publishToRegistry with concurrency > 1 throws and shows recovery info o
 "[log] Validating new package versions...
 
 [log] Package versions are OK to publish:
-  • pkg3@1.0.0
-  • pkg2@1.0.0
   • pkg1@1.0.0
+  • pkg2@1.0.0
+  • pkg3@1.0.0
   • pkg4@1.0.0
   • pkg5@1.0.0
 
@@ -214,12 +214,43 @@ oh no
 To recover from this, run "beachball sync" to synchronize local package.json files with the registry."
 `;
 
-exports[`publishToRegistry with packToPath packs packages 1`] = `
+exports[`publishToRegistry with packToPath packs packages into layer folders 1`] = `
 "[log] Validating new package versions...
 
 [log] Package versions are OK to publish:
+  • app@1.0.0
+  • other@1.0.0
   • lib@1.0.0
+
+[log] Validating no private package among package dependencies
+[log]   OK!
+
+[log] Packing - app@1.0.0
+[log]   (cwd: <root>/packages/app)
+
+[log] app-1.0.0.tgz
+
+[log] Packed app@1.0.0 to <packPath>/2/app-1.0.0.tgz
+[log] Packing - other@1.0.0
+[log]   (cwd: <root>/packages/other)
+
+[log] other-1.0.0.tgz
+
+[log] Packed other@1.0.0 to <packPath>/2/other-1.0.0.tgz
+[log] Packing - lib@1.0.0
+[log]   (cwd: <root>/packages/lib)
+
+[log] lib-1.0.0.tgz
+
+[log] Packed lib@1.0.0 to <packPath>/1/lib-1.0.0.tgz"
+`;
+
+exports[`publishToRegistry with packToPath packs packages sequentially by default 1`] = `
+"[log] Validating new package versions...
+
+[log] Package versions are OK to publish:
   • app@1.0.0
+  • lib@1.0.0
 
 [log] Validating no private package among package dependencies
 [log]   OK!

src/__functional__/publish/publishToRegistry.test.ts

@@ -310,7 +310,7 @@ describe('publishToRegistry', () => {
       removeTempDir(packToPath);
     });
 
-    it('packs packages', async () => {
+    it('packs packages sequentially by default', async () => {
       const bumpInfo = makeBumpInfo({
         app: { dependencies: { lib: '1.0.0' } },
         lib: {},
@@ -349,5 +349,33 @@ describe('publishToRegistry', () => {
         logs.getMockLines('all', { replacePaths: { [tempRoot]: '<root>', [packToPath]: '<packPath>' } })
       ).toMatchSnapshot();
     });
+
+    it('packs packages into layer folders', async () => {
+      const bumpInfo = makeBumpInfo({
+        app: { dependencies: { lib: '1.0.0' } },
+        other: { dependencies: { lib: '1.0.0' } },
+        lib: {},
+      });
+
+      await publishToRegistry(bumpInfo, { ...defaultOptions, packToPath, packStyle: 'layer' });
+
+      // Nothing should be published to the registry
+      expect(npmMock.getPublishedVersions('lib')).toBeUndefined();
+      expect(npmMock.getPublishedVersions('app')).toBeUndefined();
+
+      // Tgz files should be in numbered subdirectories by layer
+      // lib has no deps → layer 0 (folder "1"), app and other depend on lib → layer 1 (folder "2")
+      const layer1 = fs.readdirSync(path.join(packToPath, '1'));
+      const layer2 = fs.readdirSync(path.join(packToPath, '2'));
+      expect(layer1).toEqual([getMockNpmPackName(bumpInfo.packageInfos.lib)]);
+      expect(layer2.sort()).toEqual([
+        getMockNpmPackName(bumpInfo.packageInfos.app),
+        getMockNpmPackName(bumpInfo.packageInfos.other),
+      ]);
+
+      expect(
+        logs.getMockLines('all', { replacePaths: { [tempRoot]: '<root>', [packToPath]: '<packPath>' } })
+      ).toMatchSnapshot();
+    });
   });
 });

src/__tests__/bump/bumpInMemory.test.ts

@@ -1,12 +1,11 @@
 import { describe, expect, it } from '@jest/globals';
 import path from 'path';
-import { generateChanges, type PartialChangeFile } from '../../__fixtures__/changeFiles';
+import { generateChangeSet, type PartialChangeFile } from '../../__fixtures__/changeFiles';
 import { initMockLogs } from '../../__fixtures__/mockLogs';
 import { makePackageInfosByFolder, type PartialPackageInfo } from '../../__fixtures__/packageInfos';
 import { bumpInMemory } from '../../bump/bumpInMemory';
 import { getParsedOptions } from '../../options/getOptions';
 import type { RepoOptions } from '../../types/BeachballOptions';
-import type { ChangeSet } from '../../types/ChangeInfo';
 import { getScopedPackages } from '../../monorepo/getScopedPackages';
 import { getPackageGroups } from '../../monorepo/getPackageGroups';
 
@@ -29,10 +28,7 @@ describe('bumpInMemory', () => {
       cwd,
       cliOptions,
     });
-    const changeSet: ChangeSet = generateChanges(params.changes).map((change, i) => ({
-      change,
-      changeFile: `change${i}.json`,
-    }));
+    const changeSet = generateChangeSet(params.changes);
     const scopedPackages = getScopedPackages(options, originalPackageInfos);
     const packageGroups = getPackageGroups(originalPackageInfos, cwd, options.groups);
 

src/__tests__/bump/callHook.test.ts

@@ -14,7 +14,7 @@ describe('callHook', () => {
       // This graph only has one possible ordering
       pkg1: { dependencies: { pkg2: '*' } },
       pkg2: { version: '2.0.0', peerDependencies: { pkg3: '*', pkg4: '*' } },
-      pkg3: { devDependencies: { pkg4: '*' } },
+      pkg3: { dependencies: { pkg4: '*' } },
       pkg4: { optionalDependencies: { pkg5: '*' } },
       pkg5: {},
     },