Skip to content

chore(bundle-size-tools): drop unused/flawed legacy behaviors#27242

Merged
ChumpChief merged 4 commits intomicrosoft:mainfrom
ChumpChief:bundle-size-tools-behavior-cleanup
May 6, 2026
Merged

chore(bundle-size-tools): drop unused/flawed legacy behaviors#27242
ChumpChief merged 4 commits intomicrosoft:mainfrom
ChumpChief:bundle-size-tools-behavior-cleanup

Conversation

@ChumpChief
Copy link
Copy Markdown
Contributor

Description

Follow-up to #27224 (analyzer.json swap) and #27239 (dead-code cleanup). Part of AB#56981 (re-enable PR bundle size comparison).

Three pre-existing behaviors are removed because they don't fit the bundleSizeDiff design that #27158 began (a producer that emits a structured artifact, consumed by a future GitHub Actions workflow). Each is independent; commits are split for review.

1. sizeRegressionDetected field on bundleSizeDiff output (commit 1)

Background. flub generate bundleSizeDiff runs a producer-side regression check: any non-total metric growing by more than a hardcoded 5KB threshold sets a sizeRegressionDetected: boolean field on result.json's "changes" variant.

Why remove. That threshold is a policy decision, not raw data. The broader design has the producer emit unopinionated per-bundle deltas and the consumer (the future GH Actions workflow that will post PR comments / manage labels) apply its own threshold. Producer-side hardcoding pre-commits us to one definition of "regression" that the consumer can't override.

Removed.

  • detectSizeRegression function
  • sizeRegressionThresholdBytes constant
  • sizeRegressionDetected field from BundleSizeDiffResult's "changes" variant
  • BundleMetric type import (only used by the removed function)

Compatibility. No consumer reads result.json yet — the GH workflow that will is a future PR — so this is a no-op at the seam and lets that future consumer pick its own threshold.

2. tagWaiting mechanism in ADOSizeComparator (commit 2)

Background. When the comparator runs against a baseline commit whose CI build hasn't completed yet, it tags the PR's own build with a marker (via tagBuildAsWaiting) so that a downstream process could re-run the comparison once the baseline finished.

Why remove. The downstream half (consuming the tag and retriggering the PR build) was never written, so the tagging is effectively no-op overhead today. The new bundleSizeDiff design — and PR 3's planned scheduled re-run — replaces this need entirely with workflow-side polling.

Removed.

  • bundle-size-tools/src/ADO/getBuildTagForCommit.ts (whole file)
  • ADOSizeComparator.tagBuildAsWaiting private method
  • ADOSizeComparator constructor param adoBuildId
  • getSizeComparison's tagWaiting parameter
  • IADOConstants fields prBuildDefinitionId and projectRepoGuid (only used by tagWaiting and the now-gone PR-comments machinery)
  • Corresponding barrel exports

3. Naive fallback in ADOSizeComparator (commit 2)

Background. When no CI build is found for the baseline commit (or the build has no usable artifact), the comparator walks prior commits via getPriorCommit looking for one with a usable build, and compares the PR against that.

Why remove. The resulting comparison is incorrect-by-design: the PR is being compared against a commit on main that's N commits older than the merge-base, so main-side deltas between merge-base and merge-base−N get falsely attributed to the PR.

Removed.

  • ADOSizeComparator.naiveFallbackCommitGenerator static method
  • ADOSizeComparator constructor param getFallbackCommit
  • utilities/gitCommands.ts's getPriorCommit (only used by the fallback)
  • Corresponding barrel exports

Cleanup side-effect. With both tagWaiting and the fallback gone, getSizeComparison flattens from a while-loop with continue/fallback branches into a linear sequence of early-return validations, and error messages become more specific — "No CI build found for baseline commit X" / "Baseline build did not publish bundle artifacts" instead of the generic "Could not find a usable baseline build".

4. BannedModulesPlugin (commit 3)

Background. A custom Webpack plugin shipped by bundle-size-tools and used by bundle-size-tests to fail the build if any chunk imports a banned module. The only banned module configured today is Node-core assert, whose polyfill is large in browser builds.

Why remove. Webpack 5 supports the same effect natively via resolve.fallback: { assert: false }, which blocks the polyfill from being bundled at all. The plugin existed because earlier Webpack versions didn't have this option. The native config is a few lines; the plugin is a ~70-line file plus the burden of being a public API export of bundle-size-tools.

Removed.

  • bundle-size-tools/src/bannedModulesPlugin/bannedModulesPlugin.ts (whole file)
  • BannedModule, BannedModulesPlugin, BannedModulesPluginOptions exports

