From c598c79c32a79d7a6fc698b5d049f40cc04af09d Mon Sep 17 00:00:00 2001 From: Elizabeth Craig Date: Mon, 25 Nov 2024 17:47:08 -0800 Subject: [PATCH 1/2] getPackageInfos should only get repo and CLI options once --- ...-1290455d-876a-4e72-821c-6e0bc0750219.json | 7 + docs/overview/configuration.md | 35 ++- src/__e2e__/publishE2E.test.ts | 2 +- src/__fixtures__/registry.ts | 11 +- .../monorepo/getPackageInfos.test.ts | 212 ++++++++---------- src/monorepo/getPackageInfos.ts | 83 +++---- src/monorepo/infoFromPackageJson.ts | 19 -- src/options/getOptions.ts | 1 + src/options/getPackageInfosWithOptions.ts | 49 ++++ src/options/getPackageOptions.ts | 38 ---- src/types/BeachballOptions.ts | 1 - src/types/PackageInfo.ts | 4 +- 12 files changed, 219 insertions(+), 243 deletions(-) create mode 100644 change/beachball-1290455d-876a-4e72-821c-6e0bc0750219.json delete mode 100644 src/monorepo/infoFromPackageJson.ts create mode 100644 src/options/getPackageInfosWithOptions.ts delete mode 100644 src/options/getPackageOptions.ts diff --git a/change/beachball-1290455d-876a-4e72-821c-6e0bc0750219.json b/change/beachball-1290455d-876a-4e72-821c-6e0bc0750219.json new file mode 100644 index 00000000..215cb547 --- /dev/null +++ b/change/beachball-1290455d-876a-4e72-821c-6e0bc0750219.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "getPackageInfos should only get repo and CLI options once", + "packageName": "beachball", + "email": "elcraig@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/docs/overview/configuration.md b/docs/overview/configuration.md index 44a1c4ee..8e6cc719 100644 --- a/docs/overview/configuration.md +++ b/docs/overview/configuration.md @@ -13,7 +13,7 @@ There are two types of configurations: 1. repository config 2. package config -## Configuration files +## Repository config `beachball` uses [`cosmiconfig`](https://github.com/davidtheclark/cosmiconfig) to read its configuration, so you can specify configuration in several ways (in addition to CLI arguments). @@ -22,37 +22,48 @@ There are two types of configurations: - `.beachballrc.json` - `beachball.config.js` (CJS or ESM depending on your project setup; explicit `.cjs` or `.mjs` is also supported) -### `beachball.config.js` +It's most common to use a JavaScript file for the repo-level config, since it's the most flexible and allows comments. Usually this file is at the repo root. -In many cases, you'll want to use a JavaScript config file, since this is the most flexible and allows comments. The example below uses JSDoc type annotations to enable intellisense in some editors (these are optional). +The `beachball.config.js` example below uses JSDoc type annotations to enable intellisense in some editors (these are optional). ```js // @ts-check /** @type {import('beachball').BeachallConfig} */ const config = { - key: value, - key2: value2 - key3: value3 + disallowedChangeTypes: ['major'], + changehint: 'Run "yarn change" to generate a change file', + groupChanges: true, }; module.exports = config; ``` -Config files can be placed in either the root of a repo and/or within individual packages (package config overrides the repo config where applicable). For example: +## Package config + +Package-level configuration is currently only supported under the `beachball` key in `package.json`. + +For example, suppose the repo config above is at `beachball.config.js` at the repo root, and there are these other files: ``` packages/ foo/ - src/ package.json - beachball.config.js bar/ - src/ package.json -package.json beachball.config.js +package.json ``` -It's also common to have a repo-level `beachball.config.js` and any individual package overrides (if they're simple) in the `"beachball"` key in the package's `package.json`. +To change the `disallowedChangeTypes` for package `foo`, you could add the following to `packages/foo/package.json`: + +```json +{ + "name": "foo", + "version": "1.0.0", + "beachball": { + "disallowedChangeTypes": null + } +} +``` ## Options diff --git a/src/__e2e__/publishE2E.test.ts b/src/__e2e__/publishE2E.test.ts index bb73f866..0cfdbb5c 100644 --- a/src/__e2e__/publishE2E.test.ts +++ b/src/__e2e__/publishE2E.test.ts @@ -483,7 +483,7 @@ describe('publish command (e2e)', () => { // All git results should still have previous information expect(repo.getCurrentTags()).toEqual(['foo_v1.1.0']); const manifestJson = fs.readFileSync(repo.pathTo('foo.txt')); - expect(manifestJson.toString()).toMatchInlineSnapshot(`"foo"`); + expect(manifestJson.toString()).toEqual('foo'); }); it('publishes multiple packages concurrently respecting the concurrency limit', async () => { diff --git a/src/__fixtures__/registry.ts b/src/__fixtures__/registry.ts index 638534f9..ce8e1a79 100644 --- a/src/__fixtures__/registry.ts +++ b/src/__fixtures__/registry.ts @@ -4,7 +4,7 @@ import execa from 'execa'; import fs from 'fs-extra'; import getPort from 'get-port'; import path from 'path'; -import { tmpdir } from './tmpdir'; +import { removeTempDir, tmpdir } from './tmpdir'; const verdaccioUser = { username: 'fake', @@ -86,13 +86,8 @@ export class Registry { /** Delete the temp directory used for the config file. */ public cleanUp(): void { - try { - this.tempRoot && fs.removeSync(this.tempRoot); - this.tempRoot = undefined; - } catch { - // This can fail on Windows with EBUSY (likely due to the server not being fully shut down - // or all handles released or something). Just ignore it. - } + this.tempRoot && removeTempDir(this.tempRoot); + this.tempRoot = undefined; } private async startWithPort(port: number): Promise { diff --git a/src/__functional__/monorepo/getPackageInfos.test.ts b/src/__functional__/monorepo/getPackageInfos.test.ts index 44c18320..8b4ac125 100644 --- a/src/__functional__/monorepo/getPackageInfos.test.ts +++ b/src/__functional__/monorepo/getPackageInfos.test.ts @@ -92,20 +92,18 @@ describe('getPackageInfos', () => { const repo = singleFactory.cloneRepository(); let packageInfos = getPackageInfos(repo.rootPath); packageInfos = cleanPackageInfos(repo.rootPath, packageInfos); - expect(packageInfos).toMatchInlineSnapshot(` - { - "foo": { - "dependencies": { - "bar": "1.0.0", - "baz": "1.0.0", - }, - "name": "foo", - "packageJsonPath": "package.json", - "private": false, - "version": "1.0.0", + expect(packageInfos).toEqual({ + foo: { + dependencies: { + bar: '1.0.0', + baz: '1.0.0', }, - } - `); + name: 'foo', + packageJsonPath: 'package.json', + private: false, + version: '1.0.0', + }, + }); }); // both yarn and npm define "workspaces" in package.json @@ -113,46 +111,44 @@ describe('getPackageInfos', () => { const repo = monorepoFactory.cloneRepository(); let packageInfos = getPackageInfos(repo.rootPath); packageInfos = cleanPackageInfos(repo.rootPath, packageInfos); - expect(packageInfos).toMatchInlineSnapshot(` - { - "a": { - "name": "a", - "packageJsonPath": "packages/grouped/a/package.json", - "private": false, - "version": "3.1.2", - }, - "b": { - "name": "b", - "packageJsonPath": "packages/grouped/b/package.json", - "private": false, - "version": "3.1.2", + expect(packageInfos).toEqual({ + a: { + name: 'a', + packageJsonPath: 'packages/grouped/a/package.json', + private: false, + version: '3.1.2', + }, + b: { + name: 'b', + packageJsonPath: 'packages/grouped/b/package.json', + private: false, + version: '3.1.2', + }, + bar: { + dependencies: { + baz: '^1.3.4', }, - "bar": { - "dependencies": { - "baz": "^1.3.4", - }, - "name": "bar", - "packageJsonPath": "packages/bar/package.json", - "private": false, - "version": "1.3.4", + name: 'bar', + packageJsonPath: 'packages/bar/package.json', + private: false, + version: '1.3.4', + }, + baz: { + name: 'baz', + packageJsonPath: 'packages/baz/package.json', + private: false, + version: '1.3.4', + }, + foo: { + dependencies: { + bar: '^1.3.4', }, - "baz": { - "name": "baz", - "packageJsonPath": "packages/baz/package.json", - "private": false, - "version": "1.3.4", - }, - "foo": { - "dependencies": { - "bar": "^1.3.4", - }, - "name": "foo", - "packageJsonPath": "packages/foo/package.json", - "private": false, - "version": "1.0.0", - }, - } - `); + name: 'foo', + packageJsonPath: 'packages/foo/package.json', + private: false, + version: '1.0.0', + }, + }); }); it('works in pnpm monorepo', () => { @@ -162,16 +158,14 @@ describe('getPackageInfos', () => { fs.writeFileSync(repo.pathTo('pnpm-workspace.yaml'), 'packages: ["packages/*", "packages/grouped/*"]'); const rootPackageInfos = getPackageInfos(repo.rootPath); - expect(getPackageNamesAndPaths(repo.rootPath, rootPackageInfos)).toMatchInlineSnapshot(` - { - "a": "packages/grouped/a/package.json", - "b": "packages/grouped/b/package.json", - "bar": "packages/bar/package.json", - "baz": "packages/baz/package.json", - "foo": "packages/foo/package.json", - "pnpm-monorepo": "package.json", - } - `); + expect(getPackageNamesAndPaths(repo.rootPath, rootPackageInfos)).toEqual({ + a: 'packages/grouped/a/package.json', + b: 'packages/grouped/b/package.json', + bar: 'packages/bar/package.json', + baz: 'packages/baz/package.json', + foo: 'packages/foo/package.json', + 'pnpm-monorepo': 'package.json', + }); }); it('works in rush monorepo', () => { @@ -182,16 +176,14 @@ describe('getPackageInfos', () => { }); const rootPackageInfos = getPackageInfos(repo.rootPath); - expect(getPackageNamesAndPaths(repo.rootPath, rootPackageInfos)).toMatchInlineSnapshot(` - { - "a": "packages/grouped/a/package.json", - "b": "packages/grouped/b/package.json", - "bar": "packages/bar/package.json", - "baz": "packages/baz/package.json", - "foo": "packages/foo/package.json", - "rush-monorepo": "package.json", - } - `); + expect(getPackageNamesAndPaths(repo.rootPath, rootPackageInfos)).toEqual({ + a: 'packages/grouped/a/package.json', + b: 'packages/grouped/b/package.json', + bar: 'packages/bar/package.json', + baz: 'packages/baz/package.json', + foo: 'packages/foo/package.json', + 'rush-monorepo': 'package.json', + }); }); it('works in lerna monorepo', () => { @@ -200,15 +192,13 @@ describe('getPackageInfos', () => { fs.writeJSONSync(repo.pathTo('lerna.json'), { packages: ['packages/*', 'packages/grouped/*'] }); const rootPackageInfos = getPackageInfos(repo.rootPath); - expect(getPackageNamesAndPaths(repo.rootPath, rootPackageInfos)).toMatchInlineSnapshot(` - { - "a": "packages/grouped/a/package.json", - "b": "packages/grouped/b/package.json", - "bar": "packages/bar/package.json", - "baz": "packages/baz/package.json", - "foo": "packages/foo/package.json", - } - `); + expect(getPackageNamesAndPaths(repo.rootPath, rootPackageInfos)).toEqual({ + a: 'packages/grouped/a/package.json', + b: 'packages/grouped/b/package.json', + bar: 'packages/bar/package.json', + baz: 'packages/baz/package.json', + foo: 'packages/foo/package.json', + }); }); it('works multi-workspace monorepo', () => { @@ -216,46 +206,40 @@ describe('getPackageInfos', () => { // For this test, only snapshot the package names and paths const rootPackageInfos = getPackageInfos(repo.rootPath); - expect(getPackageNamesAndPaths(repo.rootPath, rootPackageInfos)).toMatchInlineSnapshot(` - { - "@workspace-a/a": "workspace-a/packages/grouped/a/package.json", - "@workspace-a/b": "workspace-a/packages/grouped/b/package.json", - "@workspace-a/bar": "workspace-a/packages/bar/package.json", - "@workspace-a/baz": "workspace-a/packages/baz/package.json", - "@workspace-a/foo": "workspace-a/packages/foo/package.json", - "@workspace-a/monorepo-fixture": "workspace-a/package.json", - "@workspace-b/a": "workspace-b/packages/grouped/a/package.json", - "@workspace-b/b": "workspace-b/packages/grouped/b/package.json", - "@workspace-b/bar": "workspace-b/packages/bar/package.json", - "@workspace-b/baz": "workspace-b/packages/baz/package.json", - "@workspace-b/foo": "workspace-b/packages/foo/package.json", - "@workspace-b/monorepo-fixture": "workspace-b/package.json", - } - `); + expect(getPackageNamesAndPaths(repo.rootPath, rootPackageInfos)).toEqual({ + '@workspace-a/a': 'workspace-a/packages/grouped/a/package.json', + '@workspace-a/b': 'workspace-a/packages/grouped/b/package.json', + '@workspace-a/bar': 'workspace-a/packages/bar/package.json', + '@workspace-a/baz': 'workspace-a/packages/baz/package.json', + '@workspace-a/foo': 'workspace-a/packages/foo/package.json', + '@workspace-a/monorepo-fixture': 'workspace-a/package.json', + '@workspace-b/a': 'workspace-b/packages/grouped/a/package.json', + '@workspace-b/b': 'workspace-b/packages/grouped/b/package.json', + '@workspace-b/bar': 'workspace-b/packages/bar/package.json', + '@workspace-b/baz': 'workspace-b/packages/baz/package.json', + '@workspace-b/foo': 'workspace-b/packages/foo/package.json', + '@workspace-b/monorepo-fixture': 'workspace-b/package.json', + }); const workspaceARoot = repo.pathTo('workspace-a'); const packageInfosA = getPackageInfos(workspaceARoot); - expect(getPackageNamesAndPaths(workspaceARoot, packageInfosA)).toMatchInlineSnapshot(` - { - "@workspace-a/a": "packages/grouped/a/package.json", - "@workspace-a/b": "packages/grouped/b/package.json", - "@workspace-a/bar": "packages/bar/package.json", - "@workspace-a/baz": "packages/baz/package.json", - "@workspace-a/foo": "packages/foo/package.json", - } - `); + expect(getPackageNamesAndPaths(workspaceARoot, packageInfosA)).toEqual({ + '@workspace-a/a': 'packages/grouped/a/package.json', + '@workspace-a/b': 'packages/grouped/b/package.json', + '@workspace-a/bar': 'packages/bar/package.json', + '@workspace-a/baz': 'packages/baz/package.json', + '@workspace-a/foo': 'packages/foo/package.json', + }); const workspaceBRoot = repo.pathTo('workspace-b'); const packageInfosB = getPackageInfos(workspaceBRoot); - expect(getPackageNamesAndPaths(workspaceBRoot, packageInfosB)).toMatchInlineSnapshot(` - { - "@workspace-b/a": "packages/grouped/a/package.json", - "@workspace-b/b": "packages/grouped/b/package.json", - "@workspace-b/bar": "packages/bar/package.json", - "@workspace-b/baz": "packages/baz/package.json", - "@workspace-b/foo": "packages/foo/package.json", - } - `); + expect(getPackageNamesAndPaths(workspaceBRoot, packageInfosB)).toEqual({ + '@workspace-b/a': 'packages/grouped/a/package.json', + '@workspace-b/b': 'packages/grouped/b/package.json', + '@workspace-b/bar': 'packages/bar/package.json', + '@workspace-b/baz': 'packages/baz/package.json', + '@workspace-b/foo': 'packages/foo/package.json', + }); }); it('throws if multiple packages have the same name in multi-workspace monorepo', () => { diff --git a/src/monorepo/getPackageInfos.ts b/src/monorepo/getPackageInfos.ts index 6cd27b63..f74e64be 100644 --- a/src/monorepo/getPackageInfos.ts +++ b/src/monorepo/getPackageInfos.ts @@ -5,10 +5,11 @@ import { listAllTrackedFiles, findPackageRoot, findProjectRoot, - type WorkspaceInfo, + type PackageInfo as WSPackageInfo, + type PackageInfos as WSPackageInfos, } from 'workspace-tools'; -import type { PackageInfos, PackageJson } from '../types/PackageInfo'; -import { infoFromPackageJson } from './infoFromPackageJson'; +import type { PackageInfos } from '../types/PackageInfo'; +import { getPackageInfosWithOptions } from '../options/getPackageInfosWithOptions'; /** * Get a mapping from package name to package info for all packages in the workspace @@ -18,71 +19,56 @@ export function getPackageInfos(cwd: string): PackageInfos { const projectRoot = findProjectRoot(cwd); const packageRoot = findPackageRoot(cwd); - return ( - (projectRoot && getPackageInfosFromWorkspace(projectRoot)) || - (projectRoot && getPackageInfosFromNonWorkspaceMonorepo(projectRoot)) || - (packageRoot && getPackageInfosFromSingleRepo(packageRoot)) || - {} - ); + let wsPackageInfos: WSPackageInfo[] | undefined; + if (projectRoot) { + wsPackageInfos = getPackageInfosFromWorkspace(projectRoot) || getPackageInfosFromNonWorkspaceMonorepo(projectRoot); + } + if (!wsPackageInfos && packageRoot) { + wsPackageInfos = [readWsPackageInfo(path.join(packageRoot, 'package.json'))]; + } + + return wsPackageInfos ? getPackageInfosWithOptions(wsPackageInfos) : {}; } -function getPackageInfosFromWorkspace(projectRoot: string): PackageInfos | undefined { - let workspacePackages: WorkspaceInfo | undefined; +function getPackageInfosFromWorkspace(projectRoot: string): WSPackageInfo[] | undefined { + let workspacePackages: WSPackageInfo[] | undefined; try { // first try using the workspace provided packages (if available) - workspacePackages = getWorkspacePackages(projectRoot); + workspacePackages = getWorkspacePackages(projectRoot).map(pkg => pkg.packageJson); } catch { // not a recognized workspace from workspace-tools } - if (!workspacePackages?.length) { - return; - } - - const packageInfos: PackageInfos = {}; - - for (const { path: packagePath, packageJson } of workspacePackages) { - const packageJsonPath = path.join(packagePath, 'package.json'); - - try { - packageInfos[packageJson.name] = infoFromPackageJson(packageJson, packageJsonPath); - } catch (e) { - // Pass, the package.json is invalid - console.warn(`Problem processing ${packageJsonPath}: ${e}`); - } - } - - return packageInfos; + return workspacePackages?.length ? workspacePackages : undefined; } -function getPackageInfosFromNonWorkspaceMonorepo(projectRoot: string): PackageInfos | undefined { +function getPackageInfosFromNonWorkspaceMonorepo(projectRoot: string): WSPackageInfo[] | undefined { const packageJsonFiles = listAllTrackedFiles(['**/package.json', 'package.json'], projectRoot); if (!packageJsonFiles.length) { return; } - const packageInfos: PackageInfos = {}; - + const wsPackageInfos: WSPackageInfos = {}; let hasError = false; - for (const packageJsonPath of packageJsonFiles) { + for (const file of packageJsonFiles) { try { - const packageJsonFullPath = path.join(projectRoot, packageJsonPath); - const packageJson = fs.readJSONSync(packageJsonFullPath) as PackageJson; - if (!packageInfos[packageJson.name]) { - packageInfos[packageJson.name] = infoFromPackageJson(packageJson, packageJsonFullPath); + const packageJson = readWsPackageInfo(path.join(projectRoot, file)); + + if (!wsPackageInfos[packageJson.name]) { + wsPackageInfos[packageJson.name] = packageJson; } else { console.error( `ERROR: Two packages have the same name "${packageJson.name}". Please rename one of these packages:\n` + - `- ${path.relative(projectRoot, packageInfos[packageJson.name].packageJsonPath)}\n` + - `- ${packageJsonPath}` + `- ${path.relative(projectRoot, wsPackageInfos[packageJson.name].packageJsonPath)}\n` + + `- ${path.relative(projectRoot, packageJson.packageJsonPath)}` ); // Keep going so we can log all the errors hasError = true; } } catch (e) { // Pass, the package.json is invalid - console.warn(`Problem processing ${packageJsonPath}: ${e}`); + console.warn(`Problem processing ${file}: ${e}`); } } @@ -90,13 +76,14 @@ function getPackageInfosFromNonWorkspaceMonorepo(projectRoot: string): PackageIn throw new Error('Duplicate package names found (see above for details)'); } - return packageInfos; + return Object.values(wsPackageInfos); } -function getPackageInfosFromSingleRepo(packageRoot: string): PackageInfos { - const packageInfos: PackageInfos = {}; - const packageJsonFullPath = path.resolve(packageRoot, 'package.json'); - const packageJson = fs.readJSONSync(packageJsonFullPath) as PackageJson; - packageInfos[packageJson.name] = infoFromPackageJson(packageJson, packageJsonFullPath); - return packageInfos; +function readWsPackageInfo(packageJsonPath: string): WSPackageInfo { + return { + // this is actually the properties of WSPackageInfo except the packageJsonPath, but using omit + // messes things up due to the index signature... + ...(fs.readJSONSync(packageJsonPath) as WSPackageInfo), + packageJsonPath, + }; } diff --git a/src/monorepo/infoFromPackageJson.ts b/src/monorepo/infoFromPackageJson.ts deleted file mode 100644 index 05211e19..00000000 --- a/src/monorepo/infoFromPackageJson.ts +++ /dev/null @@ -1,19 +0,0 @@ -import path from 'path'; -import type { PackageInfo, PackageJson } from '../types/PackageInfo'; -import { getPackageOptions, getCombinedPackageOptions } from '../options/getPackageOptions'; - -export function infoFromPackageJson(packageJson: PackageJson, packageJsonPath: string): PackageInfo { - const packageOptions = getPackageOptions(path.dirname(packageJsonPath)); - return { - name: packageJson.name, - version: packageJson.version, - packageJsonPath, - dependencies: packageJson.dependencies, - devDependencies: packageJson.devDependencies, - peerDependencies: packageJson.peerDependencies, - optionalDependencies: packageJson.optionalDependencies, - private: packageJson.private !== undefined ? packageJson.private : false, - combinedOptions: getCombinedPackageOptions(packageOptions), - packageOptions, - }; -} diff --git a/src/options/getOptions.ts b/src/options/getOptions.ts index 1f59b772..df61008d 100644 --- a/src/options/getOptions.ts +++ b/src/options/getOptions.ts @@ -8,5 +8,6 @@ import { getDefaultOptions } from './getDefaultOptions'; */ export function getOptions(argv: string[]): BeachballOptions { const cliOptions = getCliOptions(argv); + // TODO: proper recursive merging return { ...getDefaultOptions(), ...getRepoOptions(cliOptions), ...cliOptions }; } diff --git a/src/options/getPackageInfosWithOptions.ts b/src/options/getPackageInfosWithOptions.ts new file mode 100644 index 00000000..be01ef4e --- /dev/null +++ b/src/options/getPackageInfosWithOptions.ts @@ -0,0 +1,49 @@ +import type { PackageInfo as WSPackageInfo } from 'workspace-tools'; +import type { PackageOptions } from '../types/BeachballOptions'; +import { getCliOptions } from './getCliOptions'; +import { getRepoOptions } from './getRepoOptions'; +import { getDefaultOptions } from './getDefaultOptions'; +import { env } from '../env'; +import type { PackageInfos } from '../types/PackageInfo'; + +/** + * Fill in options to convert `workspace-tools` `PackageInfos` to the format used in this repo, + * which includes merged beachball options. + */ +export function getPackageInfosWithOptions(wsPackageInfos: WSPackageInfo[]): PackageInfos { + const packageInfos: PackageInfos = {}; + + // Get the CLI and repo options once instead of re-calculating for every package. + // TODO: pass the unmerged options in instead of re-calculating... + const defaultOptions = getDefaultOptions(); + // Don't use options from process.argv or the beachball repo in tests + const cliOptions = !env.isJest ? getCliOptions(process.argv) : undefined; + const repoOptions = cliOptions?.path ? getRepoOptions(cliOptions) : undefined; + + for (const packageJson of wsPackageInfos) { + // Package-level JS config files aren't currently supported - https://github.com/microsoft/beachball/issues/1021 + // (just the "beachball" key in package.json) + const packageOptions = (packageJson.beachball || {}) as Partial; + + packageInfos[packageJson.name] = { + name: packageJson.name, + version: packageJson.version, + packageJsonPath: packageJson.packageJsonPath, + dependencies: packageJson.dependencies, + devDependencies: packageJson.devDependencies, + peerDependencies: packageJson.peerDependencies, + optionalDependencies: packageJson.optionalDependencies, + private: packageJson.private !== undefined ? packageJson.private : false, + // TODO: proper recursive merging + combinedOptions: { + ...defaultOptions, + ...repoOptions, + ...packageOptions, + ...cliOptions, + }, + packageOptions, + }; + } + + return packageInfos; +} diff --git a/src/options/getPackageOptions.ts b/src/options/getPackageOptions.ts deleted file mode 100644 index b661f694..00000000 --- a/src/options/getPackageOptions.ts +++ /dev/null @@ -1,38 +0,0 @@ -import { cosmiconfigSync } from 'cosmiconfig'; -import type { PackageOptions } from '../types/BeachballOptions'; -import { getCliOptions } from './getCliOptions'; -import { getRepoOptions } from './getRepoOptions'; -import { getDefaultOptions } from './getDefaultOptions'; -import path from 'path'; -import { env } from '../env'; - -/** - * Gets all package level options (default + root options + package options + cli options) - * This function inherits packageOptions from the repoOptions - */ -export function getCombinedPackageOptions(actualPackageOptions: Partial): PackageOptions { - const defaultOptions = getDefaultOptions(); - // Don't use options from process.argv or the beachball repo in tests - const cliOptions = !env.isJest && getCliOptions(process.argv); - const repoOptions = cliOptions && getRepoOptions(cliOptions); - return { - ...defaultOptions, - ...repoOptions, - ...actualPackageOptions, - ...cliOptions, - }; -} - -/** - * Gets all the package options from the configuration file of the package itself without inheritance - */ -export function getPackageOptions(packagePath: string): Partial { - const configExplorer = cosmiconfigSync('beachball', { cache: false }); - try { - const results = configExplorer.load(path.join(packagePath, 'package.json')); - return (results?.config as PackageOptions) || {}; - } catch { - // File does not exist, returns the default packageOptions - return {}; - } -} diff --git a/src/types/BeachballOptions.ts b/src/types/BeachballOptions.ts index ad2ebab6..2727b114 100644 --- a/src/types/BeachballOptions.ts +++ b/src/types/BeachballOptions.ts @@ -200,7 +200,6 @@ export interface PackageOptions { disallowedChangeTypes: ChangeType[] | null; tag: string | null; defaultNpmTag: string; - changeFilePrompt?: ChangeFilePromptOptions; /** * Disable publishing a particular package. * (Does NOT work to enable publishing a package that wouldn't otherwise be published.) diff --git a/src/types/PackageInfo.ts b/src/types/PackageInfo.ts index 85812b10..b34ca475 100644 --- a/src/types/PackageInfo.ts +++ b/src/types/PackageInfo.ts @@ -47,10 +47,10 @@ export interface PackageInfo { optionalDependencies?: PackageDeps; private: boolean; - /** options that are combined from the root configuration */ + /** merged default, repo, package, and CLI options */ combinedOptions: PackageOptions; - /** options that are SPECIFIC to the package from its configuration file (might be nothing) */ + /** options that are SPECIFIC to the package from the `beachball` key in its package.json (might be nothing) */ packageOptions: Partial; } From 40b62b63904125b12f629b4ebd6a9abbc3b972fe Mon Sep 17 00:00:00 2001 From: Elizabeth Craig Date: Mon, 25 Nov 2024 18:12:19 -0800 Subject: [PATCH 2/2] fix prompt --- .../beachball-1290455d-876a-4e72-821c-6e0bc0750219.json | 2 +- src/__tests__/changefile/getQuestionsForPackage.test.ts | 2 +- src/__tests__/changefile/promptForChange.test.ts | 5 +---- src/changefile/getQuestionsForPackage.ts | 8 +++----- src/changefile/promptForChange.ts | 2 +- 5 files changed, 7 insertions(+), 12 deletions(-) diff --git a/change/beachball-1290455d-876a-4e72-821c-6e0bc0750219.json b/change/beachball-1290455d-876a-4e72-821c-6e0bc0750219.json index 215cb547..a73403a4 100644 --- a/change/beachball-1290455d-876a-4e72-821c-6e0bc0750219.json +++ b/change/beachball-1290455d-876a-4e72-821c-6e0bc0750219.json @@ -1,6 +1,6 @@ { "type": "patch", - "comment": "getPackageInfos should only get repo and CLI options once", + "comment": "getPackageInfos should only get repo and CLI options once. Also clarify in types and logic that changeFilePrompt can't be specified at package level.", "packageName": "beachball", "email": "elcraig@microsoft.com", "dependentChangeType": "patch" diff --git a/src/__tests__/changefile/getQuestionsForPackage.test.ts b/src/__tests__/changefile/getQuestionsForPackage.test.ts index 5f73824d..b4230018 100644 --- a/src/__tests__/changefile/getQuestionsForPackage.test.ts +++ b/src/__tests__/changefile/getQuestionsForPackage.test.ts @@ -150,7 +150,7 @@ describe('getQuestionsForPackage', () => { const questions = getQuestionsForPackage({ ...defaultQuestionsParams, - packageInfos: makePackageInfos({ [pkg]: { combinedOptions: { changeFilePrompt: { changePrompt } } } }), + options: { ...defaultQuestionsParams.options, changeFilePrompt: { changePrompt } }, }); expect(questions).toEqual(customQuestions); diff --git a/src/__tests__/changefile/promptForChange.test.ts b/src/__tests__/changefile/promptForChange.test.ts index 33e1bd7b..0d5eaad8 100644 --- a/src/__tests__/changefile/promptForChange.test.ts +++ b/src/__tests__/changefile/promptForChange.test.ts @@ -135,11 +135,8 @@ describe('promptForChange', () => { }; const changeFilesPromise = promptForChange({ ...defaultParams(), + options: { message: 'message', changeFilePrompt }, changedPackages: ['foo', 'bar'], - packageInfos: makePackageInfos({ - foo: { combinedOptions: { changeFilePrompt } }, - bar: { combinedOptions: { changeFilePrompt } }, - }), }); await waitForPrompt(); diff --git a/src/changefile/getQuestionsForPackage.ts b/src/changefile/getQuestionsForPackage.ts index defec79c..c571b62c 100644 --- a/src/changefile/getQuestionsForPackage.ts +++ b/src/changefile/getQuestionsForPackage.ts @@ -14,11 +14,10 @@ export function getQuestionsForPackage(params: { pkg: string; packageInfos: PackageInfos; packageGroups: PackageGroups; - options: Pick; + options: Pick; recentMessages: string[]; }): prompts.PromptObject[] | undefined { - const { pkg, packageInfos, options, recentMessages } = params; - const packageInfo = packageInfos[pkg]; + const { pkg, options, recentMessages } = params; const changeTypePrompt = getChangeTypePrompt(params); if (!changeTypePrompt) { @@ -30,8 +29,7 @@ export function getQuestionsForPackage(params: { description: !options.message ? getDescriptionPrompt(recentMessages) : undefined, }; - const questions = - packageInfo.combinedOptions.changeFilePrompt?.changePrompt?.(defaultPrompt, pkg) || Object.values(defaultPrompt); + const questions = options.changeFilePrompt?.changePrompt?.(defaultPrompt, pkg) || Object.values(defaultPrompt); return questions.filter((q): q is prompts.PromptObject => !!q); } diff --git a/src/changefile/promptForChange.ts b/src/changefile/promptForChange.ts index e3d68274..e71320b0 100644 --- a/src/changefile/promptForChange.ts +++ b/src/changefile/promptForChange.ts @@ -17,7 +17,7 @@ export async function promptForChange(params: { packageGroups: PackageGroups; recentMessages: string[]; email: string | null; - options: Pick; + options: Pick; }): Promise { const { changedPackages, email, options } = params; if (!changedPackages.length) {