Repository navigation
fix(ci): PR size report measures ESM entries and fills Before and Delta from the merge base - #8277
Conversation
… Delta from the merge base analyze-pr.js resolves dist entries by module type: Babel writes ESM as .js in "type": "module" packages, so ESM was always N/A and the ESM file was labeled CJS. A new size-base job builds the PR's merge base beside build-storybook and runs analyze-pr.js --base-root --base-only; the PR comment matches its head and renders Before and Delta for modified components and package gzip size.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for restoring the size report: resolving ESM entries by module type and measuring the merge base fill the columns that always read N/A, but three issues need fixing before this lands.
- An unmeasured size is coerced to
0(Number(null)), so every modified Charts, Rich Text or Vega component renders| Entry File (ESM) | N/A | N/A | 0B |; subtract only when both sides are numbers, and add a test withnullbytes. size-basefails, and marks the CI run failed, once main is more than about 250 commits past the branch point (a 50-commit fetch plus one deepen of 200); skip the measurement when no merge base is found, the wayanalyze-pr.jsalready degrades.- The package table's Gzipped before, after and Delta columns measure only the
dist/index.jsre-export barrel, so implementation-only changes read0B; label them as entry-file figures or add a caveat until the import-size follow-up.
[Automated review]
| const delta = | ||
| Number.isFinite(Number(b.gzipBytes)) && | ||
| Number.isFinite(Number(base.gzipBytes)) | ||
| ? Number(b.gzipBytes) - Number(base.gzipBytes) | ||
| : null; |
There was a problem hiding this comment.
Number(null) is 0, so this prints a delta even when one side was never measured. The component Delta above (lines 137-141) uses the same test, and now that base stats arrive, every modified Charts, Rich Text or Vega component (no per-component entry, so both byte counts are null) renders | Entry File (ESM) | N/A | N/A | 0B |. Suggest subtracting only when both values are numbers, e.g. typeof b.gzipBytes === 'number' && typeof base.gzipBytes === 'number', plus a test case with null bytes.
| if ! BASE_SHA=$(git merge-base "origin/$BASE_REF" HEAD); then | ||
| git fetch --deepen=200 origin "$BASE_REF:refs/remotes/origin/$BASE_REF" | ||
| BASE_SHA=$(git merge-base "origin/$BASE_REF" HEAD) |
There was a problem hiding this comment.
When main is more than about 250 commits past the branch point (the 50-commit fetch plus one --deepen=200), the second git merge-base fails and, under bash -e, fails the job and the CI run. analyze-pr.js degrades instead of failing in the same situation; consider skipping the measurement (exit 0, no artifact) when no merge base is found, or continue-on-error: true on the job.
| const signedBytes = delta => | ||
| delta === null ? 'N/A' : delta > 0 ? `+${delta}B` : `${delta}B`; | ||
| if (bundlePackages.length > 0 && Array.isArray(baseBundles)) { | ||
| bundleSection += `| Package | Entry (ESM) | Gzipped before | Gzipped after | Delta |\n`; |
There was a problem hiding this comment.
For Core, Lab, Charts and Rich Text these gzip figures are the package's dist/index.js re-export barrel, not the package, so an implementation-only change always shows 0B here. Consider labelling them as the entry file's (e.g. Entry gzipped before) or adding a one-line caveat until the import-size follow-up.
…out a merge base; label entry-file gzip
There was a problem hiding this comment.
Changes requested for one missing test: the size report now fills its ESM, Before and Delta cells, the last two from a merge-base build, and the three issues raised on the earlier revision are fixed. The new size-base job's lane guard (.github/workflows/ci.yml:1082) is correct, but no workflow test asserts it, so removing it would leave every CI workflow test green, while the verification for AST-030 FR8 requires a test that fails when build work runs for the Node-tooling lane. Adding expect(ci.jobs['size-base'].if).toContain(BROAD_ONLY); beside the other heavy-job assertions in .github/scripts/ci-tooling-routing.test.mjs would close it.
[Automated review]
| # still come from build-storybook's analysis. | ||
| size-base: | ||
| needs: [check-scope] | ||
| if: github.event_name == 'pull_request' && needs.check-scope.result == 'success' && needs.check-scope.outputs.docsite_only != 'true' && needs.check-scope.outputs.spec_only != 'true' && needs.check-scope.outputs.tooling_only != 'true' |
There was a problem hiding this comment.
This guard keeps the merge-base install, Core build and component analysis out of the docsite, spec and Node-tooling lanes, as AST-030 FR8 requires for tooling, but no workflow test asserts it: removing it leaves every test that parses ci.yml green. Consider adding expect(ci.jobs['size-base'].if).toContain(BROAD_ONLY); beside the build-sandbox assertion in .github/scripts/ci-tooling-routing.test.mjs.
… have a budget (#8271) ## Exact decision needed How much a component change may add to what every product downloads, and how a pull request shows that cost. ## Existing authority searched - `spec:AST-080` FR1, FR2, DEC-3: each package ships one whole stylesheet, so CSS any component adds reaches every product. - `architecture:component-style-authoring` INV1–INV5: no size outcome. - `architecture:react-component-runtime` INV5 (acquire optional resources only while enabled) and INV8 (reuse shared owners): no byte budget. - `spec:AST-036` FR22–FR23: the only numeric budgets, for the Markdown parser only. - No current record requires a size report or sets a per-component JavaScript or CSS budget. - Search terms: size, bundle, budget, gzip, stylesheet, CSS delta, lazy, value table, constants table, touch-only. Overlapping open pull requests: #8277 (the size-report fix this record governs), #8052 (touch presentation spec citing a shared value table), #8053 (the touch constants table, which the value-table rule covers), #7358 (per-component English catalog slices; catalogs stay with `architecture:internationalization` INV15), #8286 (a component-local tokenizer byte budget, unaffected). ## Decision - `spec:AST-080` FR8 / DEC-4: every pull request that changes styled modules reports, base against head, each changed package stylesheet's gzip size and each changed component's gzip CSS delta. A feature that runs only on a coarse pointer and adds more than 1 KB gzip of CSS to one component is flagged and justified; the CSS stays in the package stylesheet. - `architecture:component-style-authoring` INV6: every pull request that changes a component reports the gzip JavaScript its import pulls in, transitive modules included, and its CSS delta, base against head. Touch-only work over 2 KB gzip JavaScript or 1 KB gzip CSS per component is flagged; the JavaScript is loaded lazily, only when a coarse pointer needs it, or the cost is justified. A shared JavaScript tuning table ships to a component only the rows that component uses; StyleX variable groups and message catalogs keep their own owners. ## Rejected alternatives - A separate stylesheet for touch-only CSS: breaks the one-import-per-package setup of `spec:AST-080` DEC-3. ## Claim scope and non-goals Size reporting and the touch-only budget only. Budgets for other kinds of work, render or listener counts, and the report's CI implementation are outside this PR. ## Evidence and implementation relationship The pull-request size report measures no stylesheet, reports a component's re-export file instead of what its import pulls in, and never fills its before and delta columns. A report fix and the budget flags follow as separate implementation PRs. No implementation is included. ## Writing rubric results - R1 narration: none. - R2 research in the record: none. - R3 evidence naming a change: none; verification names report states. - R4 rejected alternatives over one line or outside a decision: one, inside DEC-4, one line. - R5 pull request or issue references remaining: none. - R6 ruled open questions still listing options: none. ## Scope - [x] This PR contains specification records only. - [x] It records observable behavior and preserves equivalent internal implementations. - [x] Any named internal mechanism is intentionally public, names its dependent caller/system, and explains why an equivalent implementation would not satisfy the contract. - [x] Verification states the evidence layer and failure signal without prescribing CI job/workflow topology. - [x] It records one intentional durable decision, not a rescue for a separable tagalong. - [x] The canonical knowledge owner, affected current claims, and overlapping open work are named; any ownership collision is resolved or called out for human decision. - [x] Adjacent API, visual, accessibility, composition, and implementation facts remain outside scope unless this exact decision depends on them. - [x] Public text and artifacts contain no internal Meta context. ## Validation - `pnpm check:knowledge` (CI lint) - Prettier - `git diff --check`
There was a problem hiding this comment.
No remaining issues: the size report now measures each package's real ESM entry and fills Before and Delta from a merge-base build, and the points raised on earlier revisions (unmeasured sizes, a missing merge base, the entry-file labels and the size-base lane assertion) are all addressed. Nothing else is needed for this change; the per-component figure still measures the re-export entry file, so implementation-only growth reads 0B until the import-size follow-up you describe.
[Automated review]
User impact
Reviewers and authors read the PR comment's size report to see what a change costs the people who download Astryx. Today it shows no real numbers: "Bundle Size (ESM)" is always N/A, the ESM file is reported under a "CJS" label, and Before and Delta are always N/A. So size growth, including touch-only code that ships to every desktop product, lands unseen.
Problem and solution fit
dist/<Component>/index.mjs, but Babel writes ES modules with a.jsextension in"type": "module"packages. Entries are now resolved by the package's module type:index.jsis ESM there;index.mjs/index.cjskeep their meaning; a non-module package (Vega) keepsindex.mjsas ESM andindex.jsas CommonJS.baseComponentStats, which nothing wrote, and CI built only the PR head. A newsize-basejob checks out the merge base, builds Core (plus any other component package the PR changes), and runsanalyze-pr.js --base-root .size-base --base-only. It runs besidebuild-storybook, so no browser lane waits on the second build.pr-size-baseartifact, uses it only when it names the exact head being reported, and renders Before and Delta for modified components and package gzip size.Expected behavior and authority
The report's own columns (Before, After, Delta, ESM) promise these figures. The pending size-budget record in #8271 requires a base-against-head report on every PR; the per-import transitive size, the CSS delta, and the budget flags follow in a separate PR once it merges.
Smallest restoration
Only the entry resolution, the base measurement, and its rendering. The per-component figure is still the component's entry file, now labeled "Entry File"; measuring what an import pulls in is the follow-up.
Evidence
"type": "module"entry as ESM (CJS null) and reads base sizes from a merge-base checkout; the comment test renders| Entry File (ESM) | 100B | 160B | +60B |and a package gzip delta.analyze-pr.test.mjs(base and head sizes),generate-pr-comment.test.mjs(base sizes),pr-preview.test.mjs(base stats used only for the exact head).Scope
Testing
pnpm vitest run .github/scripts/analyze-pr.test.mjs .github/scripts/generate-pr-comment.test.mjs .github/scripts/lib/pr-preview.test.mjs .github/scripts/ci-*.test.mjs .github/scripts/release-check-workflow.test.mjsactionlinton the changed workflows (no new findings)