diff --git a/change/beachball-02900cbb-2206-4ec3-8b28-089b5f504e16.json b/change/beachball-02900cbb-2206-4ec3-8b28-089b5f504e16.json deleted file mode 100644 index f91079bba..000000000 --- a/change/beachball-02900cbb-2206-4ec3-8b28-089b5f504e16.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "type": "none", - "comment": "Update to syntax supported by new eslint", - "packageName": "beachball", - "email": "elcraig@microsoft.com", - "dependentChangeType": "none" -} diff --git a/change/beachball-13a1b781-2b1a-4eff-ad1a-03696ad51619.json b/change/beachball-13a1b781-2b1a-4eff-ad1a-03696ad51619.json deleted file mode 100644 index c1484710f..000000000 --- a/change/beachball-13a1b781-2b1a-4eff-ad1a-03696ad51619.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "comment": "Fix skill name in change prompt", - "type": "none", - "packageName": "beachball", - "email": "elcraig@microsoft.com", - "dependentChangeType": "none" -} diff --git a/change/beachball-2aa1650c-1853-4549-bd89-242886aecfa8.json b/change/beachball-2aa1650c-1853-4549-bd89-242886aecfa8.json new file mode 100644 index 000000000..c1a1e88b5 --- /dev/null +++ b/change/beachball-2aa1650c-1853-4549-bd89-242886aecfa8.json @@ -0,0 +1,7 @@ +{ + "type": "major", + "comment": "`hooks.prebump` no longer receives `packageInfos`. This was never in the signature, and trying to modify it is likely to not fully behave as you might expect. Please open an issue if you were using this and we can find an alternative.", + "packageName": "beachball", + "email": "elcraig@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/change/beachball-5caa022b-9249-4fea-b216-e88a82a478b3.json b/change/beachball-5caa022b-9249-4fea-b216-e88a82a478b3.json deleted file mode 100644 index 55e3d3e2c..000000000 --- a/change/beachball-5caa022b-9249-4fea-b216-e88a82a478b3.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "type": "none", - "comment": "temporary workaround", - "packageName": "beachball", - "email": "elcraig@microsoft.com", - "dependentChangeType": "none" -} diff --git a/change/beachball-69f402ed-44c3-432a-9726-2202c787f535.json b/change/beachball-69f402ed-44c3-432a-9726-2202c787f535.json deleted file mode 100644 index 103cd24a2..000000000 --- a/change/beachball-69f402ed-44c3-432a-9726-2202c787f535.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "type": "none", - "comment": "update comments and in-repo logic", - "packageName": "beachball", - "email": "elcraig@microsoft.com", - "dependentChangeType": "none" -} diff --git a/change/beachball-c431c165-ed07-4220-917b-e89d21ea4b1c.json b/change/beachball-c431c165-ed07-4220-917b-e89d21ea4b1c.json deleted file mode 100644 index eaf9eb43a..000000000 --- a/change/beachball-c431c165-ed07-4220-917b-e89d21ea4b1c.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "packageName": "beachball", - "type": "none", - "dependentChangeType": "none", - "comment": "Fix spelling mistakes in changelog type docs and skill prerequisites.", - "email": "198982749+Copilot@users.noreply.github.com" -} diff --git a/change/beachball-da3685e4-59a8-4263-b0ad-8a6f050a9c3a.json b/change/beachball-da3685e4-59a8-4263-b0ad-8a6f050a9c3a.json deleted file mode 100644 index 317bde1bd..000000000 --- a/change/beachball-da3685e4-59a8-4263-b0ad-8a6f050a9c3a.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "type": "none", - "comment": "publishing process updates", - "packageName": "beachball", - "email": "elcraig@microsoft.com", - "dependentChangeType": "none" -} diff --git a/change/beachball-dacf9757-cd64-4e18-aec1-146f011ebc6e.json b/change/beachball-dacf9757-cd64-4e18-aec1-146f011ebc6e.json deleted file mode 100644 index ed8ccbcfb..000000000 --- a/change/beachball-dacf9757-cd64-4e18-aec1-146f011ebc6e.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "type": "none", - "comment": "migration updates", - "packageName": "beachball", - "email": "elcraig@microsoft.com", - "dependentChangeType": "none" -} diff --git a/change/beachball-e606449d-61ec-4934-8a24-91aa58cae28b.json b/change/beachball-e606449d-61ec-4934-8a24-91aa58cae28b.json deleted file mode 100644 index 685900f47..000000000 --- a/change/beachball-e606449d-61ec-4934-8a24-91aa58cae28b.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "type": "none", - "comment": "Tests: update verdaccio to v6", - "packageName": "beachball", - "email": "elcraig@microsoft.com", - "dependentChangeType": "none" -} diff --git a/change/beachball-edbf2503-62e3-4578-ac81-96c2cb48bf45.json b/change/beachball-edbf2503-62e3-4578-ac81-96c2cb48bf45.json deleted file mode 100644 index 2e69c8bec..000000000 --- a/change/beachball-edbf2503-62e3-4578-ac81-96c2cb48bf45.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "comment": "Fix `prebump` hooks to receive the pre-bump package version.", - "type": "major", - "packageName": "beachball", - "email": "198982749+Copilot@users.noreply.github.com", - "dependentChangeType": "patch" -} diff --git a/change/change-337918d3-ea78-4c2e-9b54-f1fe3587642e.json b/change/change-337918d3-ea78-4c2e-9b54-f1fe3587642e.json deleted file mode 100644 index 47058955a..000000000 --- a/change/change-337918d3-ea78-4c2e-9b54-f1fe3587642e.json +++ /dev/null @@ -1,11 +0,0 @@ -{ - "changes": [ - { - "type": "none", - "comment": "Update formatting", - "packageName": "beachball", - "email": "elcraig@microsoft.com", - "dependentChangeType": "none" - } - ] -} \ No newline at end of file diff --git a/change/change-37a6b8a7-6fa0-4f2a-8ef5-34bca5edf36a.json b/change/change-37a6b8a7-6fa0-4f2a-8ef5-34bca5edf36a.json deleted file mode 100644 index 88557bd02..000000000 --- a/change/change-37a6b8a7-6fa0-4f2a-8ef5-34bca5edf36a.json +++ /dev/null @@ -1,11 +0,0 @@ -{ - "changes": [ - { - "type": "none", - "comment": "Allow ESM imports in jest", - "packageName": "beachball", - "email": "elcraig@microsoft.com", - "dependentChangeType": "none" - } - ] -} \ No newline at end of file diff --git a/change/change-4095d8fa-faa9-49db-8687-4a11e49f08fd.json b/change/change-4095d8fa-faa9-49db-8687-4a11e49f08fd.json deleted file mode 100644 index a6f7bab15..000000000 --- a/change/change-4095d8fa-faa9-49db-8687-4a11e49f08fd.json +++ /dev/null @@ -1,11 +0,0 @@ -{ - "changes": [ - { - "type": "none", - "comment": "(cherry-pick) Fix npm auth environment variables with yarn 4", - "packageName": "beachball", - "email": "elcraig@microsoft.com", - "dependentChangeType": "none" - } - ] -} diff --git a/change/change-6873ab67-6141-4270-b410-176ceca62bc9.json b/change/change-6873ab67-6141-4270-b410-176ceca62bc9.json deleted file mode 100644 index b85727507..000000000 --- a/change/change-6873ab67-6141-4270-b410-176ceca62bc9.json +++ /dev/null @@ -1,11 +0,0 @@ -{ - "changes": [ - { - "type": "none", - "comment": "Update deprecated rule references", - "packageName": "beachball", - "email": "elcraig@microsoft.com", - "dependentChangeType": "none" - } - ] -} \ No newline at end of file diff --git a/change/change-9f7b2622-c36c-4110-b9f3-8e9196f26a24.json b/change/change-9f7b2622-c36c-4110-b9f3-8e9196f26a24.json deleted file mode 100644 index 16628367c..000000000 --- a/change/change-9f7b2622-c36c-4110-b9f3-8e9196f26a24.json +++ /dev/null @@ -1,11 +0,0 @@ -{ - "changes": [ - { - "type": "none", - "comment": "remove CHANGELOG.json which will not be updated", - "packageName": "beachball", - "email": "elcraig@microsoft.com", - "dependentChangeType": "none" - } - ] -} \ No newline at end of file diff --git a/change/change-c8006efc-15a3-4418-871f-356266015edf.json b/change/change-c8006efc-15a3-4418-871f-356266015edf.json deleted file mode 100644 index afd2d2c93..000000000 --- a/change/change-c8006efc-15a3-4418-871f-356266015edf.json +++ /dev/null @@ -1,11 +0,0 @@ -{ - "changes": [ - { - "type": "none", - "comment": "Start preparing for v3 release", - "packageName": "beachball", - "email": "elcraig@microsoft.com", - "dependentChangeType": "none" - } - ] -} \ No newline at end of file diff --git a/change/change-e0ebd254-f3c4-4b0b-aa4f-533fe9300419.json b/change/change-e0ebd254-f3c4-4b0b-aa4f-533fe9300419.json deleted file mode 100644 index 022e1cf5c..000000000 --- a/change/change-e0ebd254-f3c4-4b0b-aa4f-533fe9300419.json +++ /dev/null @@ -1,11 +0,0 @@ -{ - "changes": [ - { - "type": "none", - "comment": "(cherry-pick) Fully fix iterative deepening for latest git", - "packageName": "beachball", - "email": "elcraig@microsoft.com", - "dependentChangeType": "none" - } - ] -} diff --git a/change/change-eda9658e-2a58-43ae-a053-2baadf355f1d.json b/change/change-eda9658e-2a58-43ae-a053-2baadf355f1d.json deleted file mode 100644 index 4dec1f592..000000000 --- a/change/change-eda9658e-2a58-43ae-a053-2baadf355f1d.json +++ /dev/null @@ -1,11 +0,0 @@ -{ - "changes": [ - { - "type": "none", - "comment": "Update devDependency normalized-tmpdir to v1.1.0", - "packageName": "beachball", - "email": "email not defined", - "dependentChangeType": "none" - } - ] -} \ No newline at end of file diff --git a/docs/overview/v3-migration.md b/docs/overview/v3-migration.md index 30d27b6cd..e26194961 100644 --- a/docs/overview/v3-migration.md +++ b/docs/overview/v3-migration.md @@ -70,10 +70,6 @@ To migrate, simply remove the leading `!` from all `exclude` patterns. The logic for determining the comparison remote and branch is stricter: beachball now throws if no remotes are defined, or if the root `package.json` specifies a `repository` field but no matching remote is found. If your `branch` option contains a `/`, beachball checks whether the leading segment matches a configured remote name, and falls back to the default remote otherwise. -### `prebump` hook version parameter now matches the docs - -In v2, `hooks.prebump` received the post-bump package version even though the hook runs before Beachball writes version changes to disk. In v3, this is corrected to receive the pre-bump version as documented. - ### Custom changelog rendering changes Only relevant for custom changelog renderers: `PackageChangelog.tag` and `ChangelogJsonEntry.tag` are now `undefined` when the package had no associated git tag (previously a value was always present). @@ -98,18 +94,15 @@ In v3, `shouldPublish: false` packages are full participants in all steps of the - If a published package has a `shouldPublish: false` package in its production dependencies, Beachball will exit with an error (same as with `private: true` deps) - Since `shouldPublish: false` is redundant with `private: true`, `beachball migrate` reports this as an error -### Renamed options - -- Rename `BeachballOptions.changelog.groups[*].masterPackageName` to `mainPackageName` - -### Removed options +### `BeachballOptions`/`RepoOptions` updates -- `new`: This option was never needed if PR builds run `beachball check` (a new package without a change file already causes an error), and it had a significant performance cost because it checked the registry for _all_ unmodified packages. -- `packStyle`: packing always uses the layered style now. -- `help` and `version` properties: these still work on the command line but were removed from `BeachballOptions` +- Rename `changelog.groups[*].masterPackageName` to `mainPackageName`. +- Removed rarely-used options: + - `new`: This option was never needed if PR builds run `beachball check` (a new package without a change file already causes an error), and it had a significant performance cost because it checked the registry for _all_ unmodified packages. + - `packStyle`: packing always uses the layered style now. + - `help` and `version` properties: these still work on the command line but were removed from `BeachballOptions` +- `hooks.prebump` no longer receives `packageInfos`. This was never in the signature, and trying to modify it may lead to unexpected behavior. Please open an issue if you were using this and we can find an alternative. ### Other changes -- If you're deep importing beachball's internal helpers: - - Some deprecated signatures have been removed. Use the new signatures (which pre-calculate and share context) instead. - - `BumpInfo` now contains `originalPackageInfos` for `prebump` hook correctness. +- If you're deep importing beachball's internal helpers, some deprecated signatures have been removed. Use the new signatures (which pre-calculate and share context) instead. diff --git a/packages/beachball/src/__e2e__/bump.test.ts b/packages/beachball/src/__e2e__/bump.test.ts index 6235eb708..bbc71cfa3 100644 --- a/packages/beachball/src/__e2e__/bump.test.ts +++ b/packages/beachball/src/__e2e__/bump.test.ts @@ -555,11 +555,12 @@ describe('bump command', () => { prebump: jest.fn>(async (packagePath, name, version) => { expect(packagePath.endsWith('pkg-1')).toBeTruthy(); expect(name).toBe('pkg-1'); - expect(version).toBe('1.0.0'); + // "prebump" receives the bumped version, but is called before writing to disk + expect(version).toBe('1.1.0'); await new Promise(resolve => setTimeout(resolve, 0)); // simulate async work const jsonPath = path.join(packagePath, 'package.json'); - expect(readJson(jsonPath).version).toBe('1.0.0'); + expect(readJson(jsonPath).version).toBe('1.0.0'); // not bumped on disk yet }), postbump: jest.fn>(async (packagePath, name, version) => { expect(packagePath.endsWith('pkg-1')).toBeTruthy(); diff --git a/packages/beachball/src/__functional__/commands/migrate.test.ts b/packages/beachball/src/__functional__/commands/migrate.test.ts index fc7996950..1a85fa568 100644 --- a/packages/beachball/src/__functional__/commands/migrate.test.ts +++ b/packages/beachball/src/__functional__/commands/migrate.test.ts @@ -2,7 +2,7 @@ import { describe, expect, it, afterEach, jest } from '@jest/globals'; import { initMockLogs } from '../../__fixtures__/mockLogs'; import { migrate } from '../../commands/migrate'; import { getOptions as _getOptions } from '../../options/getOptions'; -import type { RepoOptions } from '../../types/BeachballOptions'; +import type { HooksOptions, RepoOptions } from '../../types/BeachballOptions'; import { removeTempDir } from '../../__fixtures__/tmpdir'; import { createTestFileStructureType, updateJsonFile } from '../../__fixtures__/createTestFileStructure'; import fs from 'fs'; @@ -68,6 +68,20 @@ describe('migrate command', () => { `); }); + it('errors on "hooks.prebump" with more than 3 params', () => { + tempRoot = createTestFileStructureType('single'); + const fn: HooksOptions['postbump'] = (_pth, _name, _version, _pkgInfos) => {}; + const options = getOptions({ + hooks: { prebump: fn as HooksOptions['prebump'] }, + }); + + expect(() => migrate(options)).toThrow(BeachballError); + expect(logs.getMockLines('all')).toMatchInlineSnapshot(` + "[error] The following updates are needed for v3: + [error] • \`hooks.prebump\` no longer receives \`packageInfos\`. See migration guide." + `); + }); + it('warns on public packages using shouldPublish option', () => { tempRoot = createTestFileStructureType('monorepo'); updateJsonFile(path.join(tempRoot, 'packages/foo/package.json'), { beachball: { shouldPublish: false } }); diff --git a/packages/beachball/src/__functional__/publish/publishToRegistry.test.ts b/packages/beachball/src/__functional__/publish/publishToRegistry.test.ts index 8bd036f2d..1145d70b4 100644 --- a/packages/beachball/src/__functional__/publish/publishToRegistry.test.ts +++ b/packages/beachball/src/__functional__/publish/publishToRegistry.test.ts @@ -26,7 +26,6 @@ describe('publishToRegistry', () => { const npmMock = initNpmMock(); const logs = initMockLogs(); - /** Needs a real fake temp root for mock npm publish */ let tempRoot: string; let defaultOptions: BeachballOptions; @@ -62,7 +61,6 @@ describe('publishToRegistry', () => { return { changeFileChangeInfos: [], packageInfos, - originalPackageInfos: makePackageInfos(partialPackageInfos, { path: tempRoot }), calculatedChangeTypes: Object.fromEntries(names.map(n => [n, 'patch' as const])), packageGroups: {}, modifiedPackages: new Set(names), @@ -231,7 +229,7 @@ describe('publishToRegistry', () => { expect(prebump).toHaveBeenCalledTimes(1); expect(postbump).toHaveBeenCalledTimes(1); const fooPath = expect.stringMatching(/packages[\\/]foo$/); - expect(prebump as typeof postbump).toHaveBeenCalledWith(fooPath, 'foo', '1.0.0', expect.anything()); + expect(prebump).toHaveBeenCalledWith(fooPath, 'foo', '1.0.1'); expect(postbump).toHaveBeenCalledWith(fooPath, 'foo', '1.0.1', expect.anything()); }); diff --git a/packages/beachball/src/__tests__/bump/callHook.test.ts b/packages/beachball/src/__tests__/bump/callHook.test.ts index 65b00ad7e..4d0dc08e1 100644 --- a/packages/beachball/src/__tests__/bump/callHook.test.ts +++ b/packages/beachball/src/__tests__/bump/callHook.test.ts @@ -4,7 +4,8 @@ import { makePackageInfos } from '../../__fixtures__/packageInfos'; import type { HooksOptions } from '../../types/BeachballOptions'; import path from 'path'; -type AnyHook = NonNullable; +type PostbumpHook = NonNullable; +type PrebumpHook = NonNullable; const root = path.resolve('/fake/root'); @@ -21,69 +22,84 @@ describe('callHook', () => { { path: root } ); - /** Get hook calls without the final `packageInfos` param for simpler diffs */ - function getHookCalls(hook: jest.Mock) { - return hook.mock.calls.map(call => call.slice(0, 3)); - } - /** Get package names from the list of hook calls */ - function getHookCallNames(hook: jest.Mock) { + function getHookCallNames(hook: jest.Mock) { return hook.mock.calls.map(call => call[1]); } it('does nothing if hook is undefined', async () => { - await callHook(undefined, ['pkg1'], packageInfos, 1); + await callHook('prebump', ['pkg1'], packageInfos, { hooks: {}, concurrency: 1 }); }); it('does nothing if no affected packages', async () => { - const mockHook = jest.fn(); - await callHook(mockHook, [], packageInfos, 1); + const mockHook = jest.fn(); + await callHook('postbump', [], packageInfos, { hooks: { postbump: mockHook }, concurrency: 1 }); expect(mockHook).not.toHaveBeenCalled(); }); // Currently there's no topological ordering for non-concurrent hooks // (might make sense to either add here or remove for concurrent hooks) it('calls hook for each affected package in order with concurrency=1', async () => { - const mockHook = jest.fn(); + const mockHook = jest.fn(); - await callHook(mockHook, ['pkg3', 'pkg2', 'pkg5'], packageInfos, 1); + await callHook('postbump', ['pkg3', 'pkg2', 'pkg5'], packageInfos, { + hooks: { postbump: mockHook }, + concurrency: 1, + }); // Verify the exact args of one call expect(mockHook).toHaveBeenCalledWith(path.join(root, 'packages/pkg2'), 'pkg2', '2.0.0', packageInfos); - // Most of the tests omit the very large final packageInfos arg for better diffs on error - expect(getHookCalls(mockHook)).toEqual([ - [expect.stringMatching(/pkg3$/), 'pkg3', '1.0.0'], - [expect.stringMatching(/pkg2$/), 'pkg2', '2.0.0'], - [expect.stringMatching(/pkg5$/), 'pkg5', '1.0.0'], + expect(getHookCallNames(mockHook)).toEqual(['pkg3', 'pkg2', 'pkg5']); + // Most of the tests omit the very large final packageInfos arg for better diffs on error, + // but test it here once + expect(mockHook.mock.calls).toEqual([ + [path.join(root, 'packages/pkg3'), 'pkg3', '1.0.0', packageInfos], + [path.join(root, 'packages/pkg2'), 'pkg2', '2.0.0', packageInfos], + [path.join(root, 'packages/pkg5'), 'pkg5', '1.0.0', packageInfos], ]); }); - it('works with set of affected packages', async () => { - const mockHook = jest.fn(); + it('works with Set of affected packages', async () => { + const mockHook = jest.fn(); + const affected = new Set(['pkg3', 'pkg2']); + await callHook('postbump', affected, packageInfos, { hooks: { postbump: mockHook }, concurrency: 1 }); + expect(getHookCallNames(mockHook)).toEqual(['pkg3', 'pkg2']); + }); - await callHook(mockHook, new Set(['pkg3', 'pkg2']), packageInfos, 1); + it.each(['postbump', 'prepublish', 'postpublish'] as const)('calls %s hook with PackageInfos', async hookName => { + const mockHook = jest.fn(); + await callHook(hookName, ['pkg1'], packageInfos, { hooks: { [hookName]: mockHook }, concurrency: 1 }); + expect(mockHook).toHaveBeenCalledTimes(1); + expect(mockHook).toHaveBeenCalledWith(path.join(root, 'packages/pkg1'), 'pkg1', '1.0.0', packageInfos); + }); - expect(getHookCallNames(mockHook)).toEqual(['pkg3', 'pkg2']); + it('calls prebump hook without PackageInfos', async () => { + const mockHook = jest.fn(); + await callHook('prebump', ['pkg1'], packageInfos, { hooks: { prebump: mockHook }, concurrency: 1 }); + expect(mockHook).toHaveBeenCalledTimes(1); + expect(mockHook).toHaveBeenCalledWith(path.join(root, 'packages/pkg1'), 'pkg1', '1.0.0'); }); // really should have been validated already it('ignores nonexistent package with concurrency=1', async () => { - const mockHook = jest.fn(); - - await callHook(mockHook, ['pkg1', 'nonexistent', 'pkg4'], packageInfos, 1); + const mockHook = jest.fn(); + await callHook('postbump', ['pkg1', 'nonexistent', 'pkg4'], packageInfos, { + hooks: { postbump: mockHook }, + concurrency: 1, + }); expect(mockHook).toHaveBeenCalledTimes(2); }); it('calls hook sequentially when concurrency=1', async () => { const callOrder: string[] = []; - const mockHook = jest.fn(async (_, name) => { + const mockHook = jest.fn(async (_, name) => { callOrder.push(`start-${name}`); await new Promise(resolve => setTimeout(resolve, 20)); callOrder.push(`end-${name}`); }); - await callHook(mockHook, ['pkg1', 'pkg2'], packageInfos, 1); + await callHook('postbump', ['pkg1', 'pkg2'], packageInfos, { hooks: { postbump: mockHook }, concurrency: 1 }); // With concurrency=1, should be fully sequential expect(callOrder).toEqual(['start-pkg1', 'end-pkg1', 'start-pkg2', 'end-pkg2']); @@ -91,39 +107,49 @@ describe('callHook', () => { // sync/async shouldn't be any different here it('propagates sync hook errors with concurrency=1', async () => { - const mockHook = jest.fn((_, name) => { + const mockHook = jest.fn((_, name) => { if (name === 'pkg2') throw new Error('oh no'); }); - await expect(() => callHook(mockHook, ['pkg1', 'pkg2', 'pkg3'], packageInfos, 1)).rejects.toThrow('oh no'); + await expect(() => + callHook('postbump', ['pkg1', 'pkg2', 'pkg3'], packageInfos, { hooks: { postbump: mockHook }, concurrency: 1 }) + ).rejects.toThrow('oh no'); // failed on second call, does not continue expect(mockHook).toHaveBeenCalledTimes(2); }); it('propagates async hook errors with concurrency=1', async () => { - const mockHook = jest.fn(async (_, name) => { + const mockHook = jest.fn(async (_, name) => { if (name === 'pkg2') { await new Promise(resolve => setTimeout(resolve, 0)); throw new Error('async oh no'); } }); - await expect(() => callHook(mockHook, ['pkg1', 'pkg2', 'pkg3'], packageInfos, 1)).rejects.toThrow('async oh no'); + await expect(() => + callHook('postbump', ['pkg1', 'pkg2', 'pkg3'], packageInfos, { hooks: { postbump: mockHook }, concurrency: 1 }) + ).rejects.toThrow('async oh no'); expect(mockHook).toHaveBeenCalledTimes(2); }); it('calls hook with concurrency > 1 in topological order', async () => { - const mockHook = jest.fn(); + const mockHook = jest.fn(); - await callHook(mockHook, ['pkg1', 'pkg5', 'pkg4', 'pkg2', 'pkg3'], packageInfos, 2); + await callHook('postbump', ['pkg1', 'pkg5', 'pkg4', 'pkg2', 'pkg3'], packageInfos, { + hooks: { postbump: mockHook }, + concurrency: 2, + }); expect(getHookCallNames(mockHook)).toEqual(['pkg5', 'pkg4', 'pkg3', 'pkg2', 'pkg1']); }); it('ignores nonexistent packages with concurrency > 1', async () => { - const mockHook = jest.fn(); + const mockHook = jest.fn(); - await callHook(mockHook, ['pkg1', 'nonexistent', 'pkg2'], packageInfos, 3); + await callHook('postbump', ['pkg1', 'nonexistent', 'pkg2'], packageInfos, { + hooks: { postbump: mockHook }, + concurrency: 3, + }); expect(getHookCallNames(mockHook)).toEqual(['pkg2', 'pkg1']); }); @@ -132,7 +158,7 @@ describe('callHook', () => { const callOrder: string[] = []; let currentConcurrency = 0; let maxConcurrency = 0; - const mockHook = jest.fn(async (_, name) => { + const mockHook = jest.fn(async (_, name) => { callOrder.push(`start-${name}`); currentConcurrency++; maxConcurrency = Math.max(maxConcurrency, currentConcurrency); @@ -141,7 +167,10 @@ describe('callHook', () => { callOrder.push(`end-${name}`); }); - await callHook(mockHook, ['pkg1', 'pkg2', 'pkg3', 'pkg4', 'pkg5'], packageInfos, 3); + await callHook('postbump', ['pkg1', 'pkg2', 'pkg3', 'pkg4', 'pkg5'], packageInfos, { + hooks: { postbump: mockHook }, + concurrency: 3, + }); expect(maxConcurrency).toBeLessThanOrEqual(3); @@ -155,20 +184,25 @@ describe('callHook', () => { // this shouldn't be any different sync/async, but just in case... it('propagates sync hook errors with concurrency > 1', async () => { - const mockHook = jest.fn((_, name) => { + const mockHook = jest.fn((_, name) => { if (name === 'pkg2') { throw new Error('oh no'); } }); // this will be in topological order so pkg2 is the third call - await expect(() => callHook(mockHook, ['pkg1', 'pkg2', 'pkg3', 'pkg4'], packageInfos, 2)).rejects.toThrow('oh no'); + await expect(() => + callHook('postbump', ['pkg1', 'pkg2', 'pkg3', 'pkg4'], packageInfos, { + hooks: { postbump: mockHook }, + concurrency: 2, + }) + ).rejects.toThrow('oh no'); // stops as soon as error is encountered expect(mockHook).toHaveBeenCalledTimes(3); }); it('propagates async hook errors with concurrency > 1', async () => { - const mockHook = jest.fn(async (_, name) => { + const mockHook = jest.fn(async (_, name) => { if (name === 'pkg2') { await new Promise(resolve => setTimeout(resolve, 0)); throw new Error('oh no'); @@ -176,7 +210,12 @@ describe('callHook', () => { }); // this will be in topological order so pkg2 is the third call - await expect(() => callHook(mockHook, ['pkg1', 'pkg2', 'pkg3', 'pkg4'], packageInfos, 2)).rejects.toThrow('oh no'); + await expect(() => + callHook('postbump', ['pkg1', 'pkg2', 'pkg3', 'pkg4'], packageInfos, { + hooks: { postbump: mockHook }, + concurrency: 2, + }) + ).rejects.toThrow('oh no'); // stops as soon as error is encountered expect(mockHook).toHaveBeenCalledTimes(3); }); diff --git a/packages/beachball/src/__tests__/bump/performBump.test.ts b/packages/beachball/src/__tests__/bump/performBump.test.ts index fd8eb9989..03d42b87a 100644 --- a/packages/beachball/src/__tests__/bump/performBump.test.ts +++ b/packages/beachball/src/__tests__/bump/performBump.test.ts @@ -40,12 +40,10 @@ describe('performBump', () => { /** Package infos for current test */ let packageInfos: PackageInfos | undefined; - /** "Original" non-bumped package infos for current test */ - let originalPackageInfos: PackageInfos | undefined; /** Get the package.json from `packageInfos` for the given package name */ - function packageJsonFor(source: PackageInfos | undefined, pkgName: string) { - const pkg = source?.[pkgName]; + function packageJsonFor(pkgName: string) { + const pkg = packageInfos?.[pkgName]; if (!pkg) { throw new Error(`No package info for ${pkgName}`); } @@ -66,7 +64,6 @@ describe('performBump', () => { */ function performBumpWrapper(params: { packageInfos: PartialPackageInfos; - originalPackageInfos?: PartialPackageInfos; modifiedPackages?: BumpInfo['modifiedPackages']; /** Names to generate empty `changeFileChangeInfos` */ changeFileNames?: string[]; @@ -80,14 +77,11 @@ describe('performBump', () => { }); packageInfos = makePackageInfos(params.packageInfos, opts.cliOptions); - originalPackageInfos = makePackageInfos(params.originalPackageInfos || params.packageInfos, opts.cliOptions); return performBump( { - // performBump only directly uses packageInfos, originalPackageInfos, modifiedPackages, - // and names from changeFileChangeInfos. + // performBump only directly uses packageInfos, modifiedPackages, and names from changeFileChangeInfos. packageInfos, - originalPackageInfos, modifiedPackages: params.modifiedPackages || new Set(Object.keys(packageInfos)), changeFileChangeInfos: (params.changeFileNames || []).map(name => ({ changeFile: name, @@ -112,22 +106,19 @@ describe('performBump', () => { // Only say package.json files exist mockFs.existsSync.mockImplementation(filePath => String(filePath).endsWith('package.json')); - // Mock readFileSync to return package.json content from originalPackageInfos, - // representing the pre-bump state still on disk when performBump starts. + // Mock readFileSync to return package.json based on packageInfos mockFs.readFileSync.mockImplementation((filePath => { filePath = String(filePath); if (!filePath.endsWith('package.json')) { throw new Error(`readFileSync not mocked for ${filePath}`); } - const packageJson = packageJsonFor(originalPackageInfos, path.basename(path.dirname(filePath))); - + const packageJson = packageJsonFor(path.basename(path.dirname(filePath))); return JSON.stringify(packageJson); }) as typeof _fs.readFileSync); }); afterEach(() => { packageInfos = undefined; - originalPackageInfos = undefined; }); it('updates package.json files for modified packages only', async () => { @@ -138,8 +129,8 @@ describe('performBump', () => { const mockCalls = mockWriteJson.mock.calls.filter(call => call[0].endsWith('package.json')); expect(mockCalls).toEqual([ - [packageInfos!.pkg2.packageJsonPath, packageJsonFor(packageInfos, 'pkg2')], - [packageInfos!.pkg3.packageJsonPath, packageJsonFor(packageInfos, 'pkg3')], + [packageInfos!.pkg2.packageJsonPath, packageJsonFor('pkg2')], + [packageInfos!.pkg3.packageJsonPath, packageJsonFor('pkg3')], ]); // other expected mocks @@ -192,6 +183,8 @@ describe('performBump', () => { expect(mockFs.rmSync).not.toHaveBeenCalled(); }); + // Currently prebump is using the wrong version, so the test/mocks might need updating + // https://github.com/microsoft/beachball/issues/1116 it('calls prebump hook for each package before writing', async () => { const hook = jest.fn(() => { expect(mockWriteJson).not.toHaveBeenCalled(); @@ -200,8 +193,7 @@ describe('performBump', () => { }); await performBumpWrapper({ - packageInfos: { pkg1: { version: '1.0.0' }, pkg2: { version: '2.0.1' }, pkg3: { version: '1.1.0' } }, - originalPackageInfos: { pkg1: { version: '1.0.0' }, pkg2: { version: '2.0.0' }, pkg3: { version: '1.0.0' } }, + packageInfos: { pkg1: { version: '1.0.0' }, pkg2: { version: '2.0.0' }, pkg3: { version: '1.0.0' } }, modifiedPackages: new Set(['pkg2', 'pkg3']), changeFileNames: ['change1', 'change2'], repoOptions: { hooks: { prebump: hook } }, diff --git a/packages/beachball/src/__tests__/publish/bumpAndPush.test.ts b/packages/beachball/src/__tests__/publish/bumpAndPush.test.ts index d4a8059ed..8532a19de 100644 --- a/packages/beachball/src/__tests__/publish/bumpAndPush.test.ts +++ b/packages/beachball/src/__tests__/publish/bumpAndPush.test.ts @@ -76,7 +76,6 @@ describe('bumpAndPush', () => { }, }); const bumpInfo: BumpInfo = { - originalPackageInfos: makePackageInfos({ foo: { version: '1.0.0' }, bar: { version: '1.0.0' } }), packageInfos: makePackageInfos({ foo: { version: '1.1.0' }, bar: { version: '2.0.0' } }), modifiedPackages: new Set(['foo', 'bar']), changeFileChangeInfos: [], diff --git a/packages/beachball/src/bump/bumpInMemory.ts b/packages/beachball/src/bump/bumpInMemory.ts index a1a1dab14..6e3d7ed16 100644 --- a/packages/beachball/src/bump/bumpInMemory.ts +++ b/packages/beachball/src/bump/bumpInMemory.ts @@ -25,7 +25,6 @@ export function bumpInMemory(options: BeachballOptions, context: Omit = { - originalPackageInfos: context.originalPackageInfos, calculatedChangeTypes, packageInfos: cloneObject(context.originalPackageInfos), packageGroups: context.packageGroups, diff --git a/packages/beachball/src/bump/callHook.ts b/packages/beachball/src/bump/callHook.ts index 011919cfc..60cb14774 100644 --- a/packages/beachball/src/bump/callHook.ts +++ b/packages/beachball/src/bump/callHook.ts @@ -1,5 +1,5 @@ import path from 'path'; -import type { HooksOptions } from '../types/BeachballOptions'; +import type { BeachballOptions } from '../types/BeachballOptions'; import type { PackageInfo, PackageInfos } from '../types/PackageInfo'; import { getPackageGraph } from '../monorepo/getPackageGraph'; @@ -7,12 +7,12 @@ import { getPackageGraph } from '../monorepo/getPackageGraph'; * Call a hook for each affected package. Does nothing if the hook is undefined. */ export async function callHook( - hook: HooksOptions['prebump' | 'postbump' | 'prepublish' | 'postpublish'], + hookName: 'prebump' | 'postbump' | 'prepublish' | 'postpublish', affectedPackages: string[] | Set, packageInfos: PackageInfos, - concurrency: number + options: Pick ): Promise { - if (!hook) { + if (!options.hooks?.[hookName]) { return; } @@ -24,10 +24,16 @@ export async function callHook( const callHookInternal = async (packageInfo: PackageInfo) => { const packagePath = path.dirname(packageInfo.packageJsonPath); - await hook(packagePath, packageInfo.name, packageInfo.version, packageInfos); + if (hookName === 'prebump') { + // prevent consumers from modifying packageInfos, which likely would not fully work + // as they intended + await options.hooks?.[hookName]?.(packagePath, packageInfo.name, packageInfo.version); + } else { + await options.hooks?.[hookName]?.(packagePath, packageInfo.name, packageInfo.version, packageInfos); + } }; - if (concurrency === 1) { + if (options.concurrency === 1) { for (const pkg of affectedPackages) { await callHookInternal(packageInfos[pkg]); } @@ -36,7 +42,7 @@ export async function callHook( const packageGraph = getPackageGraph(affectedPackages, packageInfos, callHookInternal); await packageGraph.run({ - concurrency: concurrency, + concurrency: options.concurrency, continue: false, }); } diff --git a/packages/beachball/src/bump/performBump.ts b/packages/beachball/src/bump/performBump.ts index a1808e4b9..242b3a9a6 100644 --- a/packages/beachball/src/bump/performBump.ts +++ b/packages/beachball/src/bump/performBump.ts @@ -20,9 +20,11 @@ import { updateLockFile } from './updateLockFile'; * @param bumpInfo Bump info produced by `bumpInMemory` which already reflects in-memory bumps */ export async function performBump(bumpInfo: Readonly, options: BeachballOptions): Promise { - const { modifiedPackages, packageInfos, changeFileChangeInfos, originalPackageInfos } = bumpInfo; + const { modifiedPackages, packageInfos, changeFileChangeInfos } = bumpInfo; - await callHook(options.hooks?.prebump, modifiedPackages, originalPackageInfos, options.concurrency); + // "prebump" receives the bumped version, but is called before writing to disk + // (seemingly intended by the original PR https://github.com/microsoft/beachball/pull/608) + await callHook('prebump', modifiedPackages, packageInfos, options); updatePackageJsons(modifiedPackages, packageInfos); await updateLockFile(options); @@ -35,5 +37,5 @@ export async function performBump(bumpInfo: Readonly, options: Beachba // Unlink changelogs unlinkChangeFiles(changeFileChangeInfos, options); - await callHook(options.hooks?.postbump, modifiedPackages, packageInfos, options.concurrency); + await callHook('postbump', modifiedPackages, packageInfos, options); } diff --git a/packages/beachball/src/commands/migrate.ts b/packages/beachball/src/commands/migrate.ts index 6b3352a89..9bf614f8b 100644 --- a/packages/beachball/src/commands/migrate.ts +++ b/packages/beachball/src/commands/migrate.ts @@ -74,6 +74,10 @@ export function migrate(parsedOptions: ParsedOptions): void { ); } + if (repoOptions.hooks?.prebump?.length ?? 0 > 3) { + updates.push('`hooks.prebump` no longer receives `packageInfos`. See migration guide.'); + } + if (rawPackageInfos) { checkShouldPublish({ rawPackageInfos, warnings, updates }); checkChangelogJson({ rawPackageInfos, options, repoOptions, updates }); diff --git a/packages/beachball/src/publish/publishToRegistry.ts b/packages/beachball/src/publish/publishToRegistry.ts index 9fb0cfab2..3db59da91 100644 --- a/packages/beachball/src/publish/publishToRegistry.ts +++ b/packages/beachball/src/publish/publishToRegistry.ts @@ -76,7 +76,7 @@ export async function publishToRegistry(bumpInfo: BumpInfo, options: BeachballOp performPublishOverrides(packagesToPublish, bumpInfo.packageInfos, catalogs); // if there is a prepublish hook perform a prepublish pass, calling the routine on each package - await callHook(options.hooks?.prepublish, packagesToPublish, bumpInfo.packageInfos, options.concurrency); + await callHook('prepublish', packagesToPublish, bumpInfo.packageInfos, options); // finally pass through doing the actual npm publish command const succeededPackages = new Set(); @@ -140,5 +140,5 @@ export async function publishToRegistry(bumpInfo: BumpInfo, options: BeachballOp } // if there is a postpublish hook perform a postpublish pass, calling the routine on each package - await callHook(options.hooks?.postpublish, packagesToPublish, bumpInfo.packageInfos, options.concurrency); + await callHook('postpublish', packagesToPublish, bumpInfo.packageInfos, options); } diff --git a/packages/beachball/src/types/BeachballOptions.ts b/packages/beachball/src/types/BeachballOptions.ts index 15d28ffd0..f9f7ea79b 100644 --- a/packages/beachball/src/types/BeachballOptions.ts +++ b/packages/beachball/src/types/BeachballOptions.ts @@ -316,8 +316,12 @@ export interface PackageOptions extends Partial< > { tag?: string | null; /** - * Disable publishing a particular package. - * (Does NOT work to enable publishing a package that wouldn't otherwise be published.) + * In most cases, you should use `private: true` to disable publishing a package. This option is + * **ONLY** for cases where a package shouldn't be published, but you still want it to require + * change files and go through the bumping process (changelogs, version bumps, git tags). + * An example in the beachball repo is management of the GitHub actions it provides. + * + * Does NOT work to enable publishing a package that wouldn't otherwise be published. */ shouldPublish?: false; } @@ -364,13 +368,13 @@ export interface HooksOptions { * * @param packagePath The path to the package directory * @param name The name of the package as defined in package.json - * @param version The **post-bump** version of the package to be published - * @param packageInfos Metadata about other packages processed by Beachball after bumping. Readonly. + * @param bumpedVersion The **post-bump** version of the package to be published + * @param packageInfos **Read-only** info about all packages in the repo after bumping */ prepublish?: ( packagePath: string, name: string, - version: string, + bumpedVersion: string, packageInfos: Readonly ) => void | Promise; @@ -380,49 +384,64 @@ export interface HooksOptions { * * @param packagePath The path to the package directory * @param name The name of the package as defined in package.json - * @param version The post-bump version of the package to be published - * @param packageInfos Metadata about other packages processed by Beachball after bumping. Readonly. + * @param bumpedVersion The post-bump version of the package to be published + * @param packageInfos **Read-only** info about all packages in the repo after bumping */ postpublish?: ( packagePath: string, name: string, - version: string, + bumpedVersion: string, packageInfos: Readonly ) => void | Promise; /** - * Runs for each bumped package, before writing changelog and package.json updates to the - * filesystem. Skipped if `bump: false`. + * Runs for each bumped package, **before** writing changelog and package.json updates to the + * filesystem (but after bumping in memory). Skipped if `bump: false`. * - * In the `bump` flow, this is called once for each package. - * In the `publish` flow, it's called: + * In the `bump` flow, this is called once for each bumped package. + * In the `publish` flow, it's called twice: * 1. when generating version bumps and changelogs to commit/push - * 2. before publishing to npm + * 2. before publishing to npm (unless the package has `shouldPublish: false`) + * + * File changes will be committed (`bump`) or published (`publish`), but will NOT modify + * the in-memory version bumps which have already happened. * * @param packagePath The path to the package directory * @param name The name of the package as defined in package.json - * @param version The **pre-bump** version of the package to be published + * @param bumpedVersion The **bumped version** of the package to be published. If you want the + * original version prior to bumping, read it from `package.json`. + * (The hook name `prebump` refers to the hook being called before updates are *written*.) */ - prebump?: (packagePath: string, name: string, version: string) => void | Promise; + prebump?: ( + packagePath: string, + name: string, + // Using the bumped version seems to have been the intent in the original PR: https://github.com/microsoft/beachball/pull/608 + bumpedVersion: string + // This hook does NOT receive PackageInfos, since that's easily misunderstood as being able to + // modify it, which won't fully work as expected. Any such scenarios would be better addressed + // by opening an issue to figure out a proper solution (likely a new config option). + ) => void | Promise; /** - * Runs for each bumped package, after writing changelog and package.json updates to the - * filesystem. Skipped if `bump: false`. + * Runs for each bumped package, **after** writing changelog and package.json updates to the + * filesystem, but before pushing or publishing. Skipped if `bump: false`. + * + * In the `bump` flow, this is called once for each bumped package. + * In the `publish` flow, it's called twice: + * 1. when generating version bumps and changelogs to commit/push + * 2. before publishing to npm (unless the package has `shouldPublish: false`) * - * In the `bump` flow, this is called once for each package. - * In the `publish` flow, it's called: - * 1. when generating version bumps and changelogs to commit/push (file changes will be committed) - * 2. before publishing to npm, only for packages that will be published (file changes will be published) + * File changes will be committed (`bump`) or published (`publish`). * * @param packagePath The path to the package directory * @param name The name of the package as defined in package.json - * @param version The **post-bump** version of the package to be published - * @param packageInfos Metadata about other packages processed by Beachball after bumping. Readonly. + * @param bumpedVersion The **post-bump** version of the package to be published + * @param packageInfos **Read-only** info about all packages in the repo after bumping */ postbump?: ( packagePath: string, name: string, - version: string, + bumpedVersion: string, packageInfos: Readonly ) => void | Promise; diff --git a/packages/beachball/src/types/BumpInfo.ts b/packages/beachball/src/types/BumpInfo.ts index decb5382a..9e8589340 100644 --- a/packages/beachball/src/types/BumpInfo.ts +++ b/packages/beachball/src/types/BumpInfo.ts @@ -14,9 +14,6 @@ export type BumpInfo = { */ packageInfos: PackageInfos; - /** Packages before bumping, which must NOT be mutated. */ - originalPackageInfos: PackageInfos; - /** * Mapping from package name to change type. * diff --git a/packages/beachball/src/validation/validate.ts b/packages/beachball/src/validation/validate.ts index 6e5a0dec3..a69caa506 100644 --- a/packages/beachball/src/validation/validate.ts +++ b/packages/beachball/src/validation/validate.ts @@ -142,6 +142,11 @@ export function validate(parsedOptions: ParsedOptions, validateOptions: Validate hasError = true; // the helper logs this } + if (options.hooks?.prebump?.length ?? 0 > 3) { + logValidationError('prebump hook does not receive packageInfos - see the beachball v3 migration guide'); + hasError = true; + } + const scopedPackages = getScopedPackages(options, originalPackageInfos); const changeSet = readChangeFiles(options, originalPackageInfos, scopedPackages);