fix(repo): the size gate's externals match what the build actually emits (#709) - #712
Open
rejifald wants to merge 1 commit into
Open
fix(repo): the size gate's externals match what the build actually emits (#709)#712rejifald wants to merge 1 commit into
rejifald wants to merge 1 commit into
Conversation
…its (#709) `check:size` re-bundles each scenario with `external: ['node:*']`. That pattern matches nothing in the package's own artifacts. The source is correct — `src/registry.ts` imports `node:fs`/`node:path`/`node:url` — but tsup strips the prefix, so `lib/` emits `from"fs"`, `from"path"`, `require("fs/promises")`. Across the 49 shipped JS files, zero static import or require specifiers carry a `node:` prefix; the only surviving `node:` strings sit inside `process?.getBuiltinModule?.("node:fs")`, a runtime lookup no bundler resolves. The gate passes today only because its three measured entries reach for no builtin at all, so the inert pattern has never been exercised. Point it at an entry that does and it fails outright: bundling `stitchapi/registry` under the gate's own externals reports `Could not resolve "fs"`, `"path"` and `"url"`. Under the bare specifiers it succeeds at 5593 B. The shadowing risk is real rather than theoretical. With trivial packages named `fs`, `path` and `url` planted in a `node_modules` beside a copy of `lib/`, a `platform: 'neutral'` build with `external: ['node:*']` succeeded and inlined the shadow package's body into the artifact — a measurement of something other than the library. A `node:`-prefixed specifier cannot be shadowed that way. So the list gains the bare specifiers the build actually emits. The prefixed forms stay: they cost nothing and keep this working if the build is ever changed to preserve `node:`, which is the better fix for the shadowing hazard and is left as a separate change with its own blast radius. A builtin the list misses still fails loudly rather than measuring the wrong bytes. No measured number moves — `--json` output is byte-identical before and after, so the `bundle-advertised-size` tether and the READMEs are untouched. Verified end-to-end by adding a temporary `registry` scenario: it measures 5.46 KB where it previously could not build at all. `Refs`, not `Closes`: this is §2 of the issue only. §1 — the gate's missing `splitting: true` — stays open. It is not a one-liner, because `measure()` reads `result.outputFiles[0]` and assumes a single output, so it needs an `outdir` plus chunk summing, and it moves an advertised figure the tether cross-checks against the READMEs and docs. That is a publishing decision, not a bug fix. Refs #709 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The pattern was inert
packages/core/scripts/bundle-size.mjsre-bundled every scenario withexternal: ['node:*']. That pattern matches nothing in the package's own buildartifacts.
The source is correct —
src/registry.ts:7-9importsnode:fs/node:path/node:url— but tsup strips the prefix. Across the 49 shipped JS files inlib/, zero static import or require specifiers carry anode:prefix. Thebuiltins appear bare:
The only surviving
node:strings in the whole oflib/are the 9 occurrencesinside
globalThis.process?.getBuiltinModule?.("node:fs")— a runtime lookup nobundler resolves.
The gate passes today only because its three measured entries need no builtin, so
the inert pattern has never been exercised.
The resolve failure, reproduced
Bundling
stitchapi/registry(which reacheslib/chunk-63GGCWT7.mjs, the filecarrying
from"fs"/from"path"/from"url") with the gate's own settings:Reproduced on this branch against its own freshly built
lib/, twice, the secondtime in an isolated directory to rule out cross-contamination.
The shadowing consequence — and it is the worse one
Verified first-hand, not taken from the issue. A copy of the real
lib/, withtrivial packages named
fs,pathandurlplanted in anode_modulesbesideit, each exporting a marked payload:
Note the failure mode. Without a shadow present the old pattern at least errors.
With one present it does not fail at all — it silently succeeds and reports a
number that includes 910 B of somebody else's package. A budget gate that measures
the wrong bytes and passes is worse than one that breaks. A
node:-prefixedspecifier cannot be shadowed this way.
(The marker has to be a string literal, not an identifier — an identifier is
renamed by
minify: trueand the probe reports a false negative. It did, on thefirst run, before the payload was moved into a literal.)
What changed
One list, plus a comment that now says what it means:
The prefixed forms stay — they cost nothing and keep the gate working if the build
is ever changed to preserve
node:. A builtin the list misses fails loudly(
Could not resolve) rather than silently measuring the wrong bytes.No number moves.
--jsonoutput is byte-identical before and after:stitchapi— whole entryimport { stitch }stitchapi/auth— whole surfaceVerified end-to-end with a temporary
registryscenario added to the real editedscript: it measures 5.46 KB where it previously could not build at all. (Temporary
— not committed.)
Scope
§1 of the issue (the missing
splitting: true) is deliberately NOT in this PR.It is not a one-liner:
measure()readsresult.outputFiles[0]and assumes asingle output, so it needs an
outdirplus chunk summing — and it moves anadvertised figure that the
bundle-advertised-sizetether cross-checks againstboth READMEs, the installation and principles pages, the home-page metrics
component, and the docs' source blurb. That is a publishing decision for the
maintainer, so
Refs, notFixes.Recommended follow-up: change the build so
tsuppreserves thenode:prefixthrough to
lib/. That is the better fix for the shadowing risk — it makes thespecifiers unshadowable at the source rather than externalising the bare names —
but it has its own blast radius (it changes shipped artifacts, not just the gate),
so it belongs in its own PR.
Overlap with #674 — measured, and my hunk is not in it
Open PR #674 (
claude/frosty-goodall-5de390) also edits this file. I rangit merge-treebetween the two branches rather than guess.bundle-size.mjsisthe only conflicted file, with exactly two hunks:
SCENARIOS, andbudget: 21.55 * KB(here, from fix(core): a lone failing cached call no longer kills the process (#670) #676) vsbudget: 21.7 * KB(fix(core): an abort surfaces the caller's reason; a cancel is never a retry #674).measure()merges cleanly — theexternallist this PR changes comes throughintact, with no markers. So neither conflict hunk is this PR's change: both are
#674 against current
main, and they exist whether or not this lands. #674 wascut before #676 moved that budget 21.50 → 21.55, which is why it still reads 21.50
as its base.
Nothing here for me to resolve, and nothing for a merger to trade off against this
PR. #674 needs a rebase and a re-measure against current
mainon its own merits;this PR is orthogonal to it.
Gates
All green locally, plus the full pre-push suite and CI.
CI:
verify,size,drift,coverage,e2e,mcp-e2e,sandbox,search-relevanceall green.driftpassing is the meaningful one here — it is thetether that cross-checks the advertised sizes, and it confirms nothing moved.
CHANGELOG entry went under the existing
### Notes(not### Fixed) — this is repotooling that ships no bytes to consumers, and
Notesalready carries thegate/lint-level entries. No new
###heading.Refs #709
🤖 Generated with Claude Code