Skip to content

Commit f0cb892

Browse files
ecraig12345ecraig12345
authored
Improve perf of readChangeFiles, and improve object stringifying in errors (#1099)
1 parent 69f0bbe commit f0cb892

10 files changed

Lines changed: 80 additions & 44 deletions

‎.gitignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
logs
22
*.log
3+
coverage/
34
dist
45
node_modules/
56
.DS_Store

‎change/beachball-508e4cfc-b065-4957-80fe-ab0f71bfa1c8.json‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"type": "patch",
3+
"comment": "Improve perf of readChangeFiles, and improve object stringifying in errors",
4+
"packageName": "beachball",
5+
"email": "elcraig@microsoft.com",
6+
"dependentChangeType": "patch"
7+
}

‎docs/overview/configuration.md‎

Lines changed: 29 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -71,34 +71,35 @@ For the latest full list of supported options, see `RepoOptions` [in this file](
7171

7272
"Applies to" indicates where the settings can be specified: repo-level config or package-level config.
7373

74-
| Option | Type | Default | Applies to | Description |
75-
| ----------------------- | ------------------------------ | -------------- | ------------- | ----------------------------------------------------------------------------------------------- |
76-
| `access` | `'public'` or `'restricted'` | `'restricted'` | repo | publish access level for scoped package names (e.g. `@foo/bar`) |
77-
| `branch` | `string` | [see notes][5] | repo | target branch; [see notes][5] |
78-
| `bumpDeps` | `boolean` | `true` | repo | bump dependent packages during publish (if B is bumped, and A depends on B, also bump A) |
79-
| `changeFilePrompt` | [`ChangeFilePromptOptions`][1] | | repo | customize the prompt for change files (can be used to add custom fields) |
80-
| `changehint` | `string` | | repo | hint message for when change files are not detected but required |
81-
| `changeDir` | `string` | `change` | repo | directory where change files are stored (relative to repo root) |
82-
| `changelog` | [`ChangelogOptions`][2] | | repo | changelog rendering and grouping options |
83-
| `defaultNpmTag` | `string` | `'latest'` | repo, package | the default dist-tag used for NPM publish |
84-
| `disallowedChangeTypes` | `string[]` | | repo, package | what change types are disallowed |
85-
| `fetch` | `boolean` | `true` | repo | fetch from remote before doing diff comparisons |
86-
| `generateChangelog` | `boolean \| 'md' \| 'json'` | `true` | repo | whether to generate `CHANGELOG.md/json` (`'md'` or `'json'` to generate only that type) |
87-
| `gitTags` | `boolean` | `true` | repo, package | whether to create git tags for published packages (eg: foo_v1.0.1) |
88-
| `groups` | [`VersionGroupOptions[]`][3] | | repo | bump these packages together ([see details][3]) |
89-
| `groupChanges` | `boolean` | `false` | repo | write multiple changes to a single changefile |
90-
| `hooks` | [`HooksOptions`][4] | | repo | hooks for custom pre/post publish actions |
91-
| `ignorePatterns` | `string[]` | | repo | ignore changes in files matching these glob patterns ([see notes][6]) |
92-
| `package` | `string` | | repo | specifies which package the command relates to (overrides change detection based on `git diff`) |
93-
| `prereleasePrefix` | `string` | | repo | prerelease prefix for packages that are specified to receive a prerelease bump |
94-
| `publish` | `boolean` | `true` | repo | whether to publish to npm registry |
95-
| `push` | `boolean` | `true` | repo | whether to push to the remote git branch |
96-
| `registry` | `string` | | repo | target NPM registry to publish |
97-
| `retries` | `number` | `3` | repo | number of retries for a package publish before failing |
98-
| `scope` | `string[]` | | repo | only consider package paths matching these patterns ([see details](#scoping)) |
99-
| `shouldPublish` | `false \| undefined` | | package | manually disable publishing of a package by beachball (does not work to force publishing) |
100-
| `tag` | `string` | `'latest'` | repo, package | dist-tag for npm when published |
101-
| `transform` | [`TransformOptions`][4] | | repo | transformations for change files |
74+
<!-- prettier-ignore -->
75+
| Option | Type | Default | Applies to | Description |
76+
| ------ | ---- | ------- | ---------- | ----------- |
77+
| `access` | `'public'` or `'restricted'` | `'restricted'` | repo | publish access level for scoped package names (e.g. `@foo/bar`) |
78+
| `branch` | `string` | [see notes][5] | repo | target branch; [see notes][5] |
79+
| `bumpDeps` | `boolean` | `true` | repo | bump dependent packages during publish (if B is bumped, and A depends on B, also bump A) |
80+
| `changeFilePrompt` | [`ChangeFilePromptOptions`][1] | | repo | customize the prompt for change files (can be used to add custom fields) |
81+
| `changehint` | `string` | | repo | hint message for when change files are not detected but required |
82+
| `changeDir` | `string` | `change` | repo | directory where change files are stored (relative to repo root) |
83+
| `changelog` | [`ChangelogOptions`][2] | | repo | changelog rendering and grouping options |
84+
| `defaultNpmTag` | `string` | `'latest'` | repo, package | the default dist-tag used for NPM publish |
85+
| `disallowedChangeTypes` | `string[]` | | repo, package | what change types are disallowed |
86+
| `fetch` | `boolean` | `true` | repo | fetch from remote before doing diff comparisons |
87+
| `generateChangelog` | `boolean \| 'md' \| 'json'` | `true` | repo | whether to generate `CHANGELOG.md/json` (`'md'` or `'json'` to generate only that type) |
88+
| `gitTags` | `boolean` | `true` | repo, package | whether to create git tags for published packages (eg: foo_v1.0.1) |
89+
| `groups` | [`VersionGroupOptions[]`][3] | | repo | bump these packages together ([see details][3]) |
90+
| `groupChanges` | `boolean` | `false` | repo | write multiple changes to a single changefile |
91+
| `hooks` | [`HooksOptions`][4] | | repo | hooks for custom pre/post publish actions |
92+
| `ignorePatterns` | `string[]` | | repo | ignore changes in files matching these glob patterns ([see notes][6]) |
93+
| `package` | `string` | | repo | specifies which package the command relates to (overrides change detection based on `git diff`) |
94+
| `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 |
95+
| `publish` | `boolean` | `true` | repo | whether to publish to npm registry |
96+
| `push` | `boolean` | `true` | repo | whether to push to the remote git branch |
97+
| `registry` | `string` | | repo | target NPM registry to publish |
98+
| `retries` | `number` | `3` | repo | number of retries for a package publish before failing |
99+
| `scope` | `string[]` | | repo | only consider package paths matching these patterns ([see details](#scoping)) |
100+
| `shouldPublish` | `false \| undefined` | | package | manually disable publishing of a package by beachball (does not work to force publishing) |
101+
| `tag` | `string` | `'latest'` | repo, package | dist-tag for npm when published |
102+
| `transform` | [`TransformOptions`][4] | | repo | transformations for change files |
102103

103104
[1]: https://github.com/microsoft/beachball/blob/main/src/types/ChangeFilePrompt.ts
104105
[2]: https://github.com/microsoft/beachball/blob/main/src/types/ChangelogOptions.ts

‎src/bump/bumpPackageInfoVersion.ts‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import type { BeachballOptions } from '../types/BeachballOptions';
44

55
/**
66
* Bumps an individual package version based on the change type.
7-
* **This mutates `info.version`!**
7+
* **This mutates `info.version` and `bumpInfo.modifiedPackages`!**
88
*/
99
export function bumpPackageInfoVersion(
1010
pkgName: string,
@@ -23,10 +23,7 @@ export function bumpPackageInfoVersion(
2323
console.log(`Skipping bumping private package "${pkgName}"`);
2424
} else {
2525
// Ensure we can bump the correct versions
26-
let bumpAsPrerelease = false;
27-
if (options.prereleasePrefix && !['premajor', 'preminor', 'prepatch'].includes(changeType)) {
28-
bumpAsPrerelease = true;
29-
}
26+
const bumpAsPrerelease = !!options.prereleasePrefix && !['premajor', 'preminor', 'prepatch'].includes(changeType);
3027

3128
// Version should be updated
3229
info.version = semver.inc(
@@ -36,6 +33,7 @@ export function bumpPackageInfoVersion(
3633
options.prereleasePrefix || undefined,
3734
options.identifierBase
3835
) as string;
36+
3937
modifiedPackages.add(pkgName);
4038
}
4139
}

‎src/bump/callHook.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ export async function callHook(
2626
await callHookInternal(packageInfos[pkg]);
2727
}
2828
} else {
29+
// TODO: reuse the graph across hooks if possible (depends on if internal state is used)
2930
const packageGraph = getPackageGraph(affectedPackages, packageInfos, callHookInternal);
3031

3132
await packageGraph.run({

‎src/changefile/getDisallowedChangeTypes.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,11 @@
11
import type { ChangeType } from '../types/ChangeInfo';
22
import type { PackageGroups, PackageInfos } from '../types/PackageInfo';
33

4+
/**
5+
* Get `disallowedChangeTypes` from the package's group if relevant.
6+
* Otherwise, get it from the package's own config or the repo config.
7+
*/
8+
// TODO: merge this in getPackageInfosWithOptions instead
49
export function getDisallowedChangeTypes(
510
packageName: string,
611
packageInfos: PackageInfos,

‎src/changefile/readChangeFiles.ts‎

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -45,15 +45,10 @@ export function readChangeFiles(options: BeachballOptions, packageInfos: Package
4545
filteredChangeFiles = allChangeFiles.filter(fileName => changeFilesSinceFromRef?.includes(fileName));
4646
}
4747

48-
try {
49-
// sort the change files by modified time. Most recent modified file comes first.
50-
filteredChangeFiles.sort(
51-
(f1, f2) =>
52-
fs.statSync(path.join(changePath, f2)).mtime.getTime() - fs.statSync(path.join(changePath, f1)).mtime.getTime()
53-
);
54-
} catch (err) {
55-
console.warn('Failed to sort change files', err);
56-
}
48+
// sort the change files by modified time. Most recent modified file comes first.
49+
filteredChangeFiles.sort(
50+
(f1, f2) => getMtime({ changePath, changeFile: f2 }) - getMtime({ changePath, changeFile: f1 })
51+
);
5752

5853
const changeSet: ChangeSet = [];
5954

@@ -111,3 +106,25 @@ export function readChangeFiles(options: BeachballOptions, packageInfos: Package
111106

112107
return changeSet;
113108
}
109+
110+
const mtimeCache: Record<string, number> = {};
111+
112+
/**
113+
* Get a file's modification time with caching. (Usually the caching won't matter, but it might
114+
* in large repos with many change files.)
115+
*/
116+
function getMtime(params: { changePath: string; changeFile: string }) {
117+
const cached = mtimeCache[params.changeFile];
118+
if (cached !== undefined) {
119+
return cached;
120+
}
121+
122+
try {
123+
const mtime = fs.statSync(path.join(params.changePath, params.changeFile)).mtime.getTime();
124+
mtimeCache[params.changeFile] = mtime;
125+
return mtime;
126+
} catch (err) {
127+
mtimeCache[params.changeFile] = 0;
128+
return 0;
129+
}
130+
}

‎src/logging/singleLineStringify.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,5 +3,8 @@
33
* (similar to `JSON.stringify(obj, null, 2)` but without the line breaks).
44
*/
55
export function singleLineStringify(obj: unknown): string {
6-
return JSON.stringify(obj, null, 2).replace(/\n\s*/g, ' ');
6+
return JSON.stringify(obj, null, 2)
7+
.replace(/\n\s*/g, ' ')
8+
.replace(/: \[ /g, ': [')
9+
.replace(/ \]([ ,])/g, ']$1');
710
}

‎src/options/getPackageInfosWithOptions.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ export function getPackageInfosWithOptions(
4747
// (just the "beachball" key in package.json).
4848
// If the package has no specific options (most common), reuse the pre-merged object for performance.
4949
const packageOptions = packageJson.beachball as Partial<PackageOptions> | undefined;
50+
// TODO: merge group options too (group disallowedChangeTypes currently override package)
5051
const combinedOptions = packageOptions
5152
? _mergePackageOptions({ defaultOptions, repoOptions, cliOptions, packageOptions })
5253
: { ...preMergedOptions };

‎src/validation/isValidGroupOptions.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ export function isValidGroupOptions(groups: VersionGroupOptions[]): boolean {
77
// Values that violate types could happen in a user-provided object
88
if (!Array.isArray(groups)) {
99
console.error(
10-
'ERROR: Expected "groups" configuration setting to be an array. Received:\n' + JSON.stringify(groups)
10+
'ERROR: Expected "groups" configuration setting to be an array. Received:\n' + singleLineStringify(groups)
1111
);
1212
return false;
1313
}
@@ -21,6 +21,8 @@ export function isValidGroupOptions(groups: VersionGroupOptions[]): boolean {
2121
return false;
2222
}
2323

24+
// TODO: validate disallowedChangeTypes? (they're not currently validated anywhere)
25+
2426
return true;
2527
}
2628

0 commit comments

Comments
 (0)
⚡