Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Bundle Size ReportComparing against baseline from No bundle size changes detected. |
|
Instead of comparing against a committed baseline snapshot, the workflow now builds both the PR branch and the appropriate base branch live in the same job and diffs them directly. When a PR is opened or updated, it checks out the PR at ./pr and inspects core/package.json to determine whether the base should be canary or integrations/makeswift. It then checks out that branch at ./baseline, installs and builds both independently, and generates a bundle size report for each. The two reports are compared and the result is written to the job summary and posted (or updated) as a PR comment. This eliminates the need for a committed bundle-baseline.json and a separate workflow to keep it fresh — the baseline is always the exact current state of the target branch. |
migueloller
left a comment
migueloller
left a comment
There was a problem hiding this comment.
Changes look good, but with something like this I'd like to see some sort of video recording or test or something else that shows the script working.
The PR description points to the PR comment but that hasn't been updated in ~4 days.
.github/scripts/bundle-size.mjs
Outdated
|
|
||
| // eslint-disable-next-line no-underscore-dangle | ||
| const __dirname = dirname(fileURLToPath(import.meta.url)); | ||
| const CORE_DIR = resolve(__dirname, '../../core'); |
There was a problem hiding this comment.
🍹 I think that the point of resolve is to abstract OS-specific separators so we'd want to do resolve(__dirname, '..', '..', 'core') instead?
.github/scripts/bundle-size.mjs
Outdated
| }, | ||
| }); | ||
|
|
||
| const NEXT_DIR = values.dir ? resolve(values.dir) : join(CORE_DIR, '.next'); |
There was a problem hiding this comment.
🍹 Will resolve just throw here is values.dir is an invalid directory? Is that intentional?
There was a problem hiding this comment.
resolve returns an empty string on an invalid path. The code that validates this is path is:
if (!existsSync(appManifestPath)) {
console.error('Error: .next/app-build-manifest.json not found. Run `next build` first.');
process.exit(1);
}
.github/scripts/bundle-size.mjs
Outdated
There was a problem hiding this comment.
🍹 I started reading this file but I'm realizing this is mostly AI-generated and given its determinism, probably the better thing to focus on is tests/that it does what it's supposed to. Do we have a test suite or perhaps some test runs that show that?
There was a problem hiding this comment.
Nope, but I can look into it.
There was a problem hiding this comment.
Added tests, more info below
.github/scripts/bundle-size.mjs
Outdated
| const appManifestPath = join(NEXT_DIR, 'app-build-manifest.json'); | ||
| const buildManifestPath = join(NEXT_DIR, 'build-manifest.json'); |
There was a problem hiding this comment.
🍹 Seeing us look into Next.js internals like these makes me wonder if there's an official tool or interface that supports what we need 🤔
There was a problem hiding this comment.
I looked at how some analyzers did this and it seemed like they were doing the same thing, however Next.js does have some tools to analyze bundles: https://nextjs.org/docs/app/guides/package-bundling
However, it seems overkill for what we're doing, but I can look into it again.
There was a problem hiding this comment.
It seems these tools' --output is static HTML and don't export a simple json that we can use to compare bundle sizes.
There was a problem hiding this comment.
next-bundle-analysis the gold standard is, but I believe the current approach works well for our needs.
|
@migueloller Just to be clear, the PR comment only gets updated on push. Since no changes have been made since 4 days ago, it hasn't changed. I will add some screenshots of how the comment looks if there are changes or above threshold. |
|
Generated some tests for the scripts added and made sure the tests pass succesfully: ❯ pnpm run test:scripts
> @bigcommerce/catalyst@0.1.0 test:scripts /Users/jorge.moya/dev/catalyst
> node --test .github/scripts/__tests__/*.test.mjs
▶ round1
✔ rounds up at .05 (0.35425ms)
✔ rounds down below .05 (0.061209ms)
✔ returns 0 unchanged (0.050125ms)
✔ handles negative values (0.056209ms)
✔ round1 (0.993375ms)
▶ parseManifestEntries
✔ routes /layout entries to layouts (0.438ms)
✔ routes /page entries to pages (0.058417ms)
✔ ignores entries ending in neither /layout nor /page (0.06125ms)
✔ returns empty objects for empty input (0.075959ms)
✔ handles multiple layouts and pages together (0.103542ms)
✔ parseManifestEntries (0.878709ms)
▶ computeRootLayout
✔ selects shortest path as root when multiple layouts exist (0.250458ms)
✔ returns null rootLayoutPath when layoutPaths is empty (0.087125ms)
✔ excludes sharedChunks from rootLayoutChunks (1.913459ms)
✔ rootLayoutChunks contains all non-shared layout chunks (0.313875ms)
✔ computes non-zero sizes when real files exist (0.158291ms)
✔ computeRootLayout (2.827458ms)
▶ computeRouteMetrics
✔ firstLoadJs equals firstLoadJs arg when all chunks are non-existent (0.156416ms)
✔ firstLoadJs is greater than firstLoadJs arg when real chunk files exist (0.28675ms)
✔ excludes sharedChunks from route chunk set (0.277583ms)
✔ excludes rootLayoutChunks from route chunk set (0.288125ms)
✔ includes non-root ancestor layout chunks in route size (0.240625ms)
✔ does not include root ancestor layout chunks in route size (0.090042ms)
✔ applies round1 to all output values (0.0615ms)
✔ computeRouteMetrics (1.49325ms)
▶ compareReport
✔ shows "No route changes detected." when routes are identical (0.275917ms)
✔ does not show global metrics table when global metrics are unchanged (0.050083ms)
✔ shows global metrics table only when metrics changed (0.070542ms)
✔ shows only the changed global metrics (0.049917ms)
✔ shows NEW row for added route (0.066833ms)
✔ shows REMOVED row for deleted route (0.042709ms)
✔ does not show warning for increase under threshold (0.048542ms)
✔ shows warning for increase over threshold (over 1kB AND over threshold percent) (0.051167ms)
✔ does not warn when delta is over threshold percent but 1kB or less (0.045959ms)
✔ does not warn when delta is over 1kB but at or under threshold percent (0.03925ms)
✔ respects custom threshold: no warning when under (0.038125ms)
✔ respects custom threshold: warning when over (0.053167ms)
✔ uses default threshold of 5 percent when not specified (0.042833ms)
✔ formats positive delta with + sign and percent (0.0405ms)
✔ formats negative delta with minus sign and percent (0.062125ms)
✔ sorts routes alphabetically (0.077708ms)
✔ strips the /[locale] prefix from display names (0.038167ms)
✔ omits near-zero deltas that round to 0.0 (0.031375ms)
✔ always shows Per-Route First Load JS section (0.027666ms)
✔ includes threshold value in footer (0.028167ms)
✔ shows header with baseline commitSha and updatedAt (0.044083ms)
✔ handles empty routes in both baseline and current (0.031125ms)
✔ shows table header when routes have changes (0.039292ms)
✔ compareReport (1.545958ms)
▶ getGzipSize
✔ returns 0 when file does not exist (0.089791ms)
✔ returns a positive number for an existing file (0.197666ms)
✔ caches results and returns same value on second call (0.474167ms)
✔ clearSizeCache resets the cache (0.806459ms)
✔ getGzipSize (1.6495ms)
▶ post-bundle-comment
✔ creates a new comment when no existing comment contains the marker (1.394291ms)
✔ updates existing comment when marker found (0.41025ms)
✔ body always starts with marker and newline (0.330083ms)
✔ updated comment body also starts with marker and newline (0.260583ms)
✔ includes report file content in the comment body (0.384292ms)
✔ reads report from a custom reportPath (0.896875ms)
✔ passes correct owner, repo, issue_number from context to listComments (0.540875ms)
✔ passes correct owner, repo, issue_number from context to createComment (0.469708ms)
✔ passes correct owner and repo to updateComment (0.892167ms)
✔ uses the first comment that contains the marker (not just exact match) (0.507083ms)
✔ creates comment when existing comments do not contain the marker (0.353417ms)
✔ post-bundle-comment (7.124083ms)
ℹ tests 59
ℹ suites 7
ℹ pass 59
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 560.026416 |
migueloller
left a comment
migueloller
left a comment
There was a problem hiding this comment.
Thanks for the tests and screenshots @jorgemoya
Two minor notes 🍹
- Why not use TypeScript and get the benefit of type checking? Node.js can strip types natively.
- The report when no changes happen is a bit confusing since it shows the callout with the threshold. Maybe we can simplify the comment when there are no changes?
|
@migueloller Updated the reporting to be less confusing (see PR description) + migrated to TS ( |
5 participants
What/Why?
This feature adds automated bundle size tracking to the CI pipeline. Every time a PR is opened or updated, the build is compiled and its output is compared against a stored baseline snapshot, with the size differences posted as a PR comment. To keep that baseline from going stale, a separate workflow automatically regenerates and commits it whenever code is merged into canary or integrations/makeswift.
Testing
Bundle reporting tested below. Testing the workflow to keep baseline is sync is TBD.
No changes:
Global metrics changed, routes identical:
Route changes (with warning):
Migration
N/A