Dependency drops (consequences of the swap):

  • webpack removed from bundle-size-tools/package.json (was only used by the plugin)
  • @fluidframework/bundle-size-tools removed from bundle-size-tests/package.json devDependencies (was only used here for the plugin)

Reviewer Guidance

The review process is outlined on this wiki page.

Three independent removals, each in its own commit — easy to review commit-by-commit.

ChumpChief and others added 3 commits May 6, 2026 08:58
The producer-side regression-detection check (any non-total metric growing
by more than 5KB triggers a `sizeRegressionDetected: boolean` field on the
result) was a hardcoded threshold baked into the producer. Per the broader
design direction, the producer should be unopinionated about thresholds and
emit raw size data; the consumer (the future GH Actions workflow that
posts PR comments / manages labels) decides what counts as a regression.

Removes:
- detectSizeRegression function
- sizeRegressionThresholdBytes constant
- sizeRegressionDetected field from BundleSizeDiffResult's "changes" variant
- BundleMetric type import (only used by the removed function)

The result.json schema's "changes" variant now contains only `comparison`;
the "no-changes" variant is unchanged. No consumer reads result.json yet
(the GH workflow that will is a future PR), so this is a no-op at the
seam and lets that future consumer set its own threshold.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two pre-existing behaviors getting cut, both noted as flawed by PR 2's design:

1. **tagWaiting:** when the baseline build was incomplete at PR-build time,
   the comparator would tag the PR's own build with a marker so a downstream
   process could re-run the comparison once the baseline finished. The
   downstream half (consuming the tag, retriggering the PR build) was
   never written. The tagging was effectively no-op overhead. PR 3's
   GH-Actions workflow approach (re-run flub generate bundleSizeDiff on a
   schedule) replaces this need entirely.

2. **naive fallback:** when no CI build was found for the baseline commit
   (or the build had no usable artifact), the comparator walked prior
   commits via `getPriorCommit` looking for one with a usable build. The
   resulting comparison was incorrect-by-design — main-side deltas
   between merge-base and merge-base − N were attributed to the PR.

Removing both lets ADOSizeComparator's getSizeComparison flatten from a
while-loop with continue/fallback branches into a linear sequence of
early-return validations. The error messages also become more specific —
"No CI build found for baseline commit X" / "Baseline build did not
publish bundle artifacts" rather than the generic "Could not find a
usable baseline build".

Removed:
- bundle-size-tools/src/ADO/getBuildTagForCommit.ts (whole file)
- ADOSizeComparator.tagBuildAsWaiting private method
- ADOSizeComparator.naiveFallbackCommitGenerator static method
- ADOSizeComparator constructor params: adoBuildId, getFallbackCommit
- getSizeComparison's `tagWaiting` parameter
- IADOConstants fields: prBuildDefinitionId, projectRepoGuid (only used
  by the now-gone tagWaiting / comments machinery)
- utilities/gitCommands.ts's getPriorCommit (only used by the fallback)
- All corresponding barrel exports

Updated:
- build-cli's bundleSizeDiff.ts: simplified the ADOSizeComparator
  constructor call (drops the trailing `undefined` and naiveFallbackCommitGenerator
  args) and the getSizeComparison call (no `false` arg).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Webpack 5 supports `resolve.fallback: { assert: false }` natively, which
blocks the Node-core `assert` polyfill from being bundled. This is the
same effect the custom BannedModulesPlugin provided, so drop the plugin
and its supporting code from bundle-size-tools.

Also drops `webpack` from bundle-size-tools' dependencies (it was only
used by the plugin) and `@fluidframework/bundle-size-tools` from
bundle-size-tests' devDependencies (only used here for the plugin).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 6, 2026

Hi! Thank you for opening this PR. Want me to review it?

Based on the diff (398 lines, 15 files), I've queued these reviewers:

  • Correctness — logic errors, race conditions, lifecycle issues
  • Security — vulnerabilities, secret exposure, injection
  • API Compatibility — breaking changes, release tags, type design
  • Performance — algorithmic regressions, memory leaks
  • Testing — coverage gaps, hollow tests

How this works

  • Adjust the reviewer set by ticking/unticking boxes above. Reviewer toggles alone don't trigger anything.

  • Tick Start review below to dispatch the review fleet.

  • After review finishes, tick Start review again to request another run — it auto-resets after each dispatch.

  • This comment updates as new commits land; your reviewer selections are preserved.

  • Start review

@ChumpChief ChumpChief marked this pull request as ready for review May 6, 2026 16:19
Copilot AI review requested due to automatic review settings May 6, 2026 16:19
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 6, 2026

🔭 PR Review Fleet Report

Note

This report is generated by an experimental AI review fleet and is provided as a beta feature. Findings are a starting point for discussion, not a gate. Use your own judgement.

Verdict: ❌ Request Changes

4 Spicy, 3 Pungent, 0 Smelly

Findings

Sev # Area File What Fix
🌶️ Spicy C1 API Compatibility build-tools/packages/bundle-size-tools/src/bannedModulesPlugin/bannedModulesPlugin.ts:1 BannedModulesPlugin, BannedModule, and BannedModulesPluginOptions were all @public exports and have been deleted outright with no deprecation period. Consumers who imported any of these (e.g. examples/utils/bundle-size-tests/webpack.config.cjs did exactly that) will get a hard compile/runtime error after upgrading. Before removing, add @deprecated annotations with migration guidance (e.g. 'Use webpack resolve.fallback or a custom plugin instead') and ship at least one release with the deprecated symbols intact, then remove in a follow-up.
🌶️ Spicy C2 API Compatibility build-tools/packages/bundle-size-tools/src/ADO/getBuildTagForCommit.ts:1 getBuildTagForCommit was a @public export and has been deleted without deprecation. Any consumer who imported it from @fluidframework/bundle-size-tools will get a compile error after upgrading. Deprecate the function first with a @deprecated JSDoc tag and a migration note (or note that there is no replacement), then remove it in a later release.
🌶️ Spicy C3 API Compatibility build-tools/packages/bundle-size-tools/src/utilities/gitCommands.ts:1 getPriorCommit was a @public export and has been deleted without deprecation. Any consumer importing it directly from @fluidframework/bundle-size-tools will get a compile error after upgrading. Deprecate first, then remove in a follow-up release.
🌶️ Spicy C4 API Compatibility build-tools/packages/bundle-size-tools/src/ADO/AdoSizeComparator.ts:240 ADOSizeComparator.naiveFallbackCommitGenerator was a @public static method and has been deleted without deprecation. Consumers who referenced this static directly will break at compile time. Add @deprecated to the static method and keep it for at least one release before removing.
🧄 Pungent H1 API Compatibility build-tools/packages/bundle-size-tools/src/ADO/AdoSizeComparator.ts:62 The ADOSizeComparator constructor had two @public parameters (adoBuildId: number | undefined and getFallbackCommit?: (startingCommit: string) => Generator<string>) that have been removed. Any call site passing more than three positional arguments will fail at runtime or compile time after upgrading. The API report confirms this is a public signature. This is a breaking change. If the package must ship this way, bump the major version or document it clearly as a breaking change in the changelog. Alternatively, consider keeping the parameters with @deprecated annotations before removing them.
🧄 Pungent H2 API Compatibility build-tools/packages/bundle-size-tools/src/ADO/AdoSizeComparator.ts:259 getSizeComparison(tagWaiting: boolean) changed signature to getSizeComparison() — the required tagWaiting boolean parameter was removed. Callers who pass false or true will still compile (extra args are silently dropped in JS), but any callers who relied on tagWaiting: true to trigger the tagging behavior will now silently lose that behavior with no warning. Document the behavioral removal explicitly in the changelog. If any callers pass true, they need to be migrated to an alternative mechanism.
🧄 Pungent H3 API Compatibility build-tools/packages/bundle-size-tools/src/ADO/Constants.ts:13 IADOConstants.prBuildDefinitionId and IADOConstants.projectRepoGuid were @public optional properties on a public interface and have been removed. Any consumer who implemented or extended this interface with those fields will get a TS error (excess-property checks on object literals, or structural mismatch). Any code that reads these properties will also fail to compile. Mark the properties @deprecated first and remove in a follow-up release, or accept this as a documented breaking change requiring a major/minor version bump with clear changelog entry.

View workflow run

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 13 out of 15 changed files in this pull request and generated no comments.

Files not reviewed (2)
  • build-tools/pnpm-lock.yaml: Language not supported
  • pnpm-lock.yaml: Language not supported

@ChumpChief ChumpChief requested a review from TommyBrosman May 6, 2026 16:55
…ehavior-cleanup

# Conflicts:
#	build-tools/packages/bundle-size-tools/src/ADO/AdoSizeComparator.ts
#	build-tools/packages/bundle-size-tools/src/ADO/Constants.ts
Comment thread pnpm-lock.yaml
Comment thread build-tools/packages/bundle-size-tools/src/ADO/AdoSizeComparator.ts
@ChumpChief ChumpChief merged commit e0e0e71 into microsoft:main May 6, 2026
35 checks passed
@ChumpChief ChumpChief deleted the bundle-size-tools-behavior-cleanup branch May 6, 2026 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants