From ee0244f25a1cf3307640fc83aaf1ded4f8cb13d6 Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Fri, 15 May 2026 12:20:50 -0700 Subject: [PATCH 01/21] Add pure-function library primitives for bundle-size comparison MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lays the new library shape down alongside the existing AdoSizeComparator path so nothing breaks yet. The migration off the class-based path lands in follow-up commits. New files: - library/bundleSize/types.ts gains BundleData / BundlesComparison / PackageComparison / AnalyzerJsonByPackage. Old BundleMetric / BundleSummaries / BundleComparison remain temporarily. - library/bundleSize/sourcePackageFromAnalyzerPath.ts — classifier for the nested `/analyzer.json` artifact layout. (No `__` encoding; the flatten was tried and reverted under AB#73299.) - library/bundleSize/compareJsonReports.ts and compareJsonReportsByPackage.ts — pure-function comparison primitives. Field-presence on each entry encodes added/removed (vs the old "log and skip" behavior in compareBundles.ts). - library/bundleSize/extractAnalyzerJsonsFromArtifact.ts and readAnalyzerJsonsFromFileSystem.ts — turn an artifact's ArtifactContents or a local reports dir into `AnalyzerJsonByPackage`. - library/azureDevops/getArtifactForCommit.ts — generic "find a build for a commit on this pipeline and download an artifact from it." Replaces the inline scan inside AdoSizeComparator. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../azureDevops/getArtifactForCommit.ts | 134 ++++++++++++++++++ .../library/bundleSize/compareJsonReports.ts | 58 ++++++++ .../bundleSize/compareJsonReportsByPackage.ts | 28 ++++ .../extractAnalyzerJsonsFromArtifact.ts | 27 ++++ .../readAnalyzerJsonsFromFileSystem.ts | 58 ++++++++ .../sourcePackageFromAnalyzerPath.ts | 22 +++ .../build-cli/src/library/bundleSize/types.ts | 52 +++++++ 7 files changed, 379 insertions(+) create mode 100644 build-tools/packages/build-cli/src/library/azureDevops/getArtifactForCommit.ts create mode 100644 build-tools/packages/build-cli/src/library/bundleSize/compareJsonReports.ts create mode 100644 build-tools/packages/build-cli/src/library/bundleSize/compareJsonReportsByPackage.ts create mode 100644 build-tools/packages/build-cli/src/library/bundleSize/extractAnalyzerJsonsFromArtifact.ts create mode 100644 build-tools/packages/build-cli/src/library/bundleSize/readAnalyzerJsonsFromFileSystem.ts create mode 100644 build-tools/packages/build-cli/src/library/bundleSize/sourcePackageFromAnalyzerPath.ts diff --git a/build-tools/packages/build-cli/src/library/azureDevops/getArtifactForCommit.ts b/build-tools/packages/build-cli/src/library/azureDevops/getArtifactForCommit.ts new file mode 100644 index 000000000000..4137f27fde4c --- /dev/null +++ b/build-tools/packages/build-cli/src/library/azureDevops/getArtifactForCommit.ts @@ -0,0 +1,134 @@ +/*! + * Copyright (c) Microsoft Corporation and contributors. All rights reserved. + * Licensed under the MIT License. + */ + +import type { WebApi } from "azure-devops-node-api"; +import { + type Build, + BuildResult, + BuildStatus, +} from "azure-devops-node-api/interfaces/BuildInterfaces.js"; + +import { type ArtifactContents, downloadArtifact } from "./downloadArtifact.js"; + +// Upper bound on builds fetched when searching for one matching a target commit. +// ADO has no API to query builds by commit SHA, so this window size determines +// how stale a target commit can be relative to the pipeline's recent activity +// and still be findable. +const recentBuildsToFetch = 100; + +/** + * Wrapper around the unwieldy positional signature of ADO's `getBuilds`. + */ +async function getRecentBuilds( + adoApi: WebApi, + project: string, + definitionId: number, +): Promise { + const buildApi = await adoApi.getBuildApi(); + return buildApi.getBuilds( + project, + [definitionId], + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + recentBuildsToFetch, + ); +} + +/** + * Looks up the build for `commit` in `builds` and validates that it has an id, + * is completed, and succeeded. Returns the build id on success, or a + * human-readable error explaining why no usable build was found. + */ +function findBuildIdForCommit( + builds: Build[], + commit: string, +): { kind: "found"; buildId: number } | { kind: "error"; error: string } { + const build = builds.find((b) => b.sourceVersion === commit); + + if (build === undefined) { + return { kind: "error", error: `No build found for commit ${commit}` }; + } + + if (build.id === undefined) { + return { kind: "error", error: `Build for commit ${commit} does not have a build id` }; + } + + if (build.status !== BuildStatus.Completed) { + return { kind: "error", error: `Build for commit ${commit} has not yet completed.` }; + } + + if (build.result !== BuildResult.Succeeded) { + return { + kind: "error", + error: `Build for commit ${commit} did not succeed.`, + }; + } + + return { kind: "found", buildId: build.id }; +} + +/** + * Result of looking up an artifact for a target commit on an ADO pipeline. + */ +export type ArtifactForCommitResult = + | { kind: "found"; contents: ArtifactContents } + | { kind: "error"; error: string }; + +export interface GetArtifactForCommitArgs { + /** A connection to the ADO API. */ + adoApi: WebApi; + /** Name of the pipeline artifact to fetch. */ + artifactName: string; + /** Commit whose build to look up. */ + commit: string; + /** ID of the ADO pipeline whose builds to search. */ + definitionId: number; + /** The ADO project name. */ + project: string; +} + +/** + * Look up the build for `commit` on the given ADO pipeline and return the + * contents of one of its artifacts. Returns a discriminated union: on success, + * the artifact's {@link ArtifactContents}; on failure, a human-readable error + * string covering missing/incomplete/failed builds and missing artifacts. + */ +export async function getArtifactForCommit( + args: GetArtifactForCommitArgs, +): Promise { + const { adoApi, artifactName, commit, definitionId, project } = args; + + const builds = await getRecentBuilds(adoApi, project, definitionId); + + const buildLookup = findBuildIdForCommit(builds, commit); + if (buildLookup.kind === "error") { + return buildLookup; + } + + try { + const contents = await downloadArtifact( + adoApi, + project, + buildLookup.buildId, + artifactName, + ); + return { kind: "found", contents }; + } catch (e) { + return { + kind: "error", + error: `Build for commit ${commit} did not publish artifact "${artifactName}": ${e instanceof Error ? e.message : String(e)}`, + }; + } +} diff --git a/build-tools/packages/build-cli/src/library/bundleSize/compareJsonReports.ts b/build-tools/packages/build-cli/src/library/bundleSize/compareJsonReports.ts new file mode 100644 index 000000000000..9b1980abbdfe --- /dev/null +++ b/build-tools/packages/build-cli/src/library/bundleSize/compareJsonReports.ts @@ -0,0 +1,58 @@ +/*! + * Copyright (c) Microsoft Corporation and contributors. All rights reserved. + * Licensed under the MIT License. + */ + +import type { BundleAnalyzerPlugin } from "webpack-bundle-analyzer"; + +import type { BundleData, BundlesComparison } from "./types.js"; + +/** + * Filter `report` to its asset entries and key their size data by asset name. + * `undefined` is treated as an empty report, so callers can pass an absent + * side directly. + */ +function jsonReportToBundleSizes( + report: BundleAnalyzerPlugin.JsonReport | undefined, +): Map { + const sizes = new Map(); + if (report === undefined) return sizes; + for (const entry of report) { + if (!entry.isAsset) continue; + sizes.set(entry.label, { + statSize: entry.statSize, + parsedSize: entry.parsedSize, + gzipSize: entry.gzipSize, + }); + } + return sizes; +} + +/** + * Compare the asset entries from two `JsonReport`s (one webpack-bundle-analyzer + * output each side) and produce the per-bundle comparison map. Either side may + * be `undefined` to represent a package that only exists on the other side. + * Bundles present only in one side encode added/removed via field presence + * (see {@link BundlesComparison}). + */ +export function compareJsonReports( + base: BundleAnalyzerPlugin.JsonReport | undefined, + compare: BundleAnalyzerPlugin.JsonReport | undefined, +): BundlesComparison { + const baseSizes = jsonReportToBundleSizes(base); + const compareSizes = jsonReportToBundleSizes(compare); + + const allBundleNames = new Set([...baseSizes.keys(), ...compareSizes.keys()]); + + const bundles: BundlesComparison = {}; + for (const bundleName of allBundleNames) { + const baseBundle = baseSizes.get(bundleName); + const compareBundle = compareSizes.get(bundleName); + bundles[bundleName] = { + ...(baseBundle && { base: baseBundle }), + ...(compareBundle && { compare: compareBundle }), + }; + } + + return bundles; +} diff --git a/build-tools/packages/build-cli/src/library/bundleSize/compareJsonReportsByPackage.ts b/build-tools/packages/build-cli/src/library/bundleSize/compareJsonReportsByPackage.ts new file mode 100644 index 000000000000..2fdde36ebfb0 --- /dev/null +++ b/build-tools/packages/build-cli/src/library/bundleSize/compareJsonReportsByPackage.ts @@ -0,0 +1,28 @@ +/*! + * Copyright (c) Microsoft Corporation and contributors. All rights reserved. + * Licensed under the MIT License. + */ + +import { compareJsonReports } from "./compareJsonReports.js"; +import type { AnalyzerJsonByPackage, PackageComparison } from "./types.js"; + +/** + * Compare per-package `JsonReport`s for two snapshots and produce a + * {@link PackageComparison}. Iterates the union of source packages so packages + * present only on one side are represented (their `compareJsonReports` call + * treats the absent side as empty). + */ +export function compareJsonReportsByPackage( + base: AnalyzerJsonByPackage, + compare: AnalyzerJsonByPackage, +): PackageComparison { + const allPackages = new Set([...base.keys(), ...compare.keys()]); + const result: PackageComparison = {}; + for (const sourcePackage of allPackages) { + result[sourcePackage] = compareJsonReports( + base.get(sourcePackage), + compare.get(sourcePackage), + ); + } + return result; +} diff --git a/build-tools/packages/build-cli/src/library/bundleSize/extractAnalyzerJsonsFromArtifact.ts b/build-tools/packages/build-cli/src/library/bundleSize/extractAnalyzerJsonsFromArtifact.ts new file mode 100644 index 000000000000..a5e19633a3a6 --- /dev/null +++ b/build-tools/packages/build-cli/src/library/bundleSize/extractAnalyzerJsonsFromArtifact.ts @@ -0,0 +1,27 @@ +/*! + * Copyright (c) Microsoft Corporation and contributors. All rights reserved. + * Licensed under the MIT License. + */ + +import type { BundleAnalyzerPlugin } from "webpack-bundle-analyzer"; + +import type { ArtifactContents } from "../azureDevops/downloadArtifact.js"; +import { sourcePackageFromAnalyzerPath } from "./sourcePackageFromAnalyzerPath.js"; +import type { AnalyzerJsonByPackage } from "./types.js"; + +/** + * Walks a downloaded artifact's contents, finds every `analyzer.json`, parses + * it, and keys the results by source package. + */ +export function extractAnalyzerJsonsFromArtifact( + contents: ArtifactContents, +): AnalyzerJsonByPackage { + const result: AnalyzerJsonByPackage = new Map(); + for (const [relativePath, bytes] of Object.entries(contents)) { + const sourcePackage = sourcePackageFromAnalyzerPath(relativePath); + if (sourcePackage === undefined) continue; + const text = Buffer.from(bytes).toString("utf8"); + result.set(sourcePackage, JSON.parse(text) as BundleAnalyzerPlugin.JsonReport); + } + return result; +} diff --git a/build-tools/packages/build-cli/src/library/bundleSize/readAnalyzerJsonsFromFileSystem.ts b/build-tools/packages/build-cli/src/library/bundleSize/readAnalyzerJsonsFromFileSystem.ts new file mode 100644 index 000000000000..868d25536b59 --- /dev/null +++ b/build-tools/packages/build-cli/src/library/bundleSize/readAnalyzerJsonsFromFileSystem.ts @@ -0,0 +1,58 @@ +/*! + * Copyright (c) Microsoft Corporation and contributors. All rights reserved. + * Licensed under the MIT License. + */ + +import { promises as fsPromises } from "fs"; +import { join } from "path"; +import type { BundleAnalyzerPlugin } from "webpack-bundle-analyzer"; + +import { sourcePackageFromAnalyzerPath } from "./sourcePackageFromAnalyzerPath.js"; +import type { AnalyzerJsonByPackage } from "./types.js"; + +/** + * Recursively walk `sourceFolder`, returning the relative path of every file. + */ +async function getAllFilesInDirectory( + sourceFolder: string, + partialPathPrefix: string = "", +): Promise { + const result: string[] = []; + for (const file of await fsPromises.readdir(sourceFolder)) { + const fullPath = join(sourceFolder, file); + if ((await fsPromises.stat(fullPath)).isFile()) { + result.push(join(partialPathPrefix, file)); + } else { + result.push( + ...(await getAllFilesInDirectory( + join(sourceFolder, file), + join(partialPathPrefix, file), + )), + ); + } + } + return result; +} + +/** + * Walks `rootPath`, finds every `analyzer.json` file, parses it, and keys the + * results by source package. + */ +export async function readAnalyzerJsonsFromFileSystem( + rootPath: string, +): Promise { + const allPaths = await getAllFilesInDirectory(rootPath); + const result: AnalyzerJsonByPackage = new Map(); + const reads: Promise[] = []; + for (const relativePath of allPaths) { + const sourcePackage = sourcePackageFromAnalyzerPath(relativePath); + if (sourcePackage === undefined) continue; + reads.push( + fsPromises.readFile(join(rootPath, relativePath), "utf8").then((text) => { + result.set(sourcePackage, JSON.parse(text) as BundleAnalyzerPlugin.JsonReport); + }), + ); + } + await Promise.all(reads); + return result; +} diff --git a/build-tools/packages/build-cli/src/library/bundleSize/sourcePackageFromAnalyzerPath.ts b/build-tools/packages/build-cli/src/library/bundleSize/sourcePackageFromAnalyzerPath.ts new file mode 100644 index 000000000000..dae7040da53c --- /dev/null +++ b/build-tools/packages/build-cli/src/library/bundleSize/sourcePackageFromAnalyzerPath.ts @@ -0,0 +1,22 @@ +/*! + * Copyright (c) Microsoft Corporation and contributors. All rights reserved. + * Licensed under the MIT License. + */ + +const analyzerJsonFileName = "analyzer.json"; + +/** + * If `relativePath` looks like `/analyzer.json` (nested layout — + * e.g. `@fluid-example/bundle-size-tests/analyzer.json`), returns the source + * package name. Returns `undefined` for paths that don't match. + * + * Slashes are normalized first so the same logic handles both Windows and + * POSIX path separators. + */ +export function sourcePackageFromAnalyzerPath(relativePath: string): string | undefined { + const pathParts = relativePath.replace(/\\/g, "/").split("/"); + if (pathParts.at(-1) !== analyzerJsonFileName) return undefined; + pathParts.pop(); + if (pathParts.length === 0) return undefined; + return pathParts.join("/"); +} diff --git a/build-tools/packages/build-cli/src/library/bundleSize/types.ts b/build-tools/packages/build-cli/src/library/bundleSize/types.ts index 94a5901a82bc..80fae26db400 100644 --- a/build-tools/packages/build-cli/src/library/bundleSize/types.ts +++ b/build-tools/packages/build-cli/src/library/bundleSize/types.ts @@ -3,6 +3,8 @@ * Licensed under the MIT License. */ +import type { BundleAnalyzerPlugin } from "webpack-bundle-analyzer"; + /** * A map of bundles friendly names to their relevant metrics */ @@ -30,3 +32,53 @@ export interface BundleComparison { commonBundleMetrics: { [key: string]: { baseline: BundleMetric; compare: BundleMetric } }; } + +/** + * Map from source package name to that package's parsed analyzer.json + * (webpack-bundle-analyzer's `analyzerMode: "json"` output). + */ +export type AnalyzerJsonByPackage = Map; + +/** + * Data for a single bundle (webpack entrypoint), sourced from + * webpack-bundle-analyzer's chart data. + */ +export interface BundleData { + /** + * Sum of source-module sizes before tree-shaking and minification. + */ + statSize: number; + /** + * Post-minification on-disk size — what's actually emitted to the bundle output. + */ + parsedSize: number; + /** + * Estimated size after gzip compression — closest proxy for what users download. + */ + gzipSize: number; +} + +/** + * Per-bundle comparison for one source package, keyed by bundle name (webpack + * entrypoint). Field presence on each entry encodes three states: + * - **pre-existing** (existed in both): both `base` and `compare` present + * - **added** (only in PR): only `compare` present + * - **removed** (only in baseline): only `base` present + */ +export type BundlesComparison = { + [bundleName: string]: { + base?: BundleData; + compare?: BundleData; + }; +}; + +/** + * Full comparison keyed by source package name. Packages present only on one + * side appear with that side's bundles only. + * + * The producer is deliberately unopinionated: it emits raw sizes only. Consumers + * compute deltas, percentages, and apply their own thresholds / regression rules. + */ +export type PackageComparison = { + [sourcePackage: string]: BundlesComparison; +}; From 0bbf1e49d6a8ab6331758e6eab076061bd466757 Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Fri, 15 May 2026 12:23:17 -0700 Subject: [PATCH 02/21] Rewire check bundleSize onto the new pure-function primitives MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The command no longer goes through ADOSizeComparator. Instead it threads: pickFreshestCanonicalRemote → getMergeBaseWithHead → getArtifactForCommit → extractAnalyzerJsonsFromArtifact → readAnalyzerJsonsFromFileSystem → compareJsonReportsByPackage The user-facing behavior is preserved: same `--target` flag, same auto-detect logging, same per-asset parsed+gzip output for the common case. The PackageComparison shape additionally distinguishes added / removed bundles via field presence — bundles present on only one side are now reported explicitly instead of being silently dropped. ADOSizeComparator (and the rest of bundleSize/ADO/) is now unreferenced and gets deleted in a follow-up commit. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/commands/check/bundleSize.ts | 109 ++++++++++-------- .../build-cli/src/library/bundleSize/index.ts | 10 ++ 2 files changed, 72 insertions(+), 47 deletions(-) diff --git a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts index ff3b1d6a6a23..bf9d1a808701 100644 --- a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts +++ b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts @@ -4,14 +4,17 @@ */ import { Flags } from "@oclif/core"; + +import { getArtifactForCommit } from "../../library/azureDevops/getArtifactForCommit.js"; import { getAzureDevopsApi } from "../../library/azureDevops/getAzureDevopsApi.js"; import { - ADOSizeComparator, - type BundleComparison, - bundlesContainNoChanges, + compareJsonReportsByPackage, + extractAnalyzerJsonsFromArtifact, + getMergeBaseWithHead, + type PackageComparison, pickFreshestCanonicalRemote, + readAnalyzerJsonsFromFileSystem, } from "../../library/bundleSize/index.js"; - import { BaseCommand } from "../../library/commands/base.js"; // Must match the "public" project + build-bundle-size-artifacts.yml (definitionId 48). @@ -31,7 +34,7 @@ const defaultLocalReportPath = "./artifacts/bundleAnalyzerJson"; */ type CheckBundleSizeResult = | { kind: "no-changes"; baselineCommit: string } - | { kind: "changes"; baselineCommit: string; comparison: BundleComparison[] } + | { kind: "changes"; baselineCommit: string; comparison: PackageComparison } | { kind: "error"; baselineCommit: string | undefined; error: string }; export default class CheckBundleSize extends BaseCommand { @@ -69,66 +72,78 @@ export default class CheckBundleSize extends BaseCommand this.log(`Using target ref ${targetRef}. Pass --target to override.`); } + const baselineCommit = getMergeBaseWithHead(targetRef); + this.log(`Baseline commit: ${baselineCommit}`); + // Anonymous reads work for the public ADO project at this command's scale; // automated consumers authenticate at the library layer. const adoApi = getAzureDevopsApi(undefined, adoConstants.orgUrl); - const sizeComparator = new ADOSizeComparator( - adoConstants, + const artifactResult = await getArtifactForCommit({ adoApi, - localReportPath, - targetRef, - ); - const comparisonResult = await sizeComparator.getSizeComparison(); - - if (comparisonResult.kind === "error") { - this.warning(comparisonResult.error); - return { - kind: "error", - baselineCommit: comparisonResult.baselineCommit, - error: comparisonResult.error, - }; + artifactName: adoConstants.artifactName, + commit: baselineCommit, + definitionId: adoConstants.ciBuildDefinitionId, + project: adoConstants.projectName, + }); + + if (artifactResult.kind === "error") { + this.warning(artifactResult.error); + return { kind: "error", baselineCommit, error: artifactResult.error }; } - if (comparisonResult.comparison.length === 0) { + const [baselineJsons, prJsons] = await Promise.all([ + Promise.resolve(extractAnalyzerJsonsFromArtifact(artifactResult.contents)), + readAnalyzerJsonsFromFileSystem(localReportPath), + ]); + + if (baselineJsons.size === 0 && prJsons.size === 0) { const message = "No bundles to compare — baseline artifact or local bundle reports are empty."; this.warning(message); - return { - kind: "error", - baselineCommit: comparisonResult.baselineCommit, - error: message, - }; + return { kind: "error", baselineCommit, error: message }; } - const { baselineCommit, comparison } = comparisonResult; - - if (bundlesContainNoChanges(comparison)) { - this.log(`No bundle size changes vs baseline commit ${baselineCommit}.`); - return { kind: "no-changes", baselineCommit }; - } + const comparison = compareJsonReportsByPackage(baselineJsons, prJsons); - const fmt = (before: number, after: number, delta: number): string => { + const fmt = (before: number, after: number): string => { + const delta = after - before; const sign = delta > 0 ? "+" : ""; return `${before} -> ${after} (${sign}${delta})`; }; - this.log(`Bundle size changes vs baseline commit ${baselineCommit}:`); - for (const bundle of comparison) { - const changedMetrics = Object.entries(bundle.commonBundleMetrics).flatMap( - ([metricName, { baseline, compare }]) => { - const parsedDelta = compare.parsedSize - baseline.parsedSize; - const gzipDelta = compare.gzipSize - baseline.gzipSize; - if (parsedDelta === 0 && gzipDelta === 0) return []; - return [ - ` ${metricName}: parsed ${fmt(baseline.parsedSize, compare.parsedSize, parsedDelta)}, gzip ${fmt(baseline.gzipSize, compare.gzipSize, gzipDelta)}`, - ]; - }, - ); - if (changedMetrics.length === 0) continue; - this.log(` ${bundle.bundleName}:`); - for (const line of changedMetrics) this.log(line); + const changeLines: string[] = []; + for (const [sourcePackage, bundles] of Object.entries(comparison)) { + const bundleLines: string[] = []; + for (const [bundleName, { base, compare }] of Object.entries(bundles)) { + if (base === undefined && compare !== undefined) { + bundleLines.push( + ` ${bundleName}: added (parsed ${compare.parsedSize}, gzip ${compare.gzipSize})`, + ); + } else if (compare === undefined && base !== undefined) { + bundleLines.push( + ` ${bundleName}: removed (was parsed ${base.parsedSize}, gzip ${base.gzipSize})`, + ); + } else if (base !== undefined && compare !== undefined) { + const parsedChanged = base.parsedSize !== compare.parsedSize; + const gzipChanged = base.gzipSize !== compare.gzipSize; + if (!parsedChanged && !gzipChanged) continue; + bundleLines.push( + ` ${bundleName}: parsed ${fmt(base.parsedSize, compare.parsedSize)}, gzip ${fmt(base.gzipSize, compare.gzipSize)}`, + ); + } + } + if (bundleLines.length === 0) continue; + changeLines.push(` ${sourcePackage}:`, ...bundleLines); } + if (changeLines.length === 0) { + this.log(`No bundle size changes vs baseline commit ${baselineCommit}.`); + return { kind: "no-changes", baselineCommit }; + } + + this.log(`Bundle size changes vs baseline commit ${baselineCommit}:`); + for (const line of changeLines) this.log(line); + return { kind: "changes", baselineCommit, comparison }; } } diff --git a/build-tools/packages/build-cli/src/library/bundleSize/index.ts b/build-tools/packages/build-cli/src/library/bundleSize/index.ts index e30e861a9366..7ba82729c47d 100644 --- a/build-tools/packages/build-cli/src/library/bundleSize/index.ts +++ b/build-tools/packages/build-cli/src/library/bundleSize/index.ts @@ -16,15 +16,25 @@ export { SizeComparison, } from "./ADO/index.js"; export { bundlesContainNoChanges, compareBundles } from "./compareBundles.js"; +export { compareJsonReports } from "./compareJsonReports.js"; +export { compareJsonReportsByPackage } from "./compareJsonReportsByPackage.js"; +export { extractAnalyzerJsonsFromArtifact } from "./extractAnalyzerJsonsFromArtifact.js"; +export { readAnalyzerJsonsFromFileSystem } from "./readAnalyzerJsonsFromFileSystem.js"; +export { sourcePackageFromAnalyzerPath } from "./sourcePackageFromAnalyzerPath.js"; export { + AnalyzerJsonByPackage, BundleComparison, + BundleData, BundleMetric, BundleMetricSet, BundleSummaries, + BundlesComparison, + PackageComparison, } from "./types.js"; export { GetBuildOptions, getAllFilesInDirectory, getBuilds, + getMergeBaseWithHead, pickFreshestCanonicalRemote, } from "./utilities/index.js"; From ac6c424fb4771074ceaea0333a2c5e2d818f043d Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Fri, 15 May 2026 12:30:28 -0700 Subject: [PATCH 03/21] Delete the legacy ADOSizeComparator path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check bundleSize` no longer references any of these as of the previous commit, and nothing else does. Drop: - library/bundleSize/ADO/ — the whole subtree (AdoArtifactFileProvider, AdoSizeComparator, Constants, FileSystemBundleFileProvider, getBundleFilePathsFromFolder, getBundleSummaries, index). - library/bundleSize/compareBundles.ts (replaced by compareJsonReports and compareJsonReportsByPackage). - library/bundleSize/utilities/getBuilds.ts (replaced by azureDevops/getArtifactForCommit.ts). - library/bundleSize/utilities/getAllFilesInDirectory.ts (only consumer was the deleted ADO/ subtree; readAnalyzerJsonsFromFileSystem has its own local walker). - Legacy types from library/bundleSize/types.ts: BundleMetric, BundleMetricSet, BundleSummaries, BundleComparison. Library barrel and utilities/index.ts trimmed to match. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../bundleSize/ADO/AdoArtifactFileProvider.ts | 26 --- .../bundleSize/ADO/AdoSizeComparator.ts | 186 ------------------ .../src/library/bundleSize/ADO/Constants.ts | 23 --- .../ADO/FileSystemBundleFileProvider.ts | 38 ---- .../ADO/getBundleFilePathsFromFolder.ts | 46 ----- .../bundleSize/ADO/getBundleSummaries.ts | 47 ----- .../src/library/bundleSize/ADO/index.ts | 20 -- .../src/library/bundleSize/compareBundles.ts | 67 ------- .../build-cli/src/library/bundleSize/index.ts | 25 +-- .../build-cli/src/library/bundleSize/types.ts | 28 --- .../utilities/getAllFilesInDirectory.ts | 33 ---- .../library/bundleSize/utilities/getBuilds.ts | 46 ----- .../src/library/bundleSize/utilities/index.ts | 2 - 13 files changed, 1 insertion(+), 586 deletions(-) delete mode 100644 build-tools/packages/build-cli/src/library/bundleSize/ADO/AdoArtifactFileProvider.ts delete mode 100644 build-tools/packages/build-cli/src/library/bundleSize/ADO/AdoSizeComparator.ts delete mode 100644 build-tools/packages/build-cli/src/library/bundleSize/ADO/Constants.ts delete mode 100644 build-tools/packages/build-cli/src/library/bundleSize/ADO/FileSystemBundleFileProvider.ts delete mode 100644 build-tools/packages/build-cli/src/library/bundleSize/ADO/getBundleFilePathsFromFolder.ts delete mode 100644 build-tools/packages/build-cli/src/library/bundleSize/ADO/getBundleSummaries.ts delete mode 100644 build-tools/packages/build-cli/src/library/bundleSize/ADO/index.ts delete mode 100644 build-tools/packages/build-cli/src/library/bundleSize/compareBundles.ts delete mode 100644 build-tools/packages/build-cli/src/library/bundleSize/utilities/getAllFilesInDirectory.ts delete mode 100644 build-tools/packages/build-cli/src/library/bundleSize/utilities/getBuilds.ts diff --git a/build-tools/packages/build-cli/src/library/bundleSize/ADO/AdoArtifactFileProvider.ts b/build-tools/packages/build-cli/src/library/bundleSize/ADO/AdoArtifactFileProvider.ts deleted file mode 100644 index be8a19593970..000000000000 --- a/build-tools/packages/build-cli/src/library/bundleSize/ADO/AdoArtifactFileProvider.ts +++ /dev/null @@ -1,26 +0,0 @@ -/*! - * Copyright (c) Microsoft Corporation and contributors. All rights reserved. - * Licensed under the MIT License. - */ - -import { strict as assert } from "node:assert"; -import type { BundleAnalyzerPlugin } from "webpack-bundle-analyzer"; - -import type { ArtifactContents } from "../../azureDevops/downloadArtifact.js"; - -/** - * Retrieves and parses an analyzer.json file (webpack-bundle-analyzer's - * `analyzerMode: "json"` output) from the decompressed artifact contents. - * @param contents - Artifact contents keyed by file path relative to the artifact root. - * @param relativePath - The relative path to the file that will be retrieved. - */ -export function getAnalyzerJsonFromContents( - contents: ArtifactContents, - relativePath: string, -): BundleAnalyzerPlugin.JsonReport { - const bytes = contents[relativePath]; - assert(bytes, `getAnalyzerJsonFromContents could not find file ${relativePath}`); - - const text = Buffer.from(bytes).toString("utf8"); - return JSON.parse(text) as BundleAnalyzerPlugin.JsonReport; -} diff --git a/build-tools/packages/build-cli/src/library/bundleSize/ADO/AdoSizeComparator.ts b/build-tools/packages/build-cli/src/library/bundleSize/ADO/AdoSizeComparator.ts deleted file mode 100644 index 8ce0f4910feb..000000000000 --- a/build-tools/packages/build-cli/src/library/bundleSize/ADO/AdoSizeComparator.ts +++ /dev/null @@ -1,186 +0,0 @@ -/*! - * Copyright (c) Microsoft Corporation and contributors. All rights reserved. - * Licensed under the MIT License. - */ - -import { join } from "node:path"; -import type { WebApi } from "azure-devops-node-api"; -import { BuildResult, BuildStatus } from "azure-devops-node-api/interfaces/BuildInterfaces.js"; - -import { - type ArtifactContents, - downloadArtifact, -} from "../../azureDevops/downloadArtifact.js"; -import { compareBundles } from "../compareBundles.js"; -import type { BundleComparison } from "../types.js"; -import { getBuilds, getMergeBaseWithHead } from "../utilities/index.js"; -import { getAnalyzerJsonFromContents } from "./AdoArtifactFileProvider.js"; -import type { IADOConstants } from "./Constants.js"; -import { - getAnalyzerJsonFromFileSystem, - getAnalyzerPathsFromFileSystem, -} from "./FileSystemBundleFileProvider.js"; -import { getAnalyzerFilePathsFromFolder } from "./getBundleFilePathsFromFolder.js"; -import { getBundleSummariesFromAnalyzer } from "./getBundleSummaries.js"; - -/** - * Result of a size comparison against a baseline build, discriminated by `kind`. - * - * On `"success"`, `comparison` holds the bundle diff against `baselineCommit`. - * On `"error"`, the comparison could not be produced and `error` holds the reason; - * `baselineCommit` reflects the last commit that was attempted and may be `undefined` - * if the search never found a candidate. - */ -export type SizeComparison = - | { kind: "success"; baselineCommit: string; comparison: BundleComparison[] } - | { kind: "error"; baselineCommit: string | undefined; error: string }; - -export class ADOSizeComparator { - /** - * The default number of most recent builds on the ADO pipeline to search when - * looking for a build matching a baseline commit. The most recent builds may not - * necessarily match the chain of commits, but typically will when the pipeline - * only builds commits to main. - */ - private static readonly defaultBuildsToSearch = 100; - - constructor( - /** - * ADO constants identifying where to fetch baseline bundle info - */ - private readonly adoConstants: IADOConstants, - /** - * The ADO connection to use to fetch baseline bundle info - */ - private readonly adoApi: WebApi, - /** - * Path to existing local bundle size reports - */ - private readonly localReportPath: string, - /** - * Target ref — the ref a PR would be opened against. The baseline commit - * is `git merge-base HEAD`, so this accepts any argument - * `git merge-base` does. - */ - private readonly targetRef: string, - ) {} - - /** - * Run the bundle size comparison against the baseline build. - * - * @returns A {@link SizeComparison} tagged with `kind: "success"` or `kind: "error"`. - * Never throws: unexpected exceptions from underlying `git` shell-outs, ADO API - * calls, or stats-file parsing are caught and reported via the `error` variant so - * callers can rely on the return shape. - */ - public async getSizeComparison(): Promise { - // Declared outside the try block so the catch can still report the last-known - // commit value in the synthesized error variant. - let baselineCommit: string | undefined; - try { - baselineCommit = getMergeBaseWithHead(this.targetRef); - console.log(`Baseline commit: ${baselineCommit}`); - - const recentBuilds = await getBuilds(this.adoApi, { - project: this.adoConstants.projectName, - definitions: [this.adoConstants.ciBuildDefinitionId], - maxBuildsPerDefinition: - this.adoConstants.buildsToSearch ?? ADOSizeComparator.defaultBuildsToSearch, - }); - - const baselineBuild = recentBuilds.find( - (build) => build.sourceVersion === baselineCommit, - ); - - if (baselineBuild === undefined) { - return { - kind: "error", - baselineCommit, - error: `No CI build found for baseline commit ${baselineCommit}`, - }; - } - - if (baselineBuild.id === undefined) { - return { - kind: "error", - baselineCommit, - error: `Baseline build does not have a build id`, - }; - } - - if (baselineBuild.status !== BuildStatus.Completed) { - return { - kind: "error", - baselineCommit, - error: "Baseline build for this commit has not yet completed.", - }; - } - - if (baselineBuild.result !== BuildResult.Succeeded) { - return { - kind: "error", - baselineCommit, - error: "Baseline CI build failed, cannot generate bundle analysis at this time", - }; - } - - console.log(`Found baseline build with id: ${baselineBuild.id}`); - - const baselineContents = await downloadArtifact( - this.adoApi, - this.adoConstants.projectName, - baselineBuild.id, - this.adoConstants.artifactName, - ).catch((error) => { - // Preserve the underlying error/stack: it's useful diagnostic - // info that doesn't reach the synthesized error variant below. - console.log(`Error downloading artifact: ${error.message}`); - console.log(`Error stack: ${error.stack}`); - return undefined; - }); - - if (baselineContents === undefined) { - return { - kind: "error", - baselineCommit, - error: "Baseline build did not publish bundle artifacts", - }; - } - - const comparison: BundleComparison[] = - await this.createComparisonFromContents(baselineContents); - - return { kind: "success", baselineCommit, comparison }; - } catch (e) { - return { - kind: "error", - baselineCommit, - error: `Unexpected failure during size comparison: ${ - e instanceof Error ? e.message : String(e) - }`, - }; - } - } - - private async createComparisonFromContents( - baselineContents: ArtifactContents, - ): Promise { - const baselineBundlePaths = getAnalyzerFilePathsFromFolder(Object.keys(baselineContents)); - - const prBundleFileSystemPaths = await getAnalyzerPathsFromFileSystem(this.localReportPath); - - const baselineSummaries = await getBundleSummariesFromAnalyzer({ - bundlePaths: baselineBundlePaths, - getAnalyzerJson: async (relativePath) => - getAnalyzerJsonFromContents(baselineContents, relativePath), - }); - - const prSummaries = await getBundleSummariesFromAnalyzer({ - bundlePaths: prBundleFileSystemPaths, - getAnalyzerJson: async (relativePath) => - getAnalyzerJsonFromFileSystem(join(this.localReportPath, relativePath)), - }); - - return compareBundles(baselineSummaries, prSummaries); - } -} diff --git a/build-tools/packages/build-cli/src/library/bundleSize/ADO/Constants.ts b/build-tools/packages/build-cli/src/library/bundleSize/ADO/Constants.ts deleted file mode 100644 index 91a851a93722..000000000000 --- a/build-tools/packages/build-cli/src/library/bundleSize/ADO/Constants.ts +++ /dev/null @@ -1,23 +0,0 @@ -/*! - * Copyright (c) Microsoft Corporation and contributors. All rights reserved. - * Licensed under the MIT License. - */ - -export interface IADOConstants { - // URL for the ADO org - orgUrl: string; - - // The ADO project that contains the repo - projectName: string; - - // The ID for the build that runs against main when PRs are merged - ciBuildDefinitionId: number; - - // The name of the build artifact that contains the bundle size data - artifactName: string; - - // The number of most recent ADO builds to pull when searching for one associated - // with a specific commit, default 20. Pulling more builds takes longer, but may - // be useful when there are a high volume of commits/builds. - buildsToSearch?: number; -} diff --git a/build-tools/packages/build-cli/src/library/bundleSize/ADO/FileSystemBundleFileProvider.ts b/build-tools/packages/build-cli/src/library/bundleSize/ADO/FileSystemBundleFileProvider.ts deleted file mode 100644 index 64ad9da0c280..000000000000 --- a/build-tools/packages/build-cli/src/library/bundleSize/ADO/FileSystemBundleFileProvider.ts +++ /dev/null @@ -1,38 +0,0 @@ -/*! - * Copyright (c) Microsoft Corporation and contributors. All rights reserved. - * Licensed under the MIT License. - */ - -import { promises as fsPromises } from "fs"; -import type { BundleAnalyzerPlugin } from "webpack-bundle-analyzer"; - -import { getAllFilesInDirectory } from "../utilities/index.js"; -import { - type BundleFileData, - getAnalyzerFilePathsFromFolder, -} from "./getBundleFilePathsFromFolder.js"; - -/** - * Returns a list of `analyzer.json` paths from the given folder (one per source package). - * @param bundleReportPath - The path to the folder containing the bundle report - */ -export async function getAnalyzerPathsFromFileSystem( - bundleReportPath: string, -): Promise { - const filePaths = await getAllFilesInDirectory(bundleReportPath); - - return getAnalyzerFilePathsFromFolder(filePaths); -} - -/** - * Reads and parses an analyzer.json file (webpack-bundle-analyzer's - * `analyzerMode: "json"` output) from the filesystem. - * @param path - the full path to the file in the filesystem - */ -export async function getAnalyzerJsonFromFileSystem( - path: string, -): Promise { - const text = await fsPromises.readFile(path, "utf8"); - - return JSON.parse(text) as BundleAnalyzerPlugin.JsonReport; -} diff --git a/build-tools/packages/build-cli/src/library/bundleSize/ADO/getBundleFilePathsFromFolder.ts b/build-tools/packages/build-cli/src/library/bundleSize/ADO/getBundleFilePathsFromFolder.ts deleted file mode 100644 index 3e6489f3a0ef..000000000000 --- a/build-tools/packages/build-cli/src/library/bundleSize/ADO/getBundleFilePathsFromFolder.ts +++ /dev/null @@ -1,46 +0,0 @@ -/*! - * Copyright (c) Microsoft Corporation and contributors. All rights reserved. - * Licensed under the MIT License. - */ - -export interface BundleFileData { - bundleName: string; - - relativePathToStatsFile: string; - - relativePathToConfigFile: string | undefined; -} - -function getBundleNameFromPath(relativePath: string): string { - // Our artifacts are stored in the the format //[/]/. - // We want to use the npm scope + package name as the bundle name. - // The regex here normalized the slashes in the path names. - const pathParts = relativePath.replace(/\\/g, "/").split("/"); - - if (pathParts.length < 3) { - throw Error(`Could not derive a bundle name from this path: ${relativePath}`); - } - pathParts.pop(); // Remove the filename - - return pathParts.join("/"); -} - -/** - * Filters the given paths down to `analyzer.json` files (one per source package), - * pairing each with its derived bundle name. - */ -export function getAnalyzerFilePathsFromFolder( - relativePathsInFolder: string[], -): BundleFileData[] { - return relativePathsInFolder - .filter((relativePath) => { - // Normalize backslashes so the same logic handles both Windows and POSIX path separators. - const fileName = relativePath.replace(/\\/g, "/").split("/").pop(); - return fileName === "analyzer.json"; - }) - .map((relativePath) => ({ - bundleName: getBundleNameFromPath(relativePath), - relativePathToStatsFile: relativePath, - relativePathToConfigFile: undefined, - })); -} diff --git a/build-tools/packages/build-cli/src/library/bundleSize/ADO/getBundleSummaries.ts b/build-tools/packages/build-cli/src/library/bundleSize/ADO/getBundleSummaries.ts deleted file mode 100644 index 13cd07ea0a81..000000000000 --- a/build-tools/packages/build-cli/src/library/bundleSize/ADO/getBundleSummaries.ts +++ /dev/null @@ -1,47 +0,0 @@ -/*! - * Copyright (c) Microsoft Corporation and contributors. All rights reserved. - * Licensed under the MIT License. - */ - -import type { BundleAnalyzerPlugin } from "webpack-bundle-analyzer"; - -import type { BundleMetricSet, BundleSummaries } from "../types.js"; -import type { BundleFileData } from "./getBundleFilePathsFromFolder.js"; - -export interface GetBundleSummariesFromAnalyzerArgs { - bundlePaths: BundleFileData[]; - - getAnalyzerJson: (relativePath: string) => Promise; -} - -/** - * Builds a {@link BundleSummaries} from analyzer.json (webpack-bundle-analyzer's - * `analyzerMode: "json"` output). The data is already pre-summarized per asset, so - * no stats processors are needed — each asset entry's `parsedSize` becomes a - * `BundleMetric` keyed by the asset's `label`. - */ -export async function getBundleSummariesFromAnalyzer( - args: GetBundleSummariesFromAnalyzerArgs, -): Promise { - const result: BundleSummaries = new Map(); - - const pendingAsyncWork = args.bundlePaths.map(async (bundle) => { - const entries = await args.getAnalyzerJson(bundle.relativePathToStatsFile); - - const metrics: BundleMetricSet = new Map(); - for (const entry of entries) { - if (entry.isAsset) { - metrics.set(entry.label, { - parsedSize: entry.parsedSize, - gzipSize: entry.gzipSize, - }); - } - } - - result.set(bundle.bundleName, metrics); - }); - - await Promise.all(pendingAsyncWork); - - return result; -} diff --git a/build-tools/packages/build-cli/src/library/bundleSize/ADO/index.ts b/build-tools/packages/build-cli/src/library/bundleSize/ADO/index.ts deleted file mode 100644 index b12d06bf265c..000000000000 --- a/build-tools/packages/build-cli/src/library/bundleSize/ADO/index.ts +++ /dev/null @@ -1,20 +0,0 @@ -/*! - * Copyright (c) Microsoft Corporation and contributors. All rights reserved. - * Licensed under the MIT License. - */ - -export { getAnalyzerJsonFromContents } from "./AdoArtifactFileProvider.js"; -export { ADOSizeComparator, SizeComparison } from "./AdoSizeComparator.js"; -export { IADOConstants } from "./Constants.js"; -export { - getAnalyzerJsonFromFileSystem, - getAnalyzerPathsFromFileSystem, -} from "./FileSystemBundleFileProvider.js"; -export { - BundleFileData, - getAnalyzerFilePathsFromFolder, -} from "./getBundleFilePathsFromFolder.js"; -export { - GetBundleSummariesFromAnalyzerArgs, - getBundleSummariesFromAnalyzer, -} from "./getBundleSummaries.js"; diff --git a/build-tools/packages/build-cli/src/library/bundleSize/compareBundles.ts b/build-tools/packages/build-cli/src/library/bundleSize/compareBundles.ts deleted file mode 100644 index d57d71fc6758..000000000000 --- a/build-tools/packages/build-cli/src/library/bundleSize/compareBundles.ts +++ /dev/null @@ -1,67 +0,0 @@ -/*! - * Copyright (c) Microsoft Corporation and contributors. All rights reserved. - * Licensed under the MIT License. - */ - -import type { BundleComparison, BundleSummaries } from "./types.js"; - -/** - * Compares all the bundle summaries for a "baseline" and a "compare" bundle. - */ -export function compareBundles( - baseline: BundleSummaries, - compare: BundleSummaries, -): BundleComparison[] { - const results: BundleComparison[] = []; - - baseline.forEach((baselineBundle, bundleName) => { - const compareBundle = compare.get(bundleName); - - if (!compareBundle) { - console.log( - `Baseline has bundle '${bundleName}' that does not appear in the comparison bundle `, - ); - } else { - const bundleComparison: BundleComparison = { bundleName, commonBundleMetrics: {} }; - - baselineBundle.forEach((baselineMetric, metricName) => { - const compareMetric = compareBundle.get(metricName); - - if (!compareMetric) { - console.log( - `Baseline has metric '${metricName}' in bundle '${bundleName}' that does not exist in the comparison bundle'`, - ); - } else { - bundleComparison.commonBundleMetrics[metricName] = { - baseline: baselineMetric, - compare: compareMetric, - }; - } - }); - - results.push(bundleComparison); - } - }); - - return results; -} - -/** - * Checks if a bundle comparison contains no size changes - * @param comparisons - bundle comparison - */ -export function bundlesContainNoChanges(comparisons: BundleComparison[]): boolean { - for (const { commonBundleMetrics } of comparisons) { - const metrics = Object.values(commonBundleMetrics); - for (const { baseline, compare } of metrics) { - if ( - baseline.parsedSize !== compare.parsedSize || - baseline.gzipSize !== compare.gzipSize - ) { - return false; - } - } - } - - return true; -} diff --git a/build-tools/packages/build-cli/src/library/bundleSize/index.ts b/build-tools/packages/build-cli/src/library/bundleSize/index.ts index 7ba82729c47d..26d1803b4219 100644 --- a/build-tools/packages/build-cli/src/library/bundleSize/index.ts +++ b/build-tools/packages/build-cli/src/library/bundleSize/index.ts @@ -3,19 +3,6 @@ * Licensed under the MIT License. */ -export { - ADOSizeComparator, - BundleFileData, - GetBundleSummariesFromAnalyzerArgs, - getAnalyzerFilePathsFromFolder, - getAnalyzerJsonFromContents, - getAnalyzerJsonFromFileSystem, - getAnalyzerPathsFromFileSystem, - getBundleSummariesFromAnalyzer, - IADOConstants, - SizeComparison, -} from "./ADO/index.js"; -export { bundlesContainNoChanges, compareBundles } from "./compareBundles.js"; export { compareJsonReports } from "./compareJsonReports.js"; export { compareJsonReportsByPackage } from "./compareJsonReportsByPackage.js"; export { extractAnalyzerJsonsFromArtifact } from "./extractAnalyzerJsonsFromArtifact.js"; @@ -23,18 +10,8 @@ export { readAnalyzerJsonsFromFileSystem } from "./readAnalyzerJsonsFromFileSyst export { sourcePackageFromAnalyzerPath } from "./sourcePackageFromAnalyzerPath.js"; export { AnalyzerJsonByPackage, - BundleComparison, BundleData, - BundleMetric, - BundleMetricSet, - BundleSummaries, BundlesComparison, PackageComparison, } from "./types.js"; -export { - GetBuildOptions, - getAllFilesInDirectory, - getBuilds, - getMergeBaseWithHead, - pickFreshestCanonicalRemote, -} from "./utilities/index.js"; +export { getMergeBaseWithHead, pickFreshestCanonicalRemote } from "./utilities/index.js"; diff --git a/build-tools/packages/build-cli/src/library/bundleSize/types.ts b/build-tools/packages/build-cli/src/library/bundleSize/types.ts index 80fae26db400..1e4b759021c9 100644 --- a/build-tools/packages/build-cli/src/library/bundleSize/types.ts +++ b/build-tools/packages/build-cli/src/library/bundleSize/types.ts @@ -5,34 +5,6 @@ import type { BundleAnalyzerPlugin } from "webpack-bundle-analyzer"; -/** - * A map of bundles friendly names to their relevant metrics - */ -export type BundleSummaries = Map; - -/** - * A collection of all the relevant size metrics for a given bundle. A bundle can have one or more named metrics that - * could map to a single chunk or a collection chunks. - */ -export type BundleMetricSet = Map; - -/** - * A description of the size of a particular part of a bundle - */ -export interface BundleMetric { - parsedSize: number; - gzipSize: number; -} - -/** - * A comparison of two bundles - */ -export interface BundleComparison { - bundleName: string; - - commonBundleMetrics: { [key: string]: { baseline: BundleMetric; compare: BundleMetric } }; -} - /** * Map from source package name to that package's parsed analyzer.json * (webpack-bundle-analyzer's `analyzerMode: "json"` output). diff --git a/build-tools/packages/build-cli/src/library/bundleSize/utilities/getAllFilesInDirectory.ts b/build-tools/packages/build-cli/src/library/bundleSize/utilities/getAllFilesInDirectory.ts deleted file mode 100644 index b448229311bc..000000000000 --- a/build-tools/packages/build-cli/src/library/bundleSize/utilities/getAllFilesInDirectory.ts +++ /dev/null @@ -1,33 +0,0 @@ -/*! - * Copyright (c) Microsoft Corporation and contributors. All rights reserved. - * Licensed under the MIT License. - */ - -import { promises as fsPromises } from "fs"; -import { join } from "path"; - -/** - * Gets the relative path of all files in this directory - * @param sourceFolder - The path of the directory to scan - * @param partialPathPrefix - The partial path built up as we recurse through directories. External callers probably don't want to set this. - */ -export async function getAllFilesInDirectory( - sourceFolder: string, - partialPathPrefix: string = "", -): Promise { - const result: string[] = []; - for (const file of await fsPromises.readdir(sourceFolder)) { - const fullPath = join(sourceFolder, file); - if ((await fsPromises.stat(fullPath)).isFile()) { - result.push(join(partialPathPrefix, file)); - } else { - result.push( - ...(await getAllFilesInDirectory( - join(sourceFolder, file), - join(partialPathPrefix, file), - )), - ); - } - } - return result; -} diff --git a/build-tools/packages/build-cli/src/library/bundleSize/utilities/getBuilds.ts b/build-tools/packages/build-cli/src/library/bundleSize/utilities/getBuilds.ts deleted file mode 100644 index cd06507d3e1c..000000000000 --- a/build-tools/packages/build-cli/src/library/bundleSize/utilities/getBuilds.ts +++ /dev/null @@ -1,46 +0,0 @@ -/*! - * Copyright (c) Microsoft Corporation and contributors. All rights reserved. - * Licensed under the MIT License. - */ - -import type { WebApi } from "azure-devops-node-api"; -import type { Build } from "azure-devops-node-api/interfaces/BuildInterfaces.js"; - -export interface GetBuildOptions { - // The ADO project name - project: string; - - // An array of ADO definitions that should be considered for this query - definitions: number[]; - - // An optional set of tags that should be on the returned builds - tagFilters?: string[]; - - // An upper limit on the number of queries to return. Can be used to improve performance - maxBuildsPerDefinition?: number; -} - -/** - * A wrapper around the terrible API signature for ADO getBuilds - */ -export async function getBuilds(adoApi: WebApi, options: GetBuildOptions): Promise { - const buildApi = await adoApi.getBuildApi(); - - return buildApi.getBuilds( - options.project, - options.definitions, - undefined, - undefined, - undefined, - undefined, - undefined, - undefined, - undefined, - undefined, - options.tagFilters, - undefined, - undefined, - undefined, - options.maxBuildsPerDefinition, - ); -} diff --git a/build-tools/packages/build-cli/src/library/bundleSize/utilities/index.ts b/build-tools/packages/build-cli/src/library/bundleSize/utilities/index.ts index d8d920b7fe66..22bfe7c68996 100644 --- a/build-tools/packages/build-cli/src/library/bundleSize/utilities/index.ts +++ b/build-tools/packages/build-cli/src/library/bundleSize/utilities/index.ts @@ -3,6 +3,4 @@ * Licensed under the MIT License. */ -export { getAllFilesInDirectory } from "./getAllFilesInDirectory.js"; -export { GetBuildOptions, getBuilds } from "./getBuilds.js"; export { getMergeBaseWithHead, pickFreshestCanonicalRemote } from "./gitCommands.js"; From 4757a75b44ecee8535fcbc238be20e5bdbbdbfe9 Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Fri, 15 May 2026 12:47:32 -0700 Subject: [PATCH 04/21] Promote git utilities out of bundleSize; parameterize the URL matcher MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The contents of `library/bundleSize/utilities/gitCommands.ts` had no bundle-size concepts in them. Two functions were also FF-specific (hardcoded `microsoft/FluidFramework` URL regex + log strings) but the specificity belonged at the call site, not in a "git utilities" file. Split it: - Generic git helpers (`getMergeBaseWithHead`, plus the previously- pickFreshestCanonicalRemote logic renamed and reshaped to `pickFreshestRemote(branch, isEligible)`) move to `library/git/gitCommands.ts`. `isEligible: (url: string) => boolean` decides which remotes are candidates; the function knows nothing about FF. Log strings are neutral ("Eligible remotes:" / "No eligible remote has […] fetched locally") — callers provide context above. - `check bundleSize` defines the FF-canonical URL regex inline and passes it as the `isEligible` predicate. The now-empty `library/bundleSize/utilities/` directory is removed. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/commands/check/bundleSize.ts | 6 +- .../build-cli/src/library/bundleSize/index.ts | 1 - .../src/library/bundleSize/utilities/index.ts | 6 -- .../utilities => git}/gitCommands.ts | 66 ++++++++----------- 4 files changed, 31 insertions(+), 48 deletions(-) delete mode 100644 build-tools/packages/build-cli/src/library/bundleSize/utilities/index.ts rename build-tools/packages/build-cli/src/library/{bundleSize/utilities => git}/gitCommands.ts (75%) diff --git a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts index bf9d1a808701..89ce4653e0fa 100644 --- a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts +++ b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts @@ -10,12 +10,11 @@ import { getAzureDevopsApi } from "../../library/azureDevops/getAzureDevopsApi.j import { compareJsonReportsByPackage, extractAnalyzerJsonsFromArtifact, - getMergeBaseWithHead, type PackageComparison, - pickFreshestCanonicalRemote, readAnalyzerJsonsFromFileSystem, } from "../../library/bundleSize/index.js"; import { BaseCommand } from "../../library/commands/base.js"; +import { getMergeBaseWithHead, pickFreshestRemote } from "../../library/git/gitCommands.js"; // Must match the "public" project + build-bundle-size-artifacts.yml (definitionId 48). const adoConstants = { @@ -62,12 +61,13 @@ export default class CheckBundleSize extends BaseCommand // Auto-detect targets `main` on the canonical remote; `--target ` overrides. const branch = "main"; + const canonicalUrl = /(^|[/:])microsoft\/fluidframework(\.git)?$/i; let targetRef: string; if (target !== undefined) { targetRef = target; this.log(`Using explicit target ref ${target}.`); } else { - const remote = pickFreshestCanonicalRemote(branch) ?? "origin"; + const remote = pickFreshestRemote(branch, (url) => canonicalUrl.test(url)) ?? "origin"; targetRef = `${remote}/${branch}`; this.log(`Using target ref ${targetRef}. Pass --target to override.`); } diff --git a/build-tools/packages/build-cli/src/library/bundleSize/index.ts b/build-tools/packages/build-cli/src/library/bundleSize/index.ts index 26d1803b4219..6b9e96b624a7 100644 --- a/build-tools/packages/build-cli/src/library/bundleSize/index.ts +++ b/build-tools/packages/build-cli/src/library/bundleSize/index.ts @@ -14,4 +14,3 @@ export { BundlesComparison, PackageComparison, } from "./types.js"; -export { getMergeBaseWithHead, pickFreshestCanonicalRemote } from "./utilities/index.js"; diff --git a/build-tools/packages/build-cli/src/library/bundleSize/utilities/index.ts b/build-tools/packages/build-cli/src/library/bundleSize/utilities/index.ts deleted file mode 100644 index 22bfe7c68996..000000000000 --- a/build-tools/packages/build-cli/src/library/bundleSize/utilities/index.ts +++ /dev/null @@ -1,6 +0,0 @@ -/*! - * Copyright (c) Microsoft Corporation and contributors. All rights reserved. - * Licensed under the MIT License. - */ - -export { getMergeBaseWithHead, pickFreshestCanonicalRemote } from "./gitCommands.js"; diff --git a/build-tools/packages/build-cli/src/library/bundleSize/utilities/gitCommands.ts b/build-tools/packages/build-cli/src/library/git/gitCommands.ts similarity index 75% rename from build-tools/packages/build-cli/src/library/bundleSize/utilities/gitCommands.ts rename to build-tools/packages/build-cli/src/library/git/gitCommands.ts index 852b05bea82b..48d2f975d8d7 100644 --- a/build-tools/packages/build-cli/src/library/bundleSize/utilities/gitCommands.ts +++ b/build-tools/packages/build-cli/src/library/git/gitCommands.ts @@ -16,32 +16,27 @@ export function getMergeBaseWithHead(targetRef: string): string { } /** - * A canonical-remote ref paired with its locally-resolved tip commit. + * A remote ref paired with its locally-resolved tip commit. */ -interface CanonicalCandidate { +interface RemoteCandidate { name: string; ref: string; tip: string; } /** - * List remotes that point at the canonical `microsoft/FluidFramework` - * repository. + * List every remote configured in the local git repo. * - * Match is case-insensitive and tolerant of a trailing `.git`, covering both - * HTTPS (`https://github.com/microsoft/FluidFramework[.git]`) and SSH - * (`git@github.com:microsoft/FluidFramework[.git]`) remote URL forms. - * - * @returns The matching remotes in `.git/config` order, or an empty array if - * none match. + * @returns The configured remotes in `.git/config` order, or an empty array if + * none are configured. */ -function findCanonicalRemotes(): { name: string; url: string }[] { +function listRemotes(): { name: string; url: string }[] { // Read every `remote..url` config entry. `--all` returns every match // (otherwise `--regexp` returns only the first); `--show-names` includes // the key so the remote name can be extracted. // Exit codes from `git config get --regexp`: // 0 = at least one match - // 1 = no matches (e.g. clone has no canonical remote configured) + // 1 = no matches (e.g. clone has no remotes configured) // any other = the subcommand itself failed — most likely git < 2.46 // (`get` is not a recognized subcommand on older versions). // Treat status 1 as a clean "no matches" and reserve the targeted "upgrade @@ -60,22 +55,19 @@ function findCanonicalRemotes(): { name: string; url: string }[] { const detail = error instanceof Error ? error.message : String(error); throw new Error( `Failed to read remote URLs via \`git config get --regexp\` (introduced in git 2.46). ` + - `Upgrade git, or pass --target to skip remote auto-detection.\n` + + `Upgrade git to enable remote auto-detection.\n` + `Underlying error: ${detail}`, ); } const line = /^remote\.(.+)\.url\s+(.+)$/; - const canonical = /(^|[/:])microsoft\/fluidframework(\.git)?$/i; - const matches: { name: string; url: string }[] = []; + const remotes: { name: string; url: string }[] = []; for (const raw of output.split("\n")) { const match = line.exec(raw); if (match === null) continue; const [, name, url] = match; - if (canonical.test(url)) { - matches.push({ name, url }); - } + remotes.push({ name, url }); } - return matches; + return remotes; } /** @@ -135,8 +127,8 @@ function isAncestor(ancestor: string, descendant: string): boolean { * one winner; equal tips don't dominate each other, and truly divergent * histories (rare for `main`) produce multiple winners. */ -function pickFreshest(candidates: CanonicalCandidate[]): CanonicalCandidate[] { - function hasStrictlyNewerPeer(candidate: CanonicalCandidate): boolean { +function pickFreshest(candidates: RemoteCandidate[]): RemoteCandidate[] { + function hasStrictlyNewerPeer(candidate: RemoteCandidate): boolean { return candidates.some((other) => { if (other === candidate) return false; if (other.tip === candidate.tip) return false; // ties don't dominate @@ -149,27 +141,29 @@ function pickFreshest(candidates: CanonicalCandidate[]): CanonicalCandidate[] { } /** - * Pick the canonical remote (one pointing at `microsoft/FluidFramework`) whose - * `/` is freshest locally. + * From the remotes configured in the local repo whose URL matches `filter`, + * pick the one whose `/` is freshest locally. * * Remotes whose `/` doesn't resolve locally are dropped. Among * the rest, pick the tip that isn't a strict ancestor of any other's; ties * (identical or divergent tips) resolve to the first candidate in config order. * - * @returns The selected remote's name, or `undefined` if no canonical remote is - * configured or none have a locally-resolvable `/`. + * @returns The selected remote's name, or `undefined` if no remote matches + * `filter` or none have a locally-resolvable `/`. */ -export function pickFreshestCanonicalRemote(branch: string): string | undefined { - const canonicals = findCanonicalRemotes(); +export function pickFreshestRemote( + branch: string, + filter: (url: string) => boolean, +): string | undefined { + const eligible = listRemotes().filter((r) => filter(r.url)); - if (canonicals.length === 0) { - console.log(`No remote found pointing at microsoft/FluidFramework.`); + if (eligible.length === 0) { return undefined; } - const candidates: CanonicalCandidate[] = []; + const candidates: RemoteCandidate[] = []; const skipped: string[] = []; - for (const remote of canonicals) { + for (const remote of eligible) { const ref = `${remote.name}/${branch}`; let tip: string | undefined; try { @@ -191,15 +185,11 @@ export function pickFreshestCanonicalRemote(branch: string): string | undefined } if (candidates.length === 0) { - console.log( - `Found remote(s) pointing at microsoft/FluidFramework but none of [${skipped.join( - ", ", - )}] are fetched locally.`, - ); + console.log(`No eligible remote has [${skipped.join(", ")}] fetched locally.`); return undefined; } - let freshest: CanonicalCandidate[]; + let freshest: RemoteCandidate[]; try { freshest = pickFreshest(candidates); } catch (error) { @@ -214,7 +204,7 @@ export function pickFreshestCanonicalRemote(branch: string): string | undefined } const selected = freshest[0]; - console.log(`Remotes pointing at microsoft/FluidFramework:`); + console.log(`Eligible remotes:`); for (const ref of skipped) { console.log(` ${ref} — not fetched locally; skipped`); } From 9e6092195e0abe6555c4a04a9e796f224d64d696 Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Fri, 15 May 2026 13:01:50 -0700 Subject: [PATCH 05/21] Rename gitCommands.ts -> pickFreshestRemote.ts; inline getMergeBaseWithHead The file's name described its implementation ("git commands") rather than its purpose. After this commit it holds exactly one exported function (`pickFreshestRemote`) and the file name says so. `getMergeBaseWithHead` had a single call site and was a one-liner around `execFileSync('git', ['merge-base', targetRef, 'HEAD'])`. Inline it. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../build-cli/src/commands/check/bundleSize.ts | 7 +++++-- .../git/{gitCommands.ts => pickFreshestRemote.ts} | 10 ---------- 2 files changed, 5 insertions(+), 12 deletions(-) rename build-tools/packages/build-cli/src/library/git/{gitCommands.ts => pickFreshestRemote.ts} (95%) diff --git a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts index 89ce4653e0fa..01b64d8fea8d 100644 --- a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts +++ b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts @@ -3,6 +3,7 @@ * Licensed under the MIT License. */ +import { execFileSync } from "node:child_process"; import { Flags } from "@oclif/core"; import { getArtifactForCommit } from "../../library/azureDevops/getArtifactForCommit.js"; @@ -14,7 +15,7 @@ import { readAnalyzerJsonsFromFileSystem, } from "../../library/bundleSize/index.js"; import { BaseCommand } from "../../library/commands/base.js"; -import { getMergeBaseWithHead, pickFreshestRemote } from "../../library/git/gitCommands.js"; +import { pickFreshestRemote } from "../../library/git/pickFreshestRemote.js"; // Must match the "public" project + build-bundle-size-artifacts.yml (definitionId 48). const adoConstants = { @@ -72,7 +73,9 @@ export default class CheckBundleSize extends BaseCommand this.log(`Using target ref ${targetRef}. Pass --target to override.`); } - const baselineCommit = getMergeBaseWithHead(targetRef); + const baselineCommit = execFileSync("git", ["merge-base", targetRef, "HEAD"]) + .toString() + .trim(); this.log(`Baseline commit: ${baselineCommit}`); // Anonymous reads work for the public ADO project at this command's scale; diff --git a/build-tools/packages/build-cli/src/library/git/gitCommands.ts b/build-tools/packages/build-cli/src/library/git/pickFreshestRemote.ts similarity index 95% rename from build-tools/packages/build-cli/src/library/git/gitCommands.ts rename to build-tools/packages/build-cli/src/library/git/pickFreshestRemote.ts index 48d2f975d8d7..24d9639d05cf 100644 --- a/build-tools/packages/build-cli/src/library/git/gitCommands.ts +++ b/build-tools/packages/build-cli/src/library/git/pickFreshestRemote.ts @@ -5,16 +5,6 @@ import { execFileSync } from "node:child_process"; -/** - * Compute the merge-base of `HEAD` and the given ref. The ref may be any - * argument `git merge-base` accepts (remote branch, local branch, SHA, tag, …). - * - * @returns The merge-base commit SHA. - */ -export function getMergeBaseWithHead(targetRef: string): string { - return execFileSync("git", ["merge-base", targetRef, "HEAD"]).toString().trim(); -} - /** * A remote ref paired with its locally-resolved tip commit. */ From 85ba6939633f48a42ed26dac330bbba2e9c89b21 Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Fri, 15 May 2026 13:14:29 -0700 Subject: [PATCH 06/21] Tighten check bundleSize: drop pointless Promise.resolve, fix mismatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `extractAnalyzerJsonsFromArtifact` is synchronous, so wrapping it in `Promise.resolve()` to feed `Promise.all` just adds noise — `Promise.all` accepts non-Promise values, and there's no real overlap to gain anyway (only the filesystem read is async). Replace with two statements. - The empty-inputs guard fires only when *both* sides are empty (`&&`), but the warning text said "baseline artifact or local bundle reports are empty," reading as either. Reword to match the condition. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../packages/build-cli/src/commands/check/bundleSize.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts index 01b64d8fea8d..25920ecd0125 100644 --- a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts +++ b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts @@ -94,14 +94,12 @@ export default class CheckBundleSize extends BaseCommand return { kind: "error", baselineCommit, error: artifactResult.error }; } - const [baselineJsons, prJsons] = await Promise.all([ - Promise.resolve(extractAnalyzerJsonsFromArtifact(artifactResult.contents)), - readAnalyzerJsonsFromFileSystem(localReportPath), - ]); + const baselineJsons = extractAnalyzerJsonsFromArtifact(artifactResult.contents); + const prJsons = await readAnalyzerJsonsFromFileSystem(localReportPath); if (baselineJsons.size === 0 && prJsons.size === 0) { const message = - "No bundles to compare — baseline artifact or local bundle reports are empty."; + "No bundles to compare — baseline artifact and local bundle reports are both empty."; this.warning(message); return { kind: "error", baselineCommit, error: message }; } From 11fa65eeffcbe78bc604d70a9930245a2da0465a Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Fri, 15 May 2026 13:23:35 -0700 Subject: [PATCH 07/21] Wrap check bundleSize run() so it always returns a result variant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous PR's `ADOSizeComparator.getSizeComparison()` wrapped its body in a try/catch that converted any throw into the `error` variant of its discriminated return. Deleting that class moved several throw- prone calls (the merge-base shell-out, `pickFreshestRemote` on old git, `readAnalyzerJsonsFromFileSystem` on bad paths, …) into the command's `run()` with no equivalent guard, regressing the "always returns a `CheckBundleSizeResult`" contract that `--json` consumers depend on. Single top-level try/catch in `run()` restores the property. The catch synthesizes the same error variant the per-step error paths produce. Doesn't try to carry `baselineCommit` forward — the catch path returns `baselineCommit: undefined` regardless of where in the flow the throw landed. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/commands/check/bundleSize.ts | 164 +++++++++--------- 1 file changed, 85 insertions(+), 79 deletions(-) diff --git a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts index 25920ecd0125..c90f7f73951e 100644 --- a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts +++ b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts @@ -58,93 +58,99 @@ export default class CheckBundleSize extends BaseCommand } as const; public async run(): Promise { - const { localReportPath, target } = this.flags; - - // Auto-detect targets `main` on the canonical remote; `--target ` overrides. - const branch = "main"; - const canonicalUrl = /(^|[/:])microsoft\/fluidframework(\.git)?$/i; - let targetRef: string; - if (target !== undefined) { - targetRef = target; - this.log(`Using explicit target ref ${target}.`); - } else { - const remote = pickFreshestRemote(branch, (url) => canonicalUrl.test(url)) ?? "origin"; - targetRef = `${remote}/${branch}`; - this.log(`Using target ref ${targetRef}. Pass --target to override.`); - } + try { + const { localReportPath, target } = this.flags; + + // Auto-detect targets `main` on the canonical remote; `--target ` overrides. + const branch = "main"; + const canonicalUrl = /(^|[/:])microsoft\/fluidframework(\.git)?$/i; + let targetRef: string; + if (target !== undefined) { + targetRef = target; + this.log(`Using explicit target ref ${target}.`); + } else { + const remote = pickFreshestRemote(branch, (url) => canonicalUrl.test(url)) ?? "origin"; + targetRef = `${remote}/${branch}`; + this.log(`Using target ref ${targetRef}. Pass --target to override.`); + } - const baselineCommit = execFileSync("git", ["merge-base", targetRef, "HEAD"]) - .toString() - .trim(); - this.log(`Baseline commit: ${baselineCommit}`); - - // Anonymous reads work for the public ADO project at this command's scale; - // automated consumers authenticate at the library layer. - const adoApi = getAzureDevopsApi(undefined, adoConstants.orgUrl); - const artifactResult = await getArtifactForCommit({ - adoApi, - artifactName: adoConstants.artifactName, - commit: baselineCommit, - definitionId: adoConstants.ciBuildDefinitionId, - project: adoConstants.projectName, - }); - - if (artifactResult.kind === "error") { - this.warning(artifactResult.error); - return { kind: "error", baselineCommit, error: artifactResult.error }; - } + const baselineCommit = execFileSync("git", ["merge-base", targetRef, "HEAD"]) + .toString() + .trim(); + this.log(`Baseline commit: ${baselineCommit}`); + + // Anonymous reads work for the public ADO project at this command's scale; + // automated consumers authenticate at the library layer. + const adoApi = getAzureDevopsApi(undefined, adoConstants.orgUrl); + const artifactResult = await getArtifactForCommit({ + adoApi, + artifactName: adoConstants.artifactName, + commit: baselineCommit, + definitionId: adoConstants.ciBuildDefinitionId, + project: adoConstants.projectName, + }); + + if (artifactResult.kind === "error") { + this.warning(artifactResult.error); + return { kind: "error", baselineCommit, error: artifactResult.error }; + } - const baselineJsons = extractAnalyzerJsonsFromArtifact(artifactResult.contents); - const prJsons = await readAnalyzerJsonsFromFileSystem(localReportPath); + const baselineJsons = extractAnalyzerJsonsFromArtifact(artifactResult.contents); + const prJsons = await readAnalyzerJsonsFromFileSystem(localReportPath); - if (baselineJsons.size === 0 && prJsons.size === 0) { - const message = - "No bundles to compare — baseline artifact and local bundle reports are both empty."; - this.warning(message); - return { kind: "error", baselineCommit, error: message }; - } + if (baselineJsons.size === 0 && prJsons.size === 0) { + const message = + "No bundles to compare — baseline artifact and local bundle reports are both empty."; + this.warning(message); + return { kind: "error", baselineCommit, error: message }; + } - const comparison = compareJsonReportsByPackage(baselineJsons, prJsons); - - const fmt = (before: number, after: number): string => { - const delta = after - before; - const sign = delta > 0 ? "+" : ""; - return `${before} -> ${after} (${sign}${delta})`; - }; - - const changeLines: string[] = []; - for (const [sourcePackage, bundles] of Object.entries(comparison)) { - const bundleLines: string[] = []; - for (const [bundleName, { base, compare }] of Object.entries(bundles)) { - if (base === undefined && compare !== undefined) { - bundleLines.push( - ` ${bundleName}: added (parsed ${compare.parsedSize}, gzip ${compare.gzipSize})`, - ); - } else if (compare === undefined && base !== undefined) { - bundleLines.push( - ` ${bundleName}: removed (was parsed ${base.parsedSize}, gzip ${base.gzipSize})`, - ); - } else if (base !== undefined && compare !== undefined) { - const parsedChanged = base.parsedSize !== compare.parsedSize; - const gzipChanged = base.gzipSize !== compare.gzipSize; - if (!parsedChanged && !gzipChanged) continue; - bundleLines.push( - ` ${bundleName}: parsed ${fmt(base.parsedSize, compare.parsedSize)}, gzip ${fmt(base.gzipSize, compare.gzipSize)}`, - ); + const comparison = compareJsonReportsByPackage(baselineJsons, prJsons); + + const fmt = (before: number, after: number): string => { + const delta = after - before; + const sign = delta > 0 ? "+" : ""; + return `${before} -> ${after} (${sign}${delta})`; + }; + + const changeLines: string[] = []; + for (const [sourcePackage, bundles] of Object.entries(comparison)) { + const bundleLines: string[] = []; + for (const [bundleName, { base, compare }] of Object.entries(bundles)) { + if (base === undefined && compare !== undefined) { + bundleLines.push( + ` ${bundleName}: added (parsed ${compare.parsedSize}, gzip ${compare.gzipSize})`, + ); + } else if (compare === undefined && base !== undefined) { + bundleLines.push( + ` ${bundleName}: removed (was parsed ${base.parsedSize}, gzip ${base.gzipSize})`, + ); + } else if (base !== undefined && compare !== undefined) { + const parsedChanged = base.parsedSize !== compare.parsedSize; + const gzipChanged = base.gzipSize !== compare.gzipSize; + if (!parsedChanged && !gzipChanged) continue; + bundleLines.push( + ` ${bundleName}: parsed ${fmt(base.parsedSize, compare.parsedSize)}, gzip ${fmt(base.gzipSize, compare.gzipSize)}`, + ); + } } + if (bundleLines.length === 0) continue; + changeLines.push(` ${sourcePackage}:`, ...bundleLines); } - if (bundleLines.length === 0) continue; - changeLines.push(` ${sourcePackage}:`, ...bundleLines); - } - if (changeLines.length === 0) { - this.log(`No bundle size changes vs baseline commit ${baselineCommit}.`); - return { kind: "no-changes", baselineCommit }; - } + if (changeLines.length === 0) { + this.log(`No bundle size changes vs baseline commit ${baselineCommit}.`); + return { kind: "no-changes", baselineCommit }; + } - this.log(`Bundle size changes vs baseline commit ${baselineCommit}:`); - for (const line of changeLines) this.log(line); + this.log(`Bundle size changes vs baseline commit ${baselineCommit}:`); + for (const line of changeLines) this.log(line); - return { kind: "changes", baselineCommit, comparison }; + return { kind: "changes", baselineCommit, comparison }; + } catch (e) { + const error = `Unexpected failure: ${e instanceof Error ? e.message : String(e)}`; + this.warning(error); + return { kind: "error", baselineCommit: undefined, error }; + } } } From 6bfdae00f6132794f0e093083d9cf78b7ac520ba Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Fri, 15 May 2026 13:40:36 -0700 Subject: [PATCH 08/21] Extract formatComparison helper Pulls the change-line builder out of `run()` into a `formatComparison` helper that turns a `PackageComparison` into a flat list of human-readable lines. The command keeps the orchestration; the helper handles presentation. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/commands/check/bundleSize.ts | 73 +++++++++++-------- 1 file changed, 42 insertions(+), 31 deletions(-) diff --git a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts index c90f7f73951e..cfbc7d3d4c4b 100644 --- a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts +++ b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts @@ -37,6 +37,47 @@ type CheckBundleSizeResult = | { kind: "changes"; baselineCommit: string; comparison: PackageComparison } | { kind: "error"; baselineCommit: string | undefined; error: string }; +/** + * Render a {@link PackageComparison} as a flat list of human-readable lines. + * Skips packages whose bundles all have zero deltas. + * + * @returns The rendered lines, or an empty array when nothing changed across + * the whole comparison. + */ +function formatComparison(comparison: PackageComparison): string[] { + const fmt = (before: number, after: number): string => { + const delta = after - before; + const sign = delta > 0 ? "+" : ""; + return `${before} -> ${after} (${sign}${delta})`; + }; + + const lines: string[] = []; + for (const [sourcePackage, bundles] of Object.entries(comparison)) { + const bundleLines: string[] = []; + for (const [bundleName, { base, compare }] of Object.entries(bundles)) { + if (base === undefined && compare !== undefined) { + bundleLines.push( + ` ${bundleName}: added (parsed ${compare.parsedSize}, gzip ${compare.gzipSize})`, + ); + } else if (compare === undefined && base !== undefined) { + bundleLines.push( + ` ${bundleName}: removed (was parsed ${base.parsedSize}, gzip ${base.gzipSize})`, + ); + } else if (base !== undefined && compare !== undefined) { + const parsedChanged = base.parsedSize !== compare.parsedSize; + const gzipChanged = base.gzipSize !== compare.gzipSize; + if (!parsedChanged && !gzipChanged) continue; + bundleLines.push( + ` ${bundleName}: parsed ${fmt(base.parsedSize, compare.parsedSize)}, gzip ${fmt(base.gzipSize, compare.gzipSize)}`, + ); + } + } + if (bundleLines.length === 0) continue; + lines.push(` ${sourcePackage}:`, ...bundleLines); + } + return lines; +} + export default class CheckBundleSize extends BaseCommand { static readonly description = `Compare the locally-collected bundle reports against the CI build of the merge-base commit (between HEAD and a target ref) and print the diff. By default, the target is auto-detected as \`/main\` where \`\` is whichever remote points at \`microsoft/FluidFramework\`; pass \`--target\` to override. Prints a human-readable summary by default; pass --json for the structured result.`; @@ -106,37 +147,7 @@ export default class CheckBundleSize extends BaseCommand } const comparison = compareJsonReportsByPackage(baselineJsons, prJsons); - - const fmt = (before: number, after: number): string => { - const delta = after - before; - const sign = delta > 0 ? "+" : ""; - return `${before} -> ${after} (${sign}${delta})`; - }; - - const changeLines: string[] = []; - for (const [sourcePackage, bundles] of Object.entries(comparison)) { - const bundleLines: string[] = []; - for (const [bundleName, { base, compare }] of Object.entries(bundles)) { - if (base === undefined && compare !== undefined) { - bundleLines.push( - ` ${bundleName}: added (parsed ${compare.parsedSize}, gzip ${compare.gzipSize})`, - ); - } else if (compare === undefined && base !== undefined) { - bundleLines.push( - ` ${bundleName}: removed (was parsed ${base.parsedSize}, gzip ${base.gzipSize})`, - ); - } else if (base !== undefined && compare !== undefined) { - const parsedChanged = base.parsedSize !== compare.parsedSize; - const gzipChanged = base.gzipSize !== compare.gzipSize; - if (!parsedChanged && !gzipChanged) continue; - bundleLines.push( - ` ${bundleName}: parsed ${fmt(base.parsedSize, compare.parsedSize)}, gzip ${fmt(base.gzipSize, compare.gzipSize)}`, - ); - } - } - if (bundleLines.length === 0) continue; - changeLines.push(` ${sourcePackage}:`, ...bundleLines); - } + const changeLines = formatComparison(comparison); if (changeLines.length === 0) { this.log(`No bundle size changes vs baseline commit ${baselineCommit}.`); From ea437ccff72dd2a7e148b53c885b3c98b7615b46 Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Fri, 15 May 2026 14:25:32 -0700 Subject: [PATCH 09/21] Let check bundleSize use oclif's error helper instead of result variants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "always returns a result variant, never throws" contract was carried over from when CI pipelines consumed the JSON output. With the diff moving to a followup GH-Actions workflow and this command now positioned for inner-dev-loop use, the contract isn't doing useful work — oclif's default error handling (clean message on stderr, non-zero exit) is the codebase's convention for fatal cases (see `bump.ts`, `list.ts`, etc., which use `this.error()` even with `enableJsonFlag = true`). Changes: - Drop the `kind: "error"` variant from `CheckBundleSizeResult`. The result is now just `no-changes | changes`. - Drop the top-level try/catch from `run()`. Unexpected throws (filesystem errors, unresolvable refs, …) propagate to oclif. - Convert the per-step error returns to `this.error(...)`: artifact lookup failures and the "both sides empty" guard. - Drop the silent `origin` fallback when no canonical remote is found. That fallback was usually wrong anyway (origin is typically the developer's fork) and silently produced bad merge-bases. Error with an actionable message pointing at both resolutions: add a microsoft/FluidFramework remote, or pass --target. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/commands/check/bundleSize.ts | 112 +++++++++--------- 1 file changed, 54 insertions(+), 58 deletions(-) diff --git a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts index cfbc7d3d4c4b..b058e52a9464 100644 --- a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts +++ b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts @@ -34,8 +34,7 @@ const defaultLocalReportPath = "./artifacts/bundleAnalyzerJson"; */ type CheckBundleSizeResult = | { kind: "no-changes"; baselineCommit: string } - | { kind: "changes"; baselineCommit: string; comparison: PackageComparison } - | { kind: "error"; baselineCommit: string | undefined; error: string }; + | { kind: "changes"; baselineCommit: string; comparison: PackageComparison }; /** * Render a {@link PackageComparison} as a flat list of human-readable lines. @@ -99,69 +98,66 @@ export default class CheckBundleSize extends BaseCommand } as const; public async run(): Promise { - try { - const { localReportPath, target } = this.flags; - - // Auto-detect targets `main` on the canonical remote; `--target ` overrides. - const branch = "main"; - const canonicalUrl = /(^|[/:])microsoft\/fluidframework(\.git)?$/i; - let targetRef: string; - if (target !== undefined) { - targetRef = target; - this.log(`Using explicit target ref ${target}.`); - } else { - const remote = pickFreshestRemote(branch, (url) => canonicalUrl.test(url)) ?? "origin"; - targetRef = `${remote}/${branch}`; - this.log(`Using target ref ${targetRef}. Pass --target to override.`); + const { localReportPath, target } = this.flags; + + // Auto-detect targets `main` on the canonical remote; `--target ` overrides. + const branch = "main"; + const canonicalUrl = /(^|[/:])microsoft\/fluidframework(\.git)?$/i; + let targetRef: string; + if (target !== undefined) { + targetRef = target; + this.log(`Using explicit target ref ${target}.`); + } else { + const remote = pickFreshestRemote(branch, (url) => canonicalUrl.test(url)); + if (remote === undefined) { + this.error( + "Could not auto-detect a canonical remote. Add a remote pointing at microsoft/FluidFramework, or pass --target to override.", + ); } + targetRef = `${remote}/${branch}`; + this.log(`Using target ref ${targetRef}. Pass --target to override.`); + } - const baselineCommit = execFileSync("git", ["merge-base", targetRef, "HEAD"]) - .toString() - .trim(); - this.log(`Baseline commit: ${baselineCommit}`); - - // Anonymous reads work for the public ADO project at this command's scale; - // automated consumers authenticate at the library layer. - const adoApi = getAzureDevopsApi(undefined, adoConstants.orgUrl); - const artifactResult = await getArtifactForCommit({ - adoApi, - artifactName: adoConstants.artifactName, - commit: baselineCommit, - definitionId: adoConstants.ciBuildDefinitionId, - project: adoConstants.projectName, - }); - - if (artifactResult.kind === "error") { - this.warning(artifactResult.error); - return { kind: "error", baselineCommit, error: artifactResult.error }; - } + const baselineCommit = execFileSync("git", ["merge-base", targetRef, "HEAD"]) + .toString() + .trim(); + this.log(`Baseline commit: ${baselineCommit}`); + + // Anonymous reads work for the public ADO project at this command's scale; + // automated consumers authenticate at the library layer. + const adoApi = getAzureDevopsApi(undefined, adoConstants.orgUrl); + const artifactResult = await getArtifactForCommit({ + adoApi, + artifactName: adoConstants.artifactName, + commit: baselineCommit, + definitionId: adoConstants.ciBuildDefinitionId, + project: adoConstants.projectName, + }); + + if (artifactResult.kind === "error") { + this.error(artifactResult.error); + } - const baselineJsons = extractAnalyzerJsonsFromArtifact(artifactResult.contents); - const prJsons = await readAnalyzerJsonsFromFileSystem(localReportPath); + const baselineJsons = extractAnalyzerJsonsFromArtifact(artifactResult.contents); + const prJsons = await readAnalyzerJsonsFromFileSystem(localReportPath); - if (baselineJsons.size === 0 && prJsons.size === 0) { - const message = - "No bundles to compare — baseline artifact and local bundle reports are both empty."; - this.warning(message); - return { kind: "error", baselineCommit, error: message }; - } + if (baselineJsons.size === 0 && prJsons.size === 0) { + this.error( + "No bundles to compare — baseline artifact and local bundle reports are both empty.", + ); + } - const comparison = compareJsonReportsByPackage(baselineJsons, prJsons); - const changeLines = formatComparison(comparison); + const comparison = compareJsonReportsByPackage(baselineJsons, prJsons); + const changeLines = formatComparison(comparison); - if (changeLines.length === 0) { - this.log(`No bundle size changes vs baseline commit ${baselineCommit}.`); - return { kind: "no-changes", baselineCommit }; - } + if (changeLines.length === 0) { + this.log(`No bundle size changes vs baseline commit ${baselineCommit}.`); + return { kind: "no-changes", baselineCommit }; + } - this.log(`Bundle size changes vs baseline commit ${baselineCommit}:`); - for (const line of changeLines) this.log(line); + this.log(`Bundle size changes vs baseline commit ${baselineCommit}:`); + for (const line of changeLines) this.log(line); - return { kind: "changes", baselineCommit, comparison }; - } catch (e) { - const error = `Unexpected failure: ${e instanceof Error ? e.message : String(e)}`; - this.warning(error); - return { kind: "error", baselineCommit: undefined, error }; - } + return { kind: "changes", baselineCommit, comparison }; } } From cca73fcf0108d08aa1a8730d37795471ee56ee49 Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Fri, 15 May 2026 14:38:02 -0700 Subject: [PATCH 10/21] Make getArtifactForCommit throw on failures instead of returning a discriminated union MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `kind: "found" | "error"` shape was an artifact of the now-removed "command never throws" contract. With the command using `this.error()` for fatal cases, the sibling `getBaselineBuildMetrics.ts` pattern of throwing on "couldn't produce the result" applies cleanly here too. Callsite drops the `if (kind === "error")` branch. The artifact-download catch is kept (it adds useful context — which artifact, which commit) but reworded neutrally so it doesn't lie about network/auth failures, and chained via `{ cause: e }` so the underlying error is preserved instead of stringified. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/commands/check/bundleSize.ts | 8 +-- .../azureDevops/getArtifactForCommit.ts | 67 ++++++------------- 2 files changed, 24 insertions(+), 51 deletions(-) diff --git a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts index b058e52a9464..4de309b7fd28 100644 --- a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts +++ b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts @@ -126,7 +126,7 @@ export default class CheckBundleSize extends BaseCommand // Anonymous reads work for the public ADO project at this command's scale; // automated consumers authenticate at the library layer. const adoApi = getAzureDevopsApi(undefined, adoConstants.orgUrl); - const artifactResult = await getArtifactForCommit({ + const artifactContents = await getArtifactForCommit({ adoApi, artifactName: adoConstants.artifactName, commit: baselineCommit, @@ -134,11 +134,7 @@ export default class CheckBundleSize extends BaseCommand project: adoConstants.projectName, }); - if (artifactResult.kind === "error") { - this.error(artifactResult.error); - } - - const baselineJsons = extractAnalyzerJsonsFromArtifact(artifactResult.contents); + const baselineJsons = extractAnalyzerJsonsFromArtifact(artifactContents); const prJsons = await readAnalyzerJsonsFromFileSystem(localReportPath); if (baselineJsons.size === 0 && prJsons.size === 0) { diff --git a/build-tools/packages/build-cli/src/library/azureDevops/getArtifactForCommit.ts b/build-tools/packages/build-cli/src/library/azureDevops/getArtifactForCommit.ts index 4137f27fde4c..4786dbe68660 100644 --- a/build-tools/packages/build-cli/src/library/azureDevops/getArtifactForCommit.ts +++ b/build-tools/packages/build-cli/src/library/azureDevops/getArtifactForCommit.ts @@ -47,45 +47,31 @@ async function getRecentBuilds( } /** - * Looks up the build for `commit` in `builds` and validates that it has an id, - * is completed, and succeeded. Returns the build id on success, or a - * human-readable error explaining why no usable build was found. + * Find the build for `commit` in `builds` and validate that it has an id, + * is completed, and succeeded. + * + * @returns The build id. Throws with a human-readable message when no usable + * build is found. */ -function findBuildIdForCommit( - builds: Build[], - commit: string, -): { kind: "found"; buildId: number } | { kind: "error"; error: string } { +function findBuildIdForCommit(builds: Build[], commit: string): number { const build = builds.find((b) => b.sourceVersion === commit); if (build === undefined) { - return { kind: "error", error: `No build found for commit ${commit}` }; + throw new Error(`No build found for commit ${commit}`); } - if (build.id === undefined) { - return { kind: "error", error: `Build for commit ${commit} does not have a build id` }; + throw new Error(`Build for commit ${commit} does not have a build id`); } - if (build.status !== BuildStatus.Completed) { - return { kind: "error", error: `Build for commit ${commit} has not yet completed.` }; + throw new Error(`Build for commit ${commit} has not yet completed.`); } - if (build.result !== BuildResult.Succeeded) { - return { - kind: "error", - error: `Build for commit ${commit} did not succeed.`, - }; + throw new Error(`Build for commit ${commit} did not succeed.`); } - return { kind: "found", buildId: build.id }; + return build.id; } -/** - * Result of looking up an artifact for a target commit on an ADO pipeline. - */ -export type ArtifactForCommitResult = - | { kind: "found"; contents: ArtifactContents } - | { kind: "error"; error: string }; - export interface GetArtifactForCommitArgs { /** A connection to the ADO API. */ adoApi: WebApi; @@ -101,34 +87,25 @@ export interface GetArtifactForCommitArgs { /** * Look up the build for `commit` on the given ADO pipeline and return the - * contents of one of its artifacts. Returns a discriminated union: on success, - * the artifact's {@link ArtifactContents}; on failure, a human-readable error - * string covering missing/incomplete/failed builds and missing artifacts. + * contents of one of its artifacts. + * + * @returns The artifact's {@link ArtifactContents}. Throws with a + * human-readable message when no usable build is found (missing, incomplete, + * failed) or the artifact can't be downloaded. */ export async function getArtifactForCommit( args: GetArtifactForCommitArgs, -): Promise { +): Promise { const { adoApi, artifactName, commit, definitionId, project } = args; const builds = await getRecentBuilds(adoApi, project, definitionId); - - const buildLookup = findBuildIdForCommit(builds, commit); - if (buildLookup.kind === "error") { - return buildLookup; - } + const buildId = findBuildIdForCommit(builds, commit); try { - const contents = await downloadArtifact( - adoApi, - project, - buildLookup.buildId, - artifactName, - ); - return { kind: "found", contents }; + return await downloadArtifact(adoApi, project, buildId, artifactName); } catch (e) { - return { - kind: "error", - error: `Build for commit ${commit} did not publish artifact "${artifactName}": ${e instanceof Error ? e.message : String(e)}`, - }; + throw new Error(`Could not download artifact "${artifactName}" for commit ${commit}`, { + cause: e, + }); } } From bfb89dcfcd19fb3447e497bd691c8c5cf8db832d Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Fri, 15 May 2026 15:02:42 -0700 Subject: [PATCH 11/21] Check the HTTP status before unzipping in downloadArtifact MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `buildApi.getArtifactContentZip` returns a `ReadableStream` per its declared type, but the underlying object is a Node `http.IncomingMessage` and the SDK doesn't throw on non-2xx for this endpoint. A missing artifact comes back as a 404 with a non-zip body, which fflate then greets with the unhelpful "invalid zip data" error. Peek at `statusCode` via a `Partial` assertion and throw a meaningful error before unzip. The assertion is defensive — if a future SDK update returns something without a `statusCode`, the existing unzip-time error remains the fallback. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/library/azureDevops/downloadArtifact.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/build-tools/packages/build-cli/src/library/azureDevops/downloadArtifact.ts b/build-tools/packages/build-cli/src/library/azureDevops/downloadArtifact.ts index 13e48e178d79..92dd9b9b3632 100644 --- a/build-tools/packages/build-cli/src/library/azureDevops/downloadArtifact.ts +++ b/build-tools/packages/build-cli/src/library/azureDevops/downloadArtifact.ts @@ -4,6 +4,7 @@ */ import { strict as assert } from "node:assert"; +import type { IncomingMessage } from "node:http"; import type { WebApi } from "azure-devops-node-api"; import { unzipSync } from "fflate"; @@ -44,6 +45,18 @@ export async function downloadArtifact( buildApi.createAcceptHeader = originalCreateAcceptHeader; } + // The declared `ReadableStream` return type hides it, but the SDK actually + // returns an `http.IncomingMessage` and doesn't throw on non-2xx responses + // for this endpoint — a missing artifact comes back as a 404 with a + // non-zip body that would otherwise blow up inside `unzipSync` with a + // useless "invalid zip data" error. + const statusCode = (artifactStream as Partial).statusCode; + if (statusCode !== undefined && (statusCode < 200 || statusCode >= 300)) { + throw new Error( + `Failed to download artifact "${artifactName}" from build ${buildId} (HTTP ${statusCode})`, + ); + } + const unzipped = unzipSync(await readStreamAsUint8Array(artifactStream)); // ADO wraps the artifact contents in a top-level folder named after the artifact. From 9eff83b6dd62220c87629060c5661450d6f6d323 Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Fri, 15 May 2026 15:14:54 -0700 Subject: [PATCH 12/21] Simplify sourcePackageFromAnalyzerPath to "strip the analyzer.json suffix" The split/check/pop/length/join chain hid a simple intent: if the path ends with `/analyzer.json`, return everything before it; else undefined. Replace with `endsWith` + `slice`. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../bundleSize/sourcePackageFromAnalyzerPath.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/build-tools/packages/build-cli/src/library/bundleSize/sourcePackageFromAnalyzerPath.ts b/build-tools/packages/build-cli/src/library/bundleSize/sourcePackageFromAnalyzerPath.ts index dae7040da53c..be7167dc22b6 100644 --- a/build-tools/packages/build-cli/src/library/bundleSize/sourcePackageFromAnalyzerPath.ts +++ b/build-tools/packages/build-cli/src/library/bundleSize/sourcePackageFromAnalyzerPath.ts @@ -3,7 +3,7 @@ * Licensed under the MIT License. */ -const analyzerJsonFileName = "analyzer.json"; +const analyzerJsonSuffix = "/analyzer.json"; /** * If `relativePath` looks like `/analyzer.json` (nested layout — @@ -14,9 +14,8 @@ const analyzerJsonFileName = "analyzer.json"; * POSIX path separators. */ export function sourcePackageFromAnalyzerPath(relativePath: string): string | undefined { - const pathParts = relativePath.replace(/\\/g, "/").split("/"); - if (pathParts.at(-1) !== analyzerJsonFileName) return undefined; - pathParts.pop(); - if (pathParts.length === 0) return undefined; - return pathParts.join("/"); + const normalized = relativePath.replace(/\\/g, "/"); + if (!normalized.endsWith(analyzerJsonSuffix)) return undefined; + const sourcePackage = normalized.slice(0, -analyzerJsonSuffix.length); + return sourcePackage.length === 0 ? undefined : sourcePackage; } From 3c74c825157358f11f44e9bf6e4e942166e030be Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Fri, 15 May 2026 15:21:49 -0700 Subject: [PATCH 13/21] Merge compareJsonReportsByPackage into compareJsonReports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two functions are tightly coupled — `compareJsonReportsByPackage` is just a per-package fanout of `compareJsonReports`, with no external caller for the per-package function. Move both into the same file, unexport `compareJsonReports` (now a private helper), delete the empty `compareJsonReportsByPackage.ts`, and update the barrel to re-export only the public `compareJsonReportsByPackage`. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../library/bundleSize/compareJsonReports.ts | 30 +++++++++++++++++-- .../bundleSize/compareJsonReportsByPackage.ts | 28 ----------------- .../build-cli/src/library/bundleSize/index.ts | 3 +- 3 files changed, 29 insertions(+), 32 deletions(-) delete mode 100644 build-tools/packages/build-cli/src/library/bundleSize/compareJsonReportsByPackage.ts diff --git a/build-tools/packages/build-cli/src/library/bundleSize/compareJsonReports.ts b/build-tools/packages/build-cli/src/library/bundleSize/compareJsonReports.ts index 9b1980abbdfe..e2fd37e387f1 100644 --- a/build-tools/packages/build-cli/src/library/bundleSize/compareJsonReports.ts +++ b/build-tools/packages/build-cli/src/library/bundleSize/compareJsonReports.ts @@ -5,7 +5,12 @@ import type { BundleAnalyzerPlugin } from "webpack-bundle-analyzer"; -import type { BundleData, BundlesComparison } from "./types.js"; +import type { + AnalyzerJsonByPackage, + BundleData, + BundlesComparison, + PackageComparison, +} from "./types.js"; /** * Filter `report` to its asset entries and key their size data by asset name. @@ -35,7 +40,7 @@ function jsonReportToBundleSizes( * Bundles present only in one side encode added/removed via field presence * (see {@link BundlesComparison}). */ -export function compareJsonReports( +function compareJsonReports( base: BundleAnalyzerPlugin.JsonReport | undefined, compare: BundleAnalyzerPlugin.JsonReport | undefined, ): BundlesComparison { @@ -56,3 +61,24 @@ export function compareJsonReports( return bundles; } + +/** + * Compare per-package `JsonReport`s for two snapshots and produce a + * {@link PackageComparison}. Iterates the union of source packages so packages + * present only on one side are represented (their `compareJsonReports` call + * treats the absent side as empty). + */ +export function compareJsonReportsByPackage( + base: AnalyzerJsonByPackage, + compare: AnalyzerJsonByPackage, +): PackageComparison { + const allPackages = new Set([...base.keys(), ...compare.keys()]); + const result: PackageComparison = {}; + for (const sourcePackage of allPackages) { + result[sourcePackage] = compareJsonReports( + base.get(sourcePackage), + compare.get(sourcePackage), + ); + } + return result; +} diff --git a/build-tools/packages/build-cli/src/library/bundleSize/compareJsonReportsByPackage.ts b/build-tools/packages/build-cli/src/library/bundleSize/compareJsonReportsByPackage.ts deleted file mode 100644 index 2fdde36ebfb0..000000000000 --- a/build-tools/packages/build-cli/src/library/bundleSize/compareJsonReportsByPackage.ts +++ /dev/null @@ -1,28 +0,0 @@ -/*! - * Copyright (c) Microsoft Corporation and contributors. All rights reserved. - * Licensed under the MIT License. - */ - -import { compareJsonReports } from "./compareJsonReports.js"; -import type { AnalyzerJsonByPackage, PackageComparison } from "./types.js"; - -/** - * Compare per-package `JsonReport`s for two snapshots and produce a - * {@link PackageComparison}. Iterates the union of source packages so packages - * present only on one side are represented (their `compareJsonReports` call - * treats the absent side as empty). - */ -export function compareJsonReportsByPackage( - base: AnalyzerJsonByPackage, - compare: AnalyzerJsonByPackage, -): PackageComparison { - const allPackages = new Set([...base.keys(), ...compare.keys()]); - const result: PackageComparison = {}; - for (const sourcePackage of allPackages) { - result[sourcePackage] = compareJsonReports( - base.get(sourcePackage), - compare.get(sourcePackage), - ); - } - return result; -} diff --git a/build-tools/packages/build-cli/src/library/bundleSize/index.ts b/build-tools/packages/build-cli/src/library/bundleSize/index.ts index 6b9e96b624a7..89156a97c889 100644 --- a/build-tools/packages/build-cli/src/library/bundleSize/index.ts +++ b/build-tools/packages/build-cli/src/library/bundleSize/index.ts @@ -3,8 +3,7 @@ * Licensed under the MIT License. */ -export { compareJsonReports } from "./compareJsonReports.js"; -export { compareJsonReportsByPackage } from "./compareJsonReportsByPackage.js"; +export { compareJsonReportsByPackage } from "./compareJsonReports.js"; export { extractAnalyzerJsonsFromArtifact } from "./extractAnalyzerJsonsFromArtifact.js"; export { readAnalyzerJsonsFromFileSystem } from "./readAnalyzerJsonsFromFileSystem.js"; export { sourcePackageFromAnalyzerPath } from "./sourcePackageFromAnalyzerPath.js"; From 680d3b3bdebad2b335b1f0d632fc8a8f3bc32558 Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Fri, 15 May 2026 15:32:37 -0700 Subject: [PATCH 14/21] Drop the no-changes / changes discriminator from CheckBundleSizeResult The kind flag was redundant with the data. A JSON consumer can compute "did anything change?" themselves (and may want their own threshold); the human display logic pivots on `changeLines.length === 0` locally and doesn't need it threaded through the return type. Result type collapses to `{ baselineCommit, comparison }`. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../build-cli/src/commands/check/bundleSize.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts index 4de309b7fd28..c14bb3283329 100644 --- a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts +++ b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts @@ -32,9 +32,10 @@ const defaultLocalReportPath = "./artifacts/bundleAnalyzerJson"; * Result serialized to stdout by `--json`. Default invocations print a * human-readable summary instead. */ -type CheckBundleSizeResult = - | { kind: "no-changes"; baselineCommit: string } - | { kind: "changes"; baselineCommit: string; comparison: PackageComparison }; +interface CheckBundleSizeResult { + baselineCommit: string; + comparison: PackageComparison; +} /** * Render a {@link PackageComparison} as a flat list of human-readable lines. @@ -148,12 +149,11 @@ export default class CheckBundleSize extends BaseCommand if (changeLines.length === 0) { this.log(`No bundle size changes vs baseline commit ${baselineCommit}.`); - return { kind: "no-changes", baselineCommit }; + } else { + this.log(`Bundle size changes vs baseline commit ${baselineCommit}:`); + for (const line of changeLines) this.log(line); } - this.log(`Bundle size changes vs baseline commit ${baselineCommit}:`); - for (const line of changeLines) this.log(line); - - return { kind: "changes", baselineCommit, comparison }; + return { baselineCommit, comparison }; } } From 271a59a97e06204248199e4da3a4f8bd8dbe27dd Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Fri, 15 May 2026 15:39:55 -0700 Subject: [PATCH 15/21] Inline adoConstants into their usage sites The constants block was only consumed in one place. Inlining surfaces the values where they're used and lets each one carry a targeted inline comment. Also drops the speculative "automated consumers authenticate at the library layer" clause that didn't refer to any actual code path. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/commands/check/bundleSize.ts | 22 +++++++------------ 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts index c14bb3283329..0eaff7c1388c 100644 --- a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts +++ b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts @@ -17,14 +17,6 @@ import { import { BaseCommand } from "../../library/commands/base.js"; import { pickFreshestRemote } from "../../library/git/pickFreshestRemote.js"; -// Must match the "public" project + build-bundle-size-artifacts.yml (definitionId 48). -const adoConstants = { - orgUrl: "https://dev.azure.com/fluidframework", - projectName: "public", - ciBuildDefinitionId: 48, - artifactName: "bundleAnalyzerJson", -} as const; - // Where `flub generate bundleStats` (via `npm run bundle-analysis:collect`) writes. const defaultLocalReportPath = "./artifacts/bundleAnalyzerJson"; @@ -124,15 +116,17 @@ export default class CheckBundleSize extends BaseCommand .trim(); this.log(`Baseline commit: ${baselineCommit}`); - // Anonymous reads work for the public ADO project at this command's scale; - // automated consumers authenticate at the library layer. - const adoApi = getAzureDevopsApi(undefined, adoConstants.orgUrl); + // Public ADO project — anonymous reads are fine at this command's scale. + const adoApi = getAzureDevopsApi(undefined, "https://dev.azure.com/fluidframework"); const artifactContents = await getArtifactForCommit({ adoApi, - artifactName: adoConstants.artifactName, + // Published by the `Build - Client bundle size artifacts` pipeline. + artifactName: "bundleAnalyzerJson", commit: baselineCommit, - definitionId: adoConstants.ciBuildDefinitionId, - project: adoConstants.projectName, + // `Build - Client bundle size artifacts` in the `public` project. + // Source-of-truth: tools/pipelines/build-bundle-size-artifacts.yml. + definitionId: 48, + project: "public", }); const baselineJsons = extractAnalyzerJsonsFromArtifact(artifactContents); From 4f4eea1ca570dcebc14d9130b8a85243fdfa7a00 Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Fri, 15 May 2026 15:44:18 -0700 Subject: [PATCH 16/21] Rename prJsons -> compareJsons in check bundleSize "PR" presumed the inner-dev-loop user is on a feature branch about to open a PR. They might just be checking local impact. `compareJsons` matches the `BundlesComparison.compare` field and `compareJsonReportsByPackage`'s `compare` parameter. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../packages/build-cli/src/commands/check/bundleSize.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts index 0eaff7c1388c..267efe806ff9 100644 --- a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts +++ b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts @@ -130,15 +130,15 @@ export default class CheckBundleSize extends BaseCommand }); const baselineJsons = extractAnalyzerJsonsFromArtifact(artifactContents); - const prJsons = await readAnalyzerJsonsFromFileSystem(localReportPath); + const compareJsons = await readAnalyzerJsonsFromFileSystem(localReportPath); - if (baselineJsons.size === 0 && prJsons.size === 0) { + if (baselineJsons.size === 0 && compareJsons.size === 0) { this.error( "No bundles to compare — baseline artifact and local bundle reports are both empty.", ); } - const comparison = compareJsonReportsByPackage(baselineJsons, prJsons); + const comparison = compareJsonReportsByPackage(baselineJsons, compareJsons); const changeLines = formatComparison(comparison); if (changeLines.length === 0) { From 7c5d2d13fb786919be1a7ed68242f30399800b1c Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Fri, 15 May 2026 16:28:18 -0700 Subject: [PATCH 17/21] Wrap two common failure paths with actionable error messages - `readAnalyzerJsonsFromFileSystem` catches `ENOENT` on the directory walk and rethrows with a pointer to `pnpm bundle-analysis:collect`, so first-time users don't see a raw `ENOENT: no such file or directory` with no guidance. - The `git merge-base` shell-out wraps `execFileSync` failures with `this.error()`, pointing at "ensure it is fetched locally, or pass --target to override." Captures git's stderr too instead of letting it leak as a parallel line above the wrapper. Also updates a doc comment from `npm run bundle-analysis:collect` to `pnpm bundle-analysis:collect`, the convention in this repo. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/commands/check/bundleSize.ts | 20 +++++++++++++++---- .../readAnalyzerJsonsFromFileSystem.ts | 16 ++++++++++++++- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts index 267efe806ff9..9918e9d81e40 100644 --- a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts +++ b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts @@ -17,7 +17,7 @@ import { import { BaseCommand } from "../../library/commands/base.js"; import { pickFreshestRemote } from "../../library/git/pickFreshestRemote.js"; -// Where `flub generate bundleStats` (via `npm run bundle-analysis:collect`) writes. +// Where `flub generate bundleStats` (via `pnpm bundle-analysis:collect`) writes. const defaultLocalReportPath = "./artifacts/bundleAnalyzerJson"; /** @@ -111,9 +111,21 @@ export default class CheckBundleSize extends BaseCommand this.log(`Using target ref ${targetRef}. Pass --target to override.`); } - const baselineCommit = execFileSync("git", ["merge-base", targetRef, "HEAD"]) - .toString() - .trim(); + let baselineCommit: string; + try { + baselineCommit = execFileSync("git", ["merge-base", targetRef, "HEAD"], { + stdio: ["ignore", "pipe", "pipe"], + }) + .toString() + .trim(); + } catch (e) { + const stderr = (e as { stderr?: Buffer }).stderr?.toString().trim(); + this.error( + `Could not determine merge-base for ref "${targetRef}". Ensure it is fetched locally, or pass --target to override.${ + stderr ? `\n${stderr}` : "" + }`, + ); + } this.log(`Baseline commit: ${baselineCommit}`); // Public ADO project — anonymous reads are fine at this command's scale. diff --git a/build-tools/packages/build-cli/src/library/bundleSize/readAnalyzerJsonsFromFileSystem.ts b/build-tools/packages/build-cli/src/library/bundleSize/readAnalyzerJsonsFromFileSystem.ts index 868d25536b59..0245f124e959 100644 --- a/build-tools/packages/build-cli/src/library/bundleSize/readAnalyzerJsonsFromFileSystem.ts +++ b/build-tools/packages/build-cli/src/library/bundleSize/readAnalyzerJsonsFromFileSystem.ts @@ -37,11 +37,25 @@ async function getAllFilesInDirectory( /** * Walks `rootPath`, finds every `analyzer.json` file, parses it, and keys the * results by source package. + * + * Throws with a contextual message when `rootPath` doesn't exist — that's the + * common path for a user who hasn't yet run `npm run bundle-analysis:collect`. */ export async function readAnalyzerJsonsFromFileSystem( rootPath: string, ): Promise { - const allPaths = await getAllFilesInDirectory(rootPath); + let allPaths: string[]; + try { + allPaths = await getAllFilesInDirectory(rootPath); + } catch (e) { + if ((e as NodeJS.ErrnoException).code === "ENOENT") { + throw new Error( + `Local bundle report directory not found at "${rootPath}". Run \`pnpm bundle-analysis:collect\` to generate it.`, + { cause: e }, + ); + } + throw e; + } const result: AnalyzerJsonByPackage = new Map(); const reads: Promise[] = []; for (const relativePath of allPaths) { From 8ee6a42acd8a1f62d4c83eb18d4e3a7682424e0d Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Fri, 15 May 2026 16:48:46 -0700 Subject: [PATCH 18/21] Surface empty-side maps separately in check bundleSize MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous guard only fired when *both* sides had no analyzer.json files. The asymmetric case (the local directory exists but is empty — e.g., bundle-analysis:collect partly failed, or the user manually emptied it) silently fed `compareJsonReportsByPackage` an empty `compare` map, producing a comparison that reported every baseline bundle as "removed". The user saw "everything was deleted" with no hint that the local data was actually missing. Check each side individually now: an empty baseline (rare — implies the artifact had no analyzer.json files) and an empty local dir get distinct, actionable messages. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../packages/build-cli/src/commands/check/bundleSize.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts index 9918e9d81e40..4629221fb70d 100644 --- a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts +++ b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts @@ -144,9 +144,14 @@ export default class CheckBundleSize extends BaseCommand const baselineJsons = extractAnalyzerJsonsFromArtifact(artifactContents); const compareJsons = await readAnalyzerJsonsFromFileSystem(localReportPath); - if (baselineJsons.size === 0 && compareJsons.size === 0) { + if (baselineJsons.size === 0) { this.error( - "No bundles to compare — baseline artifact and local bundle reports are both empty.", + `Baseline artifact contains no analyzer.json files for commit ${baselineCommit}.`, + ); + } + if (compareJsons.size === 0) { + this.error( + `Local bundle report directory "${localReportPath}" exists but contains no analyzer.json files. Run \`pnpm bundle-analysis:collect\` to populate it.`, ); } From 13af3d6bc3fc5fc40252d12efa8b2ff9795a6285 Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Fri, 15 May 2026 18:15:58 -0700 Subject: [PATCH 19/21] Pre-check local reports with a discriminated check function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the previous "is local empty?" guard that only fired when both sides were empty: a local directory that exists but is empty (or holds files but no analyzer.json) silently produced an "everything removed" diff. Catch it before the walk. - New `checkLocalBundleAnalysisExists(rootPath)` returns `"ok" | "missing" | "noAnalyzerJson"`. The library function does no message-building; the command decides what to print and appends the `pnpm bundle-analysis:collect` hint only on the default path (overrides are populated from some source we don't know about). - Internals use `statSync({ throwIfNoEntry: false })` and `globSync` — single-shot calls where async buys nothing. Function is sync. - `readAnalyzerJsonsFromFileSystem` switched to `globSync(...).map(read) + Promise.all` (cleaner than the for-await-with-explicit-reads-array). - Direct `readFile` import from `node:fs/promises` instead of aliasing the whole promises namespace for one function. - Baseline-empty check moves to immediately after baseline extraction. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/commands/check/bundleSize.ts | 19 +++-- .../build-cli/src/library/bundleSize/index.ts | 5 +- .../readAnalyzerJsonsFromFileSystem.ts | 71 +++++++------------ 3 files changed, 43 insertions(+), 52 deletions(-) diff --git a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts index 4629221fb70d..b6c0e349c25b 100644 --- a/build-tools/packages/build-cli/src/commands/check/bundleSize.ts +++ b/build-tools/packages/build-cli/src/commands/check/bundleSize.ts @@ -9,6 +9,7 @@ import { Flags } from "@oclif/core"; import { getArtifactForCommit } from "../../library/azureDevops/getArtifactForCommit.js"; import { getAzureDevopsApi } from "../../library/azureDevops/getAzureDevopsApi.js"; import { + checkLocalBundleAnalysisExists, compareJsonReportsByPackage, extractAnalyzerJsonsFromArtifact, type PackageComparison, @@ -142,18 +143,28 @@ export default class CheckBundleSize extends BaseCommand }); const baselineJsons = extractAnalyzerJsonsFromArtifact(artifactContents); - const compareJsons = await readAnalyzerJsonsFromFileSystem(localReportPath); - if (baselineJsons.size === 0) { this.error( `Baseline artifact contains no analyzer.json files for commit ${baselineCommit}.`, ); } - if (compareJsons.size === 0) { + + const localCheck = checkLocalBundleAnalysisExists(localReportPath); + // Append the `pnpm bundle-analysis:collect` hint only on the default + // path — overrides are populated from some source we don't know about. + const hint = + localReportPath === defaultLocalReportPath + ? " Run `pnpm bundle-analysis:collect` to populate it." + : ""; + if (localCheck === "missing") { + this.error(`Local bundle report directory not found at "${localReportPath}".${hint}`); + } + if (localCheck === "noAnalyzerJson") { this.error( - `Local bundle report directory "${localReportPath}" exists but contains no analyzer.json files. Run \`pnpm bundle-analysis:collect\` to populate it.`, + `Local bundle report directory "${localReportPath}" contains no analyzer.json files.${hint}`, ); } + const compareJsons = await readAnalyzerJsonsFromFileSystem(localReportPath); const comparison = compareJsonReportsByPackage(baselineJsons, compareJsons); const changeLines = formatComparison(comparison); diff --git a/build-tools/packages/build-cli/src/library/bundleSize/index.ts b/build-tools/packages/build-cli/src/library/bundleSize/index.ts index 89156a97c889..a00bb8ba49ba 100644 --- a/build-tools/packages/build-cli/src/library/bundleSize/index.ts +++ b/build-tools/packages/build-cli/src/library/bundleSize/index.ts @@ -5,7 +5,10 @@ export { compareJsonReportsByPackage } from "./compareJsonReports.js"; export { extractAnalyzerJsonsFromArtifact } from "./extractAnalyzerJsonsFromArtifact.js"; -export { readAnalyzerJsonsFromFileSystem } from "./readAnalyzerJsonsFromFileSystem.js"; +export { + checkLocalBundleAnalysisExists, + readAnalyzerJsonsFromFileSystem, +} from "./readAnalyzerJsonsFromFileSystem.js"; export { sourcePackageFromAnalyzerPath } from "./sourcePackageFromAnalyzerPath.js"; export { AnalyzerJsonByPackage, diff --git a/build-tools/packages/build-cli/src/library/bundleSize/readAnalyzerJsonsFromFileSystem.ts b/build-tools/packages/build-cli/src/library/bundleSize/readAnalyzerJsonsFromFileSystem.ts index 0245f124e959..1925efacbb9a 100644 --- a/build-tools/packages/build-cli/src/library/bundleSize/readAnalyzerJsonsFromFileSystem.ts +++ b/build-tools/packages/build-cli/src/library/bundleSize/readAnalyzerJsonsFromFileSystem.ts @@ -3,70 +3,47 @@ * Licensed under the MIT License. */ -import { promises as fsPromises } from "fs"; -import { join } from "path"; +import { globSync, statSync } from "node:fs"; +import { readFile } from "node:fs/promises"; +import { join } from "node:path"; import type { BundleAnalyzerPlugin } from "webpack-bundle-analyzer"; import { sourcePackageFromAnalyzerPath } from "./sourcePackageFromAnalyzerPath.js"; import type { AnalyzerJsonByPackage } from "./types.js"; +const analyzerJsonGlob = "**/analyzer.json"; + /** - * Recursively walk `sourceFolder`, returning the relative path of every file. + * Check whether `rootPath` exists and contains any `analyzer.json` file. + * + * @returns `"ok"`, `"missing"` (rootPath doesn't exist), or `"noAnalyzerJson"` + * (rootPath exists but the tree has none). Real failures (permission errors, + * broken symlinks, …) propagate as-is rather than collapsing to a kind. */ -async function getAllFilesInDirectory( - sourceFolder: string, - partialPathPrefix: string = "", -): Promise { - const result: string[] = []; - for (const file of await fsPromises.readdir(sourceFolder)) { - const fullPath = join(sourceFolder, file); - if ((await fsPromises.stat(fullPath)).isFile()) { - result.push(join(partialPathPrefix, file)); - } else { - result.push( - ...(await getAllFilesInDirectory( - join(sourceFolder, file), - join(partialPathPrefix, file), - )), - ); - } +export function checkLocalBundleAnalysisExists( + rootPath: string, +): "ok" | "missing" | "noAnalyzerJson" { + if (statSync(rootPath, { throwIfNoEntry: false }) === undefined) { + return "missing"; } - return result; + return globSync(analyzerJsonGlob, { cwd: rootPath }).length > 0 ? "ok" : "noAnalyzerJson"; } /** * Walks `rootPath`, finds every `analyzer.json` file, parses it, and keys the * results by source package. - * - * Throws with a contextual message when `rootPath` doesn't exist — that's the - * common path for a user who hasn't yet run `npm run bundle-analysis:collect`. */ export async function readAnalyzerJsonsFromFileSystem( rootPath: string, ): Promise { - let allPaths: string[]; - try { - allPaths = await getAllFilesInDirectory(rootPath); - } catch (e) { - if ((e as NodeJS.ErrnoException).code === "ENOENT") { - throw new Error( - `Local bundle report directory not found at "${rootPath}". Run \`pnpm bundle-analysis:collect\` to generate it.`, - { cause: e }, - ); - } - throw e; - } const result: AnalyzerJsonByPackage = new Map(); - const reads: Promise[] = []; - for (const relativePath of allPaths) { - const sourcePackage = sourcePackageFromAnalyzerPath(relativePath); - if (sourcePackage === undefined) continue; - reads.push( - fsPromises.readFile(join(rootPath, relativePath), "utf8").then((text) => { - result.set(sourcePackage, JSON.parse(text) as BundleAnalyzerPlugin.JsonReport); - }), - ); - } - await Promise.all(reads); + await Promise.all( + globSync(analyzerJsonGlob, { cwd: rootPath }).map(async (relativePath) => { + const sourcePackage = sourcePackageFromAnalyzerPath(relativePath); + if (sourcePackage === undefined) return; + const text = await readFile(join(rootPath, relativePath), "utf8"); + result.set(sourcePackage, JSON.parse(text) as BundleAnalyzerPlugin.JsonReport); + }), + ); return result; } From 4d2e8707839b4027a82fec74551874ba68fbe146 Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Fri, 15 May 2026 18:40:57 -0700 Subject: [PATCH 20/21] Scan all ADO builds for a commit, not just the first match A commit can have multiple ADO builds (manual re-run, partial-success retry); `Array.find` could lock onto an older failed one even when a newer succeeded build exists. Filter all matches, prefer a usable one, and report the most actionable failure state across candidates. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../azureDevops/getArtifactForCommit.ts | 48 ++++++++++++++----- 1 file changed, 35 insertions(+), 13 deletions(-) diff --git a/build-tools/packages/build-cli/src/library/azureDevops/getArtifactForCommit.ts b/build-tools/packages/build-cli/src/library/azureDevops/getArtifactForCommit.ts index 4786dbe68660..f4013b123c40 100644 --- a/build-tools/packages/build-cli/src/library/azureDevops/getArtifactForCommit.ts +++ b/build-tools/packages/build-cli/src/library/azureDevops/getArtifactForCommit.ts @@ -47,29 +47,51 @@ async function getRecentBuilds( } /** - * Find the build for `commit` in `builds` and validate that it has an id, - * is completed, and succeeded. + * Find a usable build for `commit` in `builds` — one with an id, status + * Completed, and result Succeeded. A commit can have more than one ADO build + * (manual re-run, partial-success retry, …), so scan all matches rather than + * locking onto the first one ADO returned. * * @returns The build id. Throws with a human-readable message when no usable - * build is found. + * build is found, prioritizing "not yet completed" over "did not succeed" + * since retrying later might help. */ function findBuildIdForCommit(builds: Build[], commit: string): number { - const build = builds.find((b) => b.sourceVersion === commit); + const candidates = builds.filter((b) => b.sourceVersion === commit); - if (build === undefined) { + if (candidates.length === 0) { throw new Error(`No build found for commit ${commit}`); } - if (build.id === undefined) { - throw new Error(`Build for commit ${commit} does not have a build id`); + + const usable = candidates.find( + (b): b is Build & { id: number } => + b.id !== undefined && + b.status === BuildStatus.Completed && + b.result === BuildResult.Succeeded, + ); + if (usable !== undefined) { + return usable.id; } - if (build.status !== BuildStatus.Completed) { - throw new Error(`Build for commit ${commit} has not yet completed.`); + + // No usable found — report the most actionable state across the candidates. + // "Actively running" gets priority since the user might just need to wait; + // Cancelling is *not* in that bucket because it's heading toward Canceled. + const isActivelyRunning = (b: Build): boolean => + b.status === BuildStatus.NotStarted || + b.status === BuildStatus.InProgress || + b.status === BuildStatus.Postponed; + if (candidates.some(isActivelyRunning)) { + throw new Error( + `Found an in-progress build for commit ${commit}; none have succeeded yet.`, + ); } - if (build.result !== BuildResult.Succeeded) { - throw new Error(`Build for commit ${commit} did not succeed.`); + if (candidates.some((b) => b.result !== BuildResult.Succeeded)) { + throw new Error(`All builds for commit ${commit} have completed but none succeeded.`); } - - return build.id; + // Reaching here means every candidate is Completed + Succeeded but missing + // an `id` — an ADO state anomaly that shouldn't happen in practice, but the + // `id` field is typed `number | undefined` so we surface it explicitly. + throw new Error(`No build for commit ${commit} has a usable build id.`); } export interface GetArtifactForCommitArgs { From 56973772c455d39a3eb8ba7bd52bcf961f4fdb39 Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Mon, 18 May 2026 09:55:14 -0700 Subject: [PATCH 21/21] Fix "none succeeded" throw to require every candidate failed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `some` form fires whenever any candidate failed, including the mixed case where one succeeded (but had no `id`) alongside a failure — producing the misleading "none succeeded" message. `every` correctly limits that throw to the all-failed case; the mixed-success-no-id case falls through to the final "no usable build id" throw. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/library/azureDevops/getArtifactForCommit.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/build-tools/packages/build-cli/src/library/azureDevops/getArtifactForCommit.ts b/build-tools/packages/build-cli/src/library/azureDevops/getArtifactForCommit.ts index f4013b123c40..6483f75b70f2 100644 --- a/build-tools/packages/build-cli/src/library/azureDevops/getArtifactForCommit.ts +++ b/build-tools/packages/build-cli/src/library/azureDevops/getArtifactForCommit.ts @@ -85,12 +85,13 @@ function findBuildIdForCommit(builds: Build[], commit: string): number { `Found an in-progress build for commit ${commit}; none have succeeded yet.`, ); } - if (candidates.some((b) => b.result !== BuildResult.Succeeded)) { + if (candidates.every((b) => b.result !== BuildResult.Succeeded)) { throw new Error(`All builds for commit ${commit} have completed but none succeeded.`); } - // Reaching here means every candidate is Completed + Succeeded but missing - // an `id` — an ADO state anomaly that shouldn't happen in practice, but the - // `id` field is typed `number | undefined` so we surface it explicitly. + // Reaching here means at least one candidate Succeeded but is missing an + // `id` (possibly alongside other failed candidates) — an ADO state anomaly + // that shouldn't happen in practice, but the `id` field is typed + // `number | undefined` so we surface it explicitly. throw new Error(`No build for commit ${commit} has a usable build id.`); }