fix(transforms)!: run the browser server-exports strip before compile - #3855
fix(transforms)!: run the browser server-exports strip before compile#3855kojiwakayama wants to merge 22 commits into
Conversation
…pile Moves browserServerExportsStripPlugin ahead of compilePlugin in BROWSER_PIPELINE and changes its stage from COMPILE + 0.6 to PARSE + 0.5. Both had to change: array position decides order until a custom plugin registers, at which point the pipeline is re-sorted by stage. The strip stage recognised esbuild `__name(fn, "fn")` registrations that Veryfront's own compile stage injected one position earlier, with keepNames enabled. Running the strip first means no keepNames helper is ever emitted for a declaration the pass removes. Deletes the compile/strip sourcemap stash-and-restore handshake. It existed only because compile ran first and embedded pre-strip source in its development map. With the strip first, the compiler builds its map from already-stripped input, so there is nothing to defend. Refs veryfront/veryfront-issue-inbox#112
Babel attaches a file's opening comments to its first statement, so removing that statement takes them with it. Running the server-exports strip on authored source rather than on compiled output makes this observable: a `@jsxImportSource` pragma above a hook-only import was dropped and the page silently rendered through the configured default JSX factory instead. Legal banners and lint suppressions went the same way. Relocates the leading comments of a removed statement onto the next surviving one. Refs veryfront/veryfront-issue-inbox#112
|
Warning Review limit reached
Next review available in: 53 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
|
@coderabbitai review Draft on purpose, but the review is wanted. This is a pipeline ordering change for the browser target plus the deletion of the compile/strip sourcemap handshake it made unnecessary. Please pay particular attention to The known open issue on this branch is stated in the description and does not need re-reporting: an unused import that the stripped hooks do not own is still reduced to a bare side-effect import, which post-reorder puts a server module into the browser graph. The fix is a contract change posted as a yes/no on veryfront/veryfront-issue-inbox#112. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Changed files ==='
git diff --name-status HEAD^ HEAD
echo '=== Diff: pipeline and browser strip stage ==='
git diff --find-renames --unified=80 HEAD^ HEAD -- \
src/transforms/pipeline/index.ts \
src/transforms/pipeline/stages/browser-server-exports-strip.ts
echo '=== Outline: pipeline index ==='
ast-grep outline src/transforms/pipeline/index.ts --items all --view expanded || true
echo '=== Outline: browser strip stage ==='
ast-grep outline src/transforms/pipeline/stages/browser-server-exports-strip.ts --items all --view expanded || true
echo '=== Focused tests and references ==='
rg -n -C 4 \
'retainLeadingComments|BROWSER_PIPELINE|browserServerExportsStripPlugin|runs before compile|compile map|jsx pragma|source[Mm]ap' \
src tests 2>/dev/null || trueLength of output: 164198 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Pipeline declaration and custom-plugin sorting ==='
sed -n '58,88p;320,380p' src/transforms/pipeline/index.ts
echo '=== Transform stage values ==='
sed -n '1,130p' src/transforms/pipeline/types.ts
echo '=== Complete PR diff for the focused test ==='
git diff --find-renames --unified=30 HEAD^ HEAD -- \
src/transforms/pipeline/stages/browser-server-exports-strip.test.ts
echo '=== Ordering and source-map test implementations ==='
sed -n '1020,1175p' src/transforms/pipeline/stages/browser-server-exports-strip.test.ts
echo '=== Comment-relocation test implementation and nearby cases ==='
sed -n '1215,1285p' src/transforms/pipeline/stages/browser-server-exports-strip.test.ts
echo '=== Comment mutation sites and parser/generator contracts ==='
rg -n -C 5 \
'leadingComments|trailingComments|interface CodeParser|generate\(' \
src/transforms/pipeline/stages/browser-server-exports-strip.ts \
src/extensions/parser/code-parser.ts \
src/extensions/parserLength of output: 30676
The pipeline order and stage value agree.
|
|
Decision on the open leak is yes: Short version: the demotion is an artifact of the strip running last, like the sourcemap handshake this PR already deletes. With Implementing now. Two conditions: genuine side-effect imports ( |
`retainLeadingComments` relocated a removed statement's comments onto the next surviving statement, and dropped them when no surviving statement followed. A legal banner between two removed trailing declarations was lost. Attach the remainder to the last surviving statement instead. Reported by CodeRabbit on #3855. Refs veryfront/veryfront-issue-inbox#112
|
@coderabbitai Confirmed and fixed in Your repro as written does not lose the banner, because Babel attaches a comment between two statements to both of them: it is import { getEnv } from "veryfront";
export async function getServerData() { return { props: { s: SECRET, r: RAW } }; }
const RAW = getEnv("SECRET_VALUE");
/*! package license */
const SECRET = RAW + "x";Both tail declarations are pruned, the only surviving statement precedes the banner, and The fix tracks the last surviving statement and appends the remainder to its On your second point, agreed, and that agreement is now enforced rather than asserted: |
|
Noting for coordination, since the comment above says the import-demotion change is being implemented now: this session is not implementing it, so there is no duplicate work in flight from here. This branch stops at the ordering change, the handshake deletion, and the two comment-preservation fixes, and it still carries the leak described in the description. Two things that will help whoever lands the demotion change on top:
This PR stays a draft until that change lands and the probe goes clean. |
|
Tip For best results, initiate chat on the files or code changes.
The new test covers the defect. The two ordering tests also independently protect the array position and 🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
`dropUnusedImportBindings` reduced an unused import it does not own to a bare side-effect import, described as compatibility with the older conservative behavior. That demotion is an artifact of the strip running last: with nothing downstream to clean up, a bare import conservatively preserved the module's side effects. Now that compile runs after the strip, the demotion converts an ERASABLE named import into a NON-ERASABLE one. Side-effect imports are preserved by design, so it defeats the cleanup that would otherwise drop the module, and a server-only module reachable only through an unused import ships to the browser. `main` is clean because the compiler erases the import before the pass ever sees it. The pass now removes only the specifiers the stripped hooks owned and leaves everything else exactly as authored. A genuine side-effect import is still untouched, and a hook-owned import is still deleted outright. The correctness of this depends on the pipeline ordering, so the call site carries an explicit note: compile must keep running after this stage, or the demotion has to come back with the ordering. Refs veryfront/veryfront-issue-inbox#112
|
The decision at veryfront/veryfront-issue-inbox#112 (comment 5332914896) is now implemented in What changed in
Measured through the real reordered pipeline for the blocking case in the description:
Two things worth your attention specifically:
@coderabbitai review Still a draft on purpose. Do not mark it ready. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR commits and changed files since the prior reviewed commit ==='
git log --oneline --decorate -12
git diff --name-status 30c64eeb..HEAD
echo '=== Focused diff ==='
git diff --find-renames --unified=55 30c64eeb..HEAD -- \
src/transforms/pipeline/stages/browser-server-exports-strip.ts \
src/transforms/pipeline/stages/browser-server-exports-strip.test.ts \
src/transforms/pipeline/index.ts
echo '=== Source outlines ==='
ast-grep outline src/transforms/pipeline/stages/browser-server-exports-strip.ts --items all --view expanded || true
ast-grep outline src/transforms/pipeline/stages/browser-server-exports-strip.test.ts --items all --view expanded || true
ast-grep outline src/transforms/pipeline/index.ts --items all --view expanded || true
echo '=== Import-removal implementation and ordering declarations ==='
rg -n -C 12 \
'dropUnusedImportBindings|dropOwnedSpecifiers|PIPELINE ORDERING DEPENDENCY|stripServerOnlyExports|browserServerExportsStripPlugin|BROWSER_PIPELINE|stage:' \
src/transforms/pipeline/stages/browser-server-exports-strip.ts \
src/transforms/pipeline/index.ts
echo '=== Targeted test references and side-effect import coverage ==='
rg -n -C 10 \
'`@audit`|declare id|enum member|same-scope enum|block-scoped enum|side-effect import|side effect import|server-only-lib|dropOwnedSpecifiers|dropUnusedImportBindings' \
src/transforms/pipeline/stages/browser-server-exports-strip.test.tsLength of output: 127099 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Direct stripServerOnlyExports call sites ==='
rg -n -C 8 '\bstripServerOnlyExports\s*\(' src
echo '=== Compile stage configuration ==='
ast-grep outline src/transforms/pipeline/stages --items all --type function,variable --match 'compilePlugin|compile' --view expanded || true
rg -n -C 18 'compilePlugin|treeShaking|treeShaking:|minify|esbuild.*transform|transform\(' \
src/transforms/pipeline/stages \
src/transforms/pipeline
echo '=== Splitter integration ==='
fd -a 'esbuild-plugin.ts' src/build/bundler/code-splitter -x sh -c \
'echo "=== $1 ==="; ast-grep outline "$1" --items all --view expanded || true; rg -n -C 16 "\bstripServerOnlyExports\s*\(|onLoad|esbuild|treeShaking" "$1"' sh {}
echo '=== All changed test names and assertions related to import output ==='
git diff --unified=4 30c64eeb..HEAD -- src/transforms/pipeline/stages/browser-server-exports-strip.test.ts \
| rg -n -C 5 '^[+-].*(it\(|assertStringIncludes|assertNotIncludes|assertEquals|import )' || true
echo '=== Existing tests that protect authored side-effect imports ==='
rg -n -C 8 'import "\./|import "\.\./|side-effect import|side effect import|polyfill|analytics' \
src/transforms/pipeline/stages/browser-server-exports-strip.test.tsLength of output: 201355 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== runPipeline options and dev propagation ==='
sed -n '189,380p' src/transforms/pipeline/index.ts
sed -n '1,180p' src/transforms/pipeline/context.ts
echo '=== Focused test calls and any development-mode pipeline coverage ==='
sed -n '1048,1165p;1235,1305p' src/transforms/pipeline/stages/browser-server-exports-strip.test.ts
rg -n -C 6 \
'dev:\s*true|dev:\s*false|pre-compile-unused-import-demotion|server-only-lib' \
src/transforms/pipeline/stages/browser-server-exports-strip.test.ts \
src/transforms/pipeline/index.ts \
src/transforms/pipeline/context.ts
echo '=== Compile tree-shaking configuration ==='
sed -n '66,88p' src/transforms/pipeline/stages/compile.tsLength of output: 35042
The ordering note itself is in the correct location. It is immediately before the dependent call, and the The re-anchored paired tests still discriminate their intended cases. The decorated-property pair distinguishes a runtime decorator read from an erased declared property. The enum pair distinguishes block scope from same-scope shadowing. The stage-level and end-to-end side-effect-import tests also preserve coverage for authored bare side-effect imports. 🧠 Learnings usedYou are interacting with an AI system. |
|
Blocker cleared. Verified independently at The probe that was leaking now comes back clean: Four import shapes checked end to end, since the change is to which specifiers survive:
Restoring Full repo suite at this head: 4508 passed / 106 failed, against 4503 / 111 on Merge-group pre-flight at this head: Leaving the draft flag alone rather than flipping it, since the branch has more than one author tonight and marking it ready makes it queueable. |
The previous fix left an unused specifier the stripped hooks did not own
in place, on the reasoning that compile now runs after the strip and
esbuild erases a genuinely unused import. That reasoning holds only for
the `ts` and `tsx` loaders, where a specifier may name a type and
elision is permitted. `getLoaderFromPath` maps `.js` to `js`, `.jsx` to
`jsx`, and `.md` and `.mdx` to `jsx`, and under those loaders esbuild
must preserve the module for its side effects, so it rewrites the
remainder into exactly the bare side-effect import this stage forbids.
The stage emits byte-identical output for `probe.tsx`, `probe.jsx` and
`probe.js`, so the loader alone decided the outcome and the leak was
invisible on a `.tsx` fixture.
`dropUnusedImportBindings` now deletes the whole statement whenever the
stripped hooks owned at least one specifier and no surviving specifier
is read, which is what the pass did when it ran after compile. An import
with a surviving reader still keeps every specifier it was authored
with, and an import the hooks own nothing of is still left exactly as
authored for compile to erase.
`import srv = require("./lib.js")` and `import srv = ns.member` leaked
the same way for a different reason: pre-compile the pass meets a
TypeScript node shape esbuild used to lower before it ever ran, and
neither the module-scope pruner nor the import pass recognised it. The
alias survived as `const srv = require("./lib.js")`, which keeps the
server-only module in the browser graph and throws a ReferenceError on
`require`. `moduleScopeDeclarations` now treats a value-emitting
`TSImportEqualsDeclaration` as the module-scope binding it is, so a
hook-only one is pruned and the closure grows through its right-hand
side to the namespace import behind it.
Tests run the real browser pipeline over the dialect axis (`ts`, `tsx`,
`js`, `jsx`, `mdx`) crossed with the import shapes (mixed specifiers, a
default-import sibling, both import-equals forms) in dev and prod. Every
cell now matches `main`, and the JavaScript-dialect cells fail without
the fix while the TypeScript ones pass either way, which is the gap the
single `.tsx` guard left open.
Refs veryfront/veryfront-issue-inbox#112
`moduleScopeDeclarations` now sees `TSImportEqualsDeclaration`, and Babel
represents `export import A = require("./a.js")` as that same node with
`isExport: true` rather than wrapping it in an export declaration. Every
other candidate reaches the collector already unwrapped, so the existing
"exported declarations are never candidates" rule needed the flag spelled
out or a hook-only exported alias would be pruned out of the module's
contract.
Refs veryfront/veryfront-issue-inbox#112
|
Coordination note, because two sessions were on this at once. I rewrote the PR description at a point where the head was Verified against the current head
One shape is still open and is unchanged by any of this, on Full-suite numbers in the description are honest about what did not run. This machine is pinned by twelve orphaned CPU-burner processes from an unrelated session (load average 13 for over eleven hours), so every server-backed test in |
`compilePlugin` sets `treeShaking: !ctx.dev`, so the existing end-to-end probe for the closed leak only covered the tree-shaken path. What erases an unrelated unused import is not tree shaking, it is TypeScript unused-import elision, which the `ts` and `tsx` loaders perform in both modes. Measured across `ts`, `tsx`, `js` and `jsx` crossed with dev and prod, this branch matches `main` in every cell. The development probe is added anyway, so a future change to either setting cannot quietly make development ship the module, and the ordering note now names the mechanism instead of leaving "esbuild erases it" open to being read as tree shaking. Refs veryfront/veryfront-issue-inbox#112
|
@coderabbitai The development-mode finding does not reproduce. Measured, not reasoned about.
Every cell matches The Your suggestion to pin the development path is a good one and is now in Two of your other conclusions match what I found independently: the ordering note is at the dependent call with the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Current branch and relevant commits ==='
git log --oneline -10
echo '=== Focused changes after d6e744c6 ==='
git diff --find-renames --unified=35 d6e744c6..HEAD -- \
src/transforms/pipeline/stages/browser-server-exports-strip.ts \
src/transforms/pipeline/stages/browser-server-exports-strip.test.ts
echo '=== Ordering dependency note and dependent call ==='
rg -n -C 18 \
'PIPELINE ORDERING DEPENDENCY|treeShaking|elision|stripServerOnlyExports|dropUnusedImportBindings' \
src/transforms/pipeline/stages/browser-server-exports-strip.ts \
src/transforms/pipeline/stages/compile.ts
echo '=== Development-mode regression test and its runPipeline call ==='
rg -n -C 22 \
'does not demote an unrelated unused import in development either|server-only-lib|dev:\s*true' \
src/transforms/pipeline/stages/browser-server-exports-strip.test.tsLength of output: 57573
I verified The ordering note now identifies TypeScript unused-import elision as the removal mechanism. It also correctly states that The TypeScript/TSX limitation is explicit. The remaining JavaScript/JSX behavior matches ✏️ Learnings added
🧠 Learnings usedYou are interacting with an AI system. |
Running the browser server-exports strip before compile hands it JSX the
walkers never used to see. Post-compile every JSX name had already been
lowered to a factory call, so both reference walkers counted every
JSXIdentifier as a runtime read. Three of those positions are not reads:
- an intrinsic element name (`<table>`, `<section>`, `<my-widget>`), which
is string text to the factory
- an attribute name (`placeholder=`, `title=`, `href=`), which is a
property key
- the property of a member element name (the `Item` in `<UI.Item />`),
where only the leftmost object is read
Both halves of a namespaced name (`<svg:circle xlink:href="x" />`) are
string text too, matching what esbuild emits for them.
The misclassification leaked and deleted in both directions. An import
the hooks owned survived into the browser artifact when its binding
shared a tag or attribute name, and an unrelated module-scope
declaration that shared one was pulled into the hook closure and deleted
along with the import it was the last user of.
A classic `@jsxRuntime classic` module is a second pre-compile surface:
esbuild writes the `@jsx` and `@jsxFrag` factory calls in the stage that
now runs after this one, so nothing read the factory yet and its import
was dropped while still called. Pin the pragma targets, gated on the
classic runtime because esbuild ignores the pragmas otherwise, and pin
esbuild's default `React` factory when a classic module names none.
Also document the re-export build break the reorder introduces:
`export { getServerData } from "./loaders.ts"` binds no local
declaration, so the build now stops instead of shipping the loader.
The previous commit added a tracked `node_modules` symlink pointing at its own absolute path. Any checkout of this branch then failed every Deno command with `Too many levels of symbolic links (os error 62)`, including `deno test` and `deno task typecheck`. The ignore rule reads `node_modules/`, and a trailing slash matches directories only, so a symlink of that name is not ignored and a `git add -A` picks it up. Add the slash-less form beside it.
|
Owning a mistake of mine, since another session had to clean it up.
I also misread the first CI failure on that commit as runner flake. Re-verified on |
Both reference walkers in the server-exports strip fell back to a generic
descent for any node type nobody had classified, and counted the identifiers
under it as runtime reads. Four review rounds on the strip-before-compile
reorder each found one instance of that. Two were still open.
The object of a JSX member element name was run through the intrinsic-tag
rule, so `<motion.div>`, `<styled.div>` and `<dialog.Root>` classified their
object as string text and the pass deleted the binding they read. An export
clause with a source reads no local binding, but neither walker had a case for
it, so `export { token } from "./client-utils.js"` counted `token` as a free
read and kept a hook-owned `import { token } from "./server-only.js"` alive.
Replace the ad hoc handling with one classification, derived from the pinned
@babel/types and shared by both walkers: every node type is a read, an erased
node or a structural one, and every child position that holds a fixed name is
recorded once. A guard test fails when the package defines a node type the
classification does not name, so a Babel upgrade breaks the build instead of
leaking a server-only module into the browser artifact.
An unrecognised node defaults to structural, which over-retains rather than
over-deletes. An unrecognised TypeScript node stays erased, because the
TypeScript grammar inverts that argument.
…d case Mutation testing the shared reference classification found nine cells whose revert no test caught: the fixed-name key of an object method, a class method, a class property and an auto-accessor, the computed form of each of those, the comments and tokens a parsed file carries beside its program, every import specifier shape, the exported name of a default or namespace re-export, and the identifier-valued name of a placeholder. None of them was wrong in the implementation. Each was a cell the table classified and nothing asserted, so a future edit could flip it silently, which is the failure mode this classification exists to end. Add a case per cell. The method and class member keys are asserted through both walkers, since a fixed key counted as a read pins the import it collides with. The rest are asserted on `referenceChildren`, because they hold no identifier a walker could report. Three map entries stay unkillable by construction: `V8IntrinsicIdentifier.name`, `DirectiveLiteral.value` and `InterpreterDirective.value` are string fields in `NODE_FIELDS`, so `referenceChildren` never collects them whether the table names the key or not.
|
Triage note (2026-08-19): this PR is the timeboxed spike that #3846 is waiting on. #3846's description parks the second half of #3825 — the intrinsic-tampering analysis, which is undecidable, has 21 known module shapes that defeat it, and carries six unresolved threads including a P1 build-time CPU blowup — pending:
That is exactly what this PR does. It is Getting a yes/no out of it is disproportionately valuable: a "yes" retires that entire undecidable analysis permanently instead of leaving it parked. Suggest taking it out of draft and reviewing it on its own merits, independently of #3846. Note this PR's description references |
The node-type guard pins the classification TABLE. It cannot pin the
PREDICATES that approximate compiler rules, and one of them was wrong:
`isIntrinsicJsxName` tested for an identifier with an ASCII-only regular
expression, while esbuild's rule is "first character is ASCII a-z, or the
name is not a valid ECMAScript identifier" over the full Unicode grammar.
Any JSX element name that is a valid identifier and contains a non-ASCII
character was classified as intrinsic tag text, both walkers skipped
`JSXOpeningElement.name`, and the import or declaration it named was
deleted while the artifact still called it. Measured through the real
`runPipeline`: `<Café />` compiles to `jsx(Café, {})` with its import
gone, so the page dies on a ReferenceError.
`pragmaRootBinding` in the strip carried the identical regular
expression, so a `@jsxRuntime classic` module whose pragma named a
non-ASCII factory root lost the factory import the emitted call needs.
Both now share one Unicode-aware identifier test.
The fix alone would leave the class open, so every predicate in this
stage that stands in for a compiler or parser rule now has a
differential test: it compares the predicate against the real compiler on
a corpus of inputs instead of against a second hand-written expectation.
`compiler-predicates.test.ts` covers the JSX tag-text test, the classic
pragma pins, the `declare` short-circuit and its decorator exception, the
`importKind`/`exportKind` short-circuits, the whole TypeScript erasure
split, esbuild's `keepNames` helper shape, and the sourcemap comment. Its
header records the three rules that are deliberately not there and why.
The guard is hardened at the same time. It pinned `@babel/types` but not
`@babel/parser`, which the same manifest pins separately, so a
parser-only bump would leave the guard green while a new TS-prefixed node
type fell to the erased default. It also filtered the Flow family out of
its scope on an unstated assumption; that assumption is now an assertion
over every path the parser extension takes.
The code-point sweep asserted that `isEcmaScriptIdentifier` agrees with the
host runtime's own lexer. That holds on V8 and fails on JavaScriptCore,
which CI found: `\p{ID_Start}` and `\p{ID_Continue}` are the engine's
regular-expression tables, the lexer is a separate table, and the two are
versioned independently inside the same runtime. Bun named about twenty
code points where they differ, and a local Bun of another version names
three entirely different ones.
Runtime agreement was never the property that matters. The predicate is
asked about names the COMPILER will meet, so the compiler decides whether
a disagreement can reach an artifact. The sweep now collects the
disagreements and asserts esbuild refuses every one of them: the module
does not build, so no artifact can depend on which table was right. Where
esbuild would accept such a name the test fails, because then the two
answers differ on a name that ships and one of them deletes a live
import.
Verified in both directions: the code points CI's Bun named and the three
this machine's Bun names are all rejected by esbuild.
Handover: six review rounds, six defect classes, and why I am stopping hereThis PR should not leave draft on an agent's judgement. Recording the full state so a human can decide. What the reorder achieves
Why it is still a draftEach review round found a real defect class the previous round's own tests passed through:
The open blocker: export-name normalizationThe pass matches a hook by AUTHORED SPELLING, in two places:
ECMAScript gives one export several spellings that all produce the same runtime property: an IdentifierName with unicode escapes, a StringLiteral export name, an escaped StringLiteral. esbuild normalises them. This pass does not, so five authored spellings are never recognised as hooks at all. The hook body, the secret it closes over, its It also fails OPEN, which nothing else in this pass does. Why round 6 did not catch it
The suggested closing test, which would also have caught this: compile the module, read the export names off the artifact, and assert that the set of hook-named exports the pass acted on equals the set the artifact actually exports. Also open
RecommendationThe architecture is right and the evidence for it is strong. But six rounds have each found a class the prior round's tests passed through, the newest is a silent leak in the direction that matters most, and the machinery built to prevent exactly that structurally excused the predicate that broke. The remaining work is bounded and known, but it needs an owner who can make the semantic merge call and sign off on a user-facing build break. Not marking ready. Refs veryfront/veryfront-issue-inbox#112. |
Server-only hook export names can be authored as string literals with escapes, so raw source substring checks are not a safe guard for deciding whether to parse or strip a module. The transform now uses an export-shaped candidate check and normalizes AST export names before matching hook names. Constraint: Server-only export stripping must fail closed for unstrippable hook exports Rejected: Parse every module unconditionally | existing behavior avoids parsing modules with no plausible hook export and preserves invalid non-hook fixture behavior Confidence: high Scope-risk: narrow Directive: Match server-only hooks on normalized exported names, not raw source spellings or local declaration names Tested: deno test --preload=src/testing/preload.ts --no-check --allow-all src/transforms/pipeline/stages/browser-server-exports-strip.test.ts Not-tested: Full repository test suite
The branch needed current main to clear conflicts while preserving both transform hardening lines: main’s destructuring closure rule and this branch’s pinned-name/import-equals guard. The conflict was resolved by requiring every binding in a destructuring declaration to be hook-owned before pruning while still treating pinned JSX factory names as live. Constraint: Merge must be non-force and preserve origin/main history Rejected: Prefer either side wholesale | each side carried a distinct regression fix Confidence: high Scope-risk: moderate Directive: Do not relax the destructuring closure check from every() to some(), and do not drop pinned-name liveness from unused checks Tested: deno test --preload=src/testing/preload.ts --no-check --allow-all src/transforms/pipeline/stages/browser-server-exports-strip.test.ts Not-tested: Full repository test suite after merge
Merging current main changed public surfaces enough that the generated API reference check failed. This commit contains the generated output from the repository generator so the branch does not carry stale generated docs. Constraint: Generated artifacts must be regenerated, not hand-edited Rejected: Leave docs stale | docs:api-reference:check failed after the main merge Confidence: high Scope-risk: narrow Directive: Regenerate docs/api-reference with deno task docs when exported public surfaces change Tested: deno task docs Not-tested: docs:api-reference:check after this commit
The transform fix does not change public API output. The earlier generated reference update came from a local generator/runtime mismatch, so keep the PR focused by returning API reference files to the origin/main state verified with the pinned Deno 2.7.7 binary. Constraint: Parent lane requires no broad API-doc churn in this transform-only PR Rejected: Force-push away the generated docs commit | non-force history is required for this PR lane Confidence: high Scope-risk: narrow Directive: Regenerate API reference for this PR only with the pinned Deno 2.7.7 binary unless public exports change Tested: PATH=/private/tmp/veryfront-deno-2.7.7-aarch64-apple-darwin:$PATH deno task docs:api-reference:check Not-tested: Full docs suite after this cleanup commit
|
Draft readiness audit at e41b944: hosted CI is green, the branch is mergeable, review threads are clear, escaped and string-literal export names are covered, the semantic #3861 merge keeps both the all-bindings closure rule and pinned JSX-factory guard, and unrelated API-reference churn is gone. This should still not be marked Ready by automation. The current governing issue is veryfront/veryfront-issue-inbox#605, which requires an explicit maintainer disposition: close this overgrown spike as recommended there, or name a human reviewer and freeze this SHA. If retained, the PR description must also be refreshed from closed #112 and stale 30f34a8 measurements to #605 and the current head before review. |
Review: 61/100. Request changes (real defects, fixable in place)This is a draft and you have asked repeatedly that it not be marked ready, so treat this as a review of the code, not a merge signal. The architecture is right, the evidence behind it is the strongest I have seen on this repo, and I independently reproduced your central claims: both halves of the ordering change are independently pinned, the sourcemap now survives with no secret in Score breakdown
Blocking issues1. A unicode-escaped hook export name leaks the hook, the secret and the server import.
// app/dashboard/page.tsx
import { getEnv } from "veryfront";
const KEY = getEnv("SERVER_ONLY_HOOK_SOURCE");
async function loadIt() { return { props: { k: KEY } }; }
export { loadIt as get\u0053erverData };
export default function Page() { return null; }The raw source contains neither the substring The secret initialiser is there, the real hook body is there instead of the Three spellings reproduce, all clean on
I proved the gate is the sole cause: replacing This is caused by the reorder, and it is worth stating why, because it is the general risk in moving a stage ahead of It also fails open, which nothing else in this pass does. Fix, minimal: function mayNameServerOnlyExport(code: string): boolean {
if (!/\bexport\b/.test(code)) return false;
if (SERVER_ONLY_EXPORTS.some((name) => code.includes(name))) return true;
if (/\bexport\s*(?:\*\s+as|{[\s\S]*?\bas)\s+["']/.test(code)) return true;
// A unicode or hex escape can spell any character of a hook name inside a
// plain IdentifierName: `export { loadIt as get\u0053erverData }`. Parse and
// let `exportName` normalize, rather than trusting raw source text.
return /\\u|\\x/.test(code);
}2. The test that would have caught issue 1 is the one The header records Add the closing case: compile the module, read the export names off the parsed artifact, and assert that the set of hook-named exports the pass acted on equals the set of hook-named exports the artifact actually has. That one assertion fails on all three spellings above and on any future spelling, which is exactly the differential principle your ninth commit argues for. The two cases Non-blocking
Interaction with #3846No semantic conflict, a large textual one, and #3855 has to go first.
#3846 is +4265/-582 on One overlap worth telling the #3846 reviewer: #3846's headline is the destructured server-value leak, and #3861 already closed two of the five shapes on Both PRs also sit under Verification performedWorktree at Is the change inert? No. Each half pinned independently. Exactly the claim in your body, and each half fails one and only one test. Mutation check on the Leak fixtures. I wrote a probe running ten authoring shapes through the real
Sourcemap fidelity, Diff composition. Suites. What I did not verify. I did not run |
…llings
Running the strip before compile means its pre-parse gate reads authored
source instead of esbuild's normalized output, so an escaped export name
survives into the text the gate inspects. `export { loadIt as getServerData }`
contains neither the substring `getServerData` nor a quote after `as`, so
the gate returned false, the module was never parsed, and the hook body,
the secret it closes over and its `veryfront` server import all reached
the browser artifact. `main` strips all three, because after compile the
escape is already gone.
Widening the gate one spelling at a time does not close it. Two more
shapes leaked the same way and neither is a unicode escape the eye
catches: `export{loadIt as"getServerData"}` misses the old quote
clause because it has no space after `as`, and a string-literal name
split by a line continuation spells no hook name at all in raw text.
Decide it from the grammar instead. An exported name is an
IdentifierName or a StringLiteral, and in both the only way to write a
character as anything but itself is an escape sequence, which always
starts with a backslash. A module with no backslash spells every name it
exports verbatim, so the substring test settles it; anything else parses.
That is sound for every spelling, including ones nobody has written yet.
Measured over `templates/files` (60 modules, 54 KB, 20 runs): the old
gate 1.05 ms per tree, this gate 1.76 ms, parsing every exporting module
3.76 ms. Over `src/transforms` (282 modules, 2.5 MB, a pessimistic
regex-heavy upper bound): 16.6 ms, 52.8 ms and 67.3 ms. Soundness costs
0.7 ms on a realistic tree and stays cheaper than parsing everything.
`SERVER_ONLY_EXPORTS` becomes exported so the test reads the real policy
list rather than a copy that can drift from it.
The header excused `SERVER_ONLY_EXPORTS` as framework policy with no differential partner. That conflated two questions. The SET of hook names is policy and has no oracle. Deciding whether a module exports one of them is a parser rule, and esbuild answers it exactly. Every other fixture here reads a predicate's verdict off the artifact; none asked the artifact which names the module exports, which is why a gate that could not see an escaped export name went unnoticed. This adds that case. It compiles the module, reads the export names off the parsed artifact, and asserts the set of hook-named exports the pass emptied equals the set the artifact actually has. Set equality against the compiler fails for any spelling, so a fourth or fifth way to write the name needs no new fixture. Ten spellings drive it, including a plain declaration, a plain clause, a string-literal name, three escaped IdentifierName forms, an escaped string-literal name with no space after `as`, a name split by a line continuation, two hooks at once and a module with no hook at all. A guard case asserts the hidden spellings really are hidden, so a `\u` this file resolved at its own parse time cannot pass for the wrong reason. Five of the ten fail against the gate the previous commit replaced.
kwakayama
changed the title
Blocker closed: the pre-parse gate now decides from the grammarBoth blocking findings are fixed. The PR stays a draft.
Head is now What was wrong, and what the fix isYou had it exactly right: Two more shapes leak the same way, and the second one no escape-specific regex can reach:
So instead of adding a spelling, the gate now answers from the grammar. An exported name is an IdentifierName or a StringLiteral. In both, the only way to write a character as anything other than itself is an escape sequence, and every escape sequence starts with a backslash. A module whose text holds no backslash therefore spells every name it exports verbatim, and the substring test settles it. Anything else parses and lets function mayNameServerOnlyExport(code: string): boolean {
if (!/\bexport\b/.test(code)) return false;
if (SERVER_ONLY_EXPORTS.some((name) => code.includes(name))) return true;
return code.includes("\\");
}That is one predicate covering every spelling, including the two your patch leaves open. Narrow regex or fail closed: measured, then neitherYou asked me to measure the build-time cost of biasing toward parsing. I did, over two trees, 20 runs averaged, whole tree per run.
On the app tree, 5 of the 54 exporting modules carry a backslash and now parse. Soundness costs 0.7 ms there and stays cheaper than parsing everything, so there was no reason to trade it away. The grammar argument is what makes that possible: the set of spellings is closed by the language, so a sound gate covers exactly the same ground as fail-closed at a fraction of the cost. Parsing everything would also turn any module the parser chokes on into a hard build error, which the current gate does not do. Fail-first proofMandatory for a security fix, so here it is. The differential test was written and run before the gate changed. RED, at with, for the first: GREEN at Artifact evidence, through the real
|
| shape | main 3a109046d0 |
e41b94476d |
191234f9 |
|---|---|---|---|
export { loadIt as get\u0053erverData } on .tsx |
stubbed | leaks all three | stubbed |
export async function get\u0053erverData() |
stubbed | leaks all three | stubbed |
export { loadIt as get\u0053erverData } on .js |
stubbed | leaks all three | stubbed |
export{loadIt as"get\u0053erverData"}, no space after as |
stubbed | leaks all three | stubbed |
| string-literal name split by a line continuation | stubbed | leaks all three | stubbed |
export { loadIt as "getServerData" } |
stubbed | stubbed | stubbed |
Before, at e41b94476d, spelling 1:
var n=Object.defineProperty;var t=(r,e)=>n(r,"name",{value:e,configurable:!0});
import{getEnv as o}from"/_vf_modules/_veryfront/index.client.js";
const a=o("SERVER_ONLY_HOOK_SOURCE");
async function c(){return{props:{k:a}}}t(c,"loadIt");
function u(){return null}t(u,"Page");
export{u as default,c as getServerData};
After, at 191234f9, same input:
var n=Object.defineProperty;var r=(e,t)=>n(e,"name",{value:t,configurable:!0});
async function l(){throw new Error("server-only")}r(l,"loadIt");
function o(){return null}r(o,"Page");
export{o as default,l as getServerData};
Secret gone, hook body stubbed, veryfront import gone. Byte-equivalent in effect to main.
The differential test
compiler-predicates.test.ts no longer excuses this. It compiles the module, reads the export names off the parsed artifact, and asserts the set of hook-named exports the pass emptied equals the set the artifact actually has. Set equality against the compiler fails for any spelling, so a seventh way to write the name needs no new fixture. Ten spellings drive it, plus a guard case asserting the hidden ones really are hidden, so a \u the test file resolved at its own parse time cannot pass for the wrong reason. SERVER_ONLY_EXPORTS is exported from the stage now so the test reads the real policy list rather than a copy that can drift.
The header comment keeps the policy/rule distinction rather than deleting the note, because conflating them is what hid this.
Non-blocking items
- Commit type fixed. Title is now
fix(transforms)!: run the browser server-exports strip before compile. - PR body refreshed. SHAs corrected to
3a109046d0and191234f9, the governing issue changed from closed feat: File-based SDLC conventions (issue #102) #112 to fix: remove unused vars in pipeline.behavior.test.ts #605, and a new section documents the gate. - Merge-conflict section updated.
51b31a52c4resolved it keepingmain's.everyand thepinnedterm, andgit merge-tree --write-tree origin/main HEADat191234f9returns a tree with zero conflict entries. - Mutant item closed. Verified independently: deleting
&& !pinned.has(name)and runningbrowser-server-exports-strip.test.tsgives 0 passed, 1 failed, killed bykeeps partially hook-owned destructuring and pinned JSX factory names. Removed from the handover. .gitignoresplit. Already done. That line lives alone inafc3eb7056fix(repo): remove the stray node_modules symlink committed by mistake, which touches only.gitignoreand the stray symlink, so it cherry-picks out as it stands. No new commit needed.
Still open, unchanged
Rest, computed key and sibling default destructuring, and export * from. Re-measured: they leak identically on main at 3a109046d0 and on this head, so they are pre-existing and out of scope. The two SSR stages are untouched.
Gates
deno test --preload=src/testing/preload.ts --no-check --allow-all src/transforms/ EXIT=0
ok | 162 passed (2838 steps) | 0 failed
deno check <2 touched files> EXIT=0
deno lint <2 touched files> EXIT=0
deno fmt --check <2 touched files> EXIT=0
The full src/transforms/ suite was re-run after the last edit, at the pushed HEAD. The 41-cell mutation matrix and the differential suite are intact: nothing in them broke.
One thing I could not re-measure, stated plainly
The whole-repository verification table in the body was measured at main c4e93cd8 against branch 38a0a0be. I could not re-run it here. In this sandbox, running those roots together aborts 582 of 636 files at module load with The current server runtime does not expose complete node:util/types brand checks from src/platform/compat/native-brand-checks.ts:98. It does the same on origin/main at 3a109046d0 with no branch content at all, in parallel and serial alike, and the same files pass when run in smaller groups. That is an environment fault and a number produced through it would say nothing, so the table stands as previously measured and the body now says so. Hosted CI is the authority for that column.
Still a draft.
Takes in 41 commits of `main`, including the regenerated `templates/manifest.generated.ts`. Without it `deno task typecheck` failed on this branch at its first step, `generate:manifests:check`, with `./templates/manifest.generated.ts is stale`, while the same command passed on plain `origin/main`. The manifest decompresses to identical content on both trees, so only the committed gzip bytes were behind; the merge takes `main`'s. No conflicts. The strip stage, the pipeline ordering and the pre-parse gate are unchanged by the merge.
Review: 94/100 - Approve, and it stays a draftThe blocker from the last pass is closed and I verified it end to end myself rather than reading the fix comment: reverting the gate on the current head makes five escaped spellings ship the secret, the hook body and the Score breakdown
Changes made in this pass1. Merged
The same command on plain Worth recording why, because it looks like a content drift and is not one. I decompressed both copies: byte-identical, 990887 characters, the same 48 templates and 467 files. 2. Re-measured the PR body against the real Two items the last review raised needed no work, and I confirmed both rather than taking them on trust:
Security fix, re-verifiedNot read off the fix comment. Measured through the real
The third column is the fail-first proof, taken on the current tree rather than on the old head: I replaced the gate at and with the gate restored, same input, after the
I checked the grammar argument rather than assuming it. Interaction with #3846Re-checked at the current SHAs. #3855 goes first, and the evidence is the merge, not a preference.
#3846 is +9165/-584 across 4 files against a merge base of One correction to the framing I was handed. It is not true that this PR deletes the Scope, assessed honestlyTwo commits are genuinely separable and would cut about 1050 lines from this review: The rest does not split. The docs change describes the break this PR creates, the differential suite exists because of the gate this PR moved, and Verification performedIsolated worktree, head Before the After the The one failure is pre-existing and I proved it on Run alone on this branch: exit 1, 5213.5 ms. Run alone on plain Other checks:
What I did not verify. Still a draft. Not marked ready, not merged. |
Marked ready for reviewI flipped this out of draft. Your body says "Draft. Do not mark ready or merge.", so recording plainly that this overrides that line, on the repo owner's instruction rather than on my own judgement. Convert it back if you disagree, nothing here is irreversible. What the review pass established, at
Two things a reviewer should still weigh, and neither is a defect:
Landing order still matters. This should land before #3846. Moving the strip ahead of |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f1d0c41b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A page can bind a server-only hook through a TypeScript import-equals
alias:
import Loader = require("./server");
export { Loader as getServerData };
`exportedHookBindings` records `Loader` as a hook local, but
`emptyServerOnlyHooks` only stubs function and variable declarations. A
`TSImportEqualsDeclaration` is neither, so it reported no change and
`stripServerOnlyExports` returned the source untouched -- shipping
`require("./server")`, and with it the server module's whole graph, into
the browser artifact. Verified against the real pass: no throw, source
byte-identical, `require("./server")` still present.
This is reachable only because the pass now runs ahead of
`compilePlugin`. Running after compile, esbuild had already lowered the
alias to a `const` that the variable-declaration path stubbed.
Rejecting rather than stubbing, which is the same fail-closed answer the
other shapes this pass cannot take apart already get -- a re-export from
another module, a destructured `export const { … } = …`. Silently
passing the source through is the one outcome that must not happen,
because it defeats the pass entirely.
Covers the exported form (`export import getServerData = require(…)`)
as well, and is scoped to bindings a hook actually resolves to, so an
unrelated import-equals in the same module still strips normally.
|
Addressed the P1 import-equals finding in @codex review |
There was a problem hiding this comment.
💡 Codex Review
veryfront-code/src/transforms/pipeline/stages/browser-server-exports-strip.ts
Lines 900 to 905 in 2262c26
Bind named class expressions in their class scope
When a hook-only helper contains a named class expression such as const Factory = class Inner { static make() { return Inner; } }, the class name is scoped within the class body, but this branch binds the ID only for ClassDeclaration. The walker therefore reports Inner as a free module reference; after pruning Factory, the hook closure expands to Inner and can incorrectly remove an unrelated top-level declaration such as const Inner = initClient(), including its client-side initialization. Traverse a named class expression with its ID bound in a class-local scope.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Closed in favor of the bounded replacement #3956. I reviewed the full comment and review history before closing. The valid findings and behavior are preserved in #3956:
The replacement also preserves the direct code-splitter helper contract and has an independent approval plus a green full local pre-push gate. The original PR is not a good merge unit: it is thousands of added lines, carries stale history, and mixes the ordering fix with broad reference-classification and mutation corpora. Those broad corpora are intentionally not imported. The security and compatibility behavior that still matters is covered directly at the real pipeline seam in #3956. Superseded, not rejected in principle. Continue review and landing on #3956. |
The browser artifact should never feed server-only hook bodies into custom pre-compile plugins, esbuild keepNames output, or development sourcemaps. Moving the browser strip pass ahead of compile makes the ordering explicit and removes the compile-stage metadata handoff. The strip pass now classifies only value references while preserving authored JSX pragmas. Direct helper callers keep the legacy behavior for unrelated unused imports by demoting them to side-effect imports. The browser pipeline passes an explicit TypeScript-only option so TS and TSX inputs leave unrelated named imports authored for the following compile stage to erase. Hook-owned project imports are still deleted when no surviving browser code reads any binding from that statement, so their transitive server graph is not kept alive. JS, JSX, and generated MDX browser artifacts retain only a bare side-effect import for unrelated unused project imports. Precompile parsing now accepts the compiler-supported decorator placements before and after export. Authored parser source diagnostics are surfaced with the existing compilation-error tenant-build-failure shape, while missing parser and parser-internal failures remain framework owned. Constraint: Browser pipeline custom plugins are sorted by numeric stage, so the plugin stage and pipeline array order must both place stripping between parse and compile. Constraint: Re-exported and import-equals server hooks have no local body that the strip pass can safely empty. Rejected: Cherry-pick PR #3855 wholesale | it carries unrelated broad predicate/corpus work beyond the independently landable strip-before-compile fix. Rejected: Keep compile sourcemap metadata restoration | stripping before compile makes the browser compile map naturally originate from stripped input. Confidence: medium Scope-risk: moderate Directive: Keep browser-server-exports-strip before compile unless unused-import, parse-diagnostic ownership, and source-map behavior are redesigned together. Tested: deno task test:file extensions/ext-parser-babel/src/index.test.ts src/transforms/pipeline/stages/browser-server-exports-strip.test.ts src/transforms/pipeline/stages/compile.test.ts src/transforms/pipeline/index.test.ts src/build/bundler/code-splitter/esbuild-plugin.test.ts Tested: deno fmt --check extensions/ext-parser-babel/src/index.test.ts extensions/ext-parser-babel/src/parser-only.ts src/transforms/esm/transform-utils.ts src/transforms/pipeline/stages/compile.ts src/transforms/pipeline/stages/browser-server-exports-strip.ts src/transforms/pipeline/stages/browser-server-exports-strip.test.ts Tested: deno lint extensions/ext-parser-babel/src/index.test.ts extensions/ext-parser-babel/src/parser-only.ts src/transforms/esm/transform-utils.ts src/transforms/pipeline/stages/compile.ts src/transforms/pipeline/stages/browser-server-exports-strip.ts src/transforms/pipeline/stages/browser-server-exports-strip.test.ts Tested: deno check extensions/ext-parser-babel/src/index.test.ts extensions/ext-parser-babel/src/parser-only.ts src/transforms/esm/transform-utils.ts src/transforms/pipeline/stages/compile.ts src/transforms/pipeline/stages/browser-server-exports-strip.ts src/transforms/pipeline/stages/browser-server-exports-strip.test.ts Tested: git diff --check Tested: rg -n Unicode dash scan across touched files, no matches Tested: rg -n console.log, empty catch, and hardcoded apiKey patterns across touched files, no matches Not-tested: Full repository test suite Related: #3855
The browser artifact should never feed server-only hook bodies into custom pre-compile plugins, esbuild keepNames output, or development sourcemaps. Moving the browser strip pass ahead of compile makes the ordering explicit and removes the compile-stage metadata handoff. The strip pass now classifies only value references while preserving authored JSX pragmas. Direct helper callers keep the legacy behavior for unrelated unused imports by demoting them to side-effect imports. The browser pipeline passes an explicit TypeScript-only option so TS and TSX inputs leave unrelated named imports authored for the following compile stage to erase. Hook-owned project imports are still deleted when no surviving browser code reads any binding from that statement, so their transitive server graph is not kept alive. JS, JSX, and generated MDX browser artifacts retain only a bare side-effect import for unrelated unused project imports. Precompile parsing now accepts the compiler-supported decorator placements before and after export. Authored parser source diagnostics are surfaced with the existing compilation-error tenant-build-failure shape, while missing parser and parser-internal failures remain framework owned. Constraint: Browser pipeline custom plugins are sorted by numeric stage, so the plugin stage and pipeline array order must both place stripping between parse and compile. Constraint: Re-exported and import-equals server hooks have no local body that the strip pass can safely empty. Rejected: Cherry-pick PR #3855 wholesale | it carries unrelated broad predicate/corpus work beyond the independently landable strip-before-compile fix. Rejected: Keep compile sourcemap metadata restoration | stripping before compile makes the browser compile map naturally originate from stripped input. Confidence: medium Scope-risk: moderate Directive: Keep browser-server-exports-strip before compile unless unused-import, parse-diagnostic ownership, and source-map behavior are redesigned together. Tested: deno task test:file extensions/ext-parser-babel/src/index.test.ts src/transforms/pipeline/stages/browser-server-exports-strip.test.ts src/transforms/pipeline/stages/compile.test.ts src/transforms/pipeline/index.test.ts src/build/bundler/code-splitter/esbuild-plugin.test.ts Tested: deno fmt --check extensions/ext-parser-babel/src/index.test.ts extensions/ext-parser-babel/src/parser-only.ts src/transforms/esm/transform-utils.ts src/transforms/pipeline/stages/compile.ts src/transforms/pipeline/stages/browser-server-exports-strip.ts src/transforms/pipeline/stages/browser-server-exports-strip.test.ts Tested: deno lint extensions/ext-parser-babel/src/index.test.ts extensions/ext-parser-babel/src/parser-only.ts src/transforms/esm/transform-utils.ts src/transforms/pipeline/stages/compile.ts src/transforms/pipeline/stages/browser-server-exports-strip.ts src/transforms/pipeline/stages/browser-server-exports-strip.test.ts Tested: deno check extensions/ext-parser-babel/src/index.test.ts extensions/ext-parser-babel/src/parser-only.ts src/transforms/esm/transform-utils.ts src/transforms/pipeline/stages/compile.ts src/transforms/pipeline/stages/browser-server-exports-strip.ts src/transforms/pipeline/stages/browser-server-exports-strip.test.ts Tested: git diff --check Tested: rg -n Unicode dash scan across touched files, no matches Tested: rg -n console.log, empty catch, and hardcoded apiKey patterns across touched files, no matches Not-tested: Full repository test suite Related: #3855
The browser artifact should never feed server-only hook bodies into custom pre-compile plugins, esbuild keepNames output, or development sourcemaps. Moving the browser strip pass ahead of compile makes the ordering explicit and removes the compile-stage metadata handoff. The strip pass now classifies only value references while preserving authored JSX pragmas. Direct helper callers keep the legacy behavior for unrelated unused imports by demoting them to side-effect imports. The browser pipeline passes an explicit TypeScript-only option so TS and TSX inputs leave unrelated named imports authored for the following compile stage to erase. Hook-owned project imports are still deleted when no surviving browser code reads any binding from that statement, so their transitive server graph is not kept alive. JS, JSX, and generated MDX browser artifacts retain only a bare side-effect import for unrelated unused project imports. Precompile parsing now accepts the compiler-supported decorator placements before and after export. Authored parser source diagnostics are surfaced with the existing compilation-error tenant-build-failure shape, while missing parser and parser-internal failures remain framework owned. Constraint: Browser pipeline custom plugins are sorted by numeric stage, so the plugin stage and pipeline array order must both place stripping between parse and compile. Constraint: Re-exported and import-equals server hooks have no local body that the strip pass can safely empty. Rejected: Cherry-pick PR #3855 wholesale | it carries unrelated broad predicate/corpus work beyond the independently landable strip-before-compile fix. Rejected: Keep compile sourcemap metadata restoration | stripping before compile makes the browser compile map naturally originate from stripped input. Confidence: medium Scope-risk: moderate Directive: Keep browser-server-exports-strip before compile unless unused-import, parse-diagnostic ownership, and source-map behavior are redesigned together. Tested: deno task test:file extensions/ext-parser-babel/src/index.test.ts src/transforms/pipeline/stages/browser-server-exports-strip.test.ts src/transforms/pipeline/stages/compile.test.ts src/transforms/pipeline/index.test.ts src/build/bundler/code-splitter/esbuild-plugin.test.ts Tested: deno fmt --check extensions/ext-parser-babel/src/index.test.ts extensions/ext-parser-babel/src/parser-only.ts src/transforms/esm/transform-utils.ts src/transforms/pipeline/stages/compile.ts src/transforms/pipeline/stages/browser-server-exports-strip.ts src/transforms/pipeline/stages/browser-server-exports-strip.test.ts Tested: deno lint extensions/ext-parser-babel/src/index.test.ts extensions/ext-parser-babel/src/parser-only.ts src/transforms/esm/transform-utils.ts src/transforms/pipeline/stages/compile.ts src/transforms/pipeline/stages/browser-server-exports-strip.ts src/transforms/pipeline/stages/browser-server-exports-strip.test.ts Tested: deno check extensions/ext-parser-babel/src/index.test.ts extensions/ext-parser-babel/src/parser-only.ts src/transforms/esm/transform-utils.ts src/transforms/pipeline/stages/compile.ts src/transforms/pipeline/stages/browser-server-exports-strip.ts src/transforms/pipeline/stages/browser-server-exports-strip.test.ts Tested: git diff --check Tested: rg -n Unicode dash scan across touched files, no matches Tested: rg -n console.log, empty catch, and hardcoded apiKey patterns across touched files, no matches Not-tested: Full repository test suite Related: #3855
2 participants
What this changes
This changes the pipeline ordering for the browser target.
browserServerExportsStripPluginnow runs beforecompilePlugininstead of after it:Two edits were required, not one.
src/transforms/pipeline/index.ts:334-348uses array order while no custom plugin is registered, and re-sorts the whole pipeline byplugin.stageas soon as one is. So the array position moved and the stage changed fromTransformStage.COMPILE + 0.6toTransformStage.PARSE + 0.5. Each half has its own test, and reverting either half alone turns exactly one test red.Behaviour change: a re-exported data hook now fails the browser build
This is the one user-facing break in the PR, and it is not a side effect of the reorder: it is the reorder doing its job loudly instead of quietly. Both of these authoring forms work today on
mainand stop the build on this branch.Measured through the real browser
runPipeline,ssr: false, production output, re-measured onmain(3a109046d0) and on this head (191234f9):mainexport { getServerData } from "./loaders.ts"import{getServerData as l}from"./loaders.js";…export{n as default,l as getServerData}ServerExportStripErrorexport { loadDashboard as getServerData } from "./loaders.ts"import{loadDashboard as d}from"./loaders.js";…export{o as default,d as getServerData}ServerExportStripErrorRead the
maincolumn carefully. The loader module is imported by the browser artifact, so it and its whole transitive graph execute in the browser, and the hook is not even stubbed: the artifact re-exports the realgetServerData. That is the leak this stage exists to close, and onmainit is silent.The direction of the change is right, but it does turn a working authoring pattern into a build error, so it needs a migration note rather than a release-note footnote. The error message carries the supported form, and
docs/guides/data-fetching.mdcarries it too, added in this PR:A re-export binds no local declaration, so there is no body to empty. Emptying is what keeps the export clause valid while the loader is removed; without a local binding the pass has nothing to act on and the only alternatives are shipping the loader or failing. It fails.
Why
compile.tssetskeepNames: true, so the strip stage was spending its effort recognising__name(fn, "fn")registrations that Veryfront's own compile stage had injected one position earlier in the same pipeline. The comment justifying that recognition said release modules arrive compiled. Both release paths insrc/release-assets/build-executor.tstransform uncompiled source, no framework module exports a server hook, and the other caller (src/build/bundler/code-splitter/esbuild-plugin.ts:63) already reads raw project files. Running the strip first means no keepNames helper is ever emitted for a declaration the pass removes, so the recognition stops being necessary rather than being solved.What was deleted, and why it existed
The compile/strip sourcemap stash-and-restore handshake:
COMPILE_SOURCE_MAP_DIRECTIVE_METADATAandCOMPILE_SOURCE_MAP_INPUT_METADATA, plustrailingSourceMapDirectiveanddropTrailingSourceMapDirective(31 lines incompile.ts).transform, plusappendSourceMapDirective(22 lines in the stage).It existed because compile ran first and embedded pre-strip source in its development sourcemap, so a stripped hook could still be recovered from
sourcesContent. The stage had to drop that map, and the handshake was there to put it back when nothing was actually stripped. With the strip first, the compiler builds its map from already-stripped input, so there is nothing to defend and the map simply survives.The keepNames recognition itself is not deleted here. That deletion is a separate change, and this PR keeps the diff to the ordering.
Second commit: comment preservation
@jsxImportSource preacton the line above a removed import was being dropped. Babel attaches a file's opening comments to its first statement, so removing that statement took the pragma with it and the page silently rendered through the configured default JSX factory. Legal banners and lint suppressions went the same way. The fix relocates a removed statement's leading comments onto the next surviving statement, and comments that trail the last surviving statement are reattached there.Third commit: unrelated unused imports are left as authored
dropUnusedImportBindingsreduced an unused import it does not own to a bare side-effect import, described in the code as being "for compatibility with the older conservative behavior". It no longer does. It removes only the specifiers the stripped hooks actually owned and leaves everything else exactly as authored.The demotion was an artifact of the strip running last, like the sourcemap handshake above. With nothing downstream to clean up, reducing an unused named import to
import "./x.ts"conservatively preserved the module's side effects. Withcompilerunning after the strip, the same reduction converts an erasable named import into a non-erasable bare side-effect import. Side-effect imports are preserved by design, so the demotion defeated the cleanup that would otherwise drop the module. That is the whole reason the shape leaked post-reorder and did not before, and whymainis clean: there the compiler erases the import before the pass ever sees the module.Two conditions the decision put on the change, both met:
import "./analytics.ts"as authored is still a side-effect import in the artifact. Pinned bykeeps a bare side-effect import untouchedat the stage level and bykeeps a genuine side-effect import in the artifactend to end throughrunPipeline.compilerunning after the strip. APIPELINE ORDERING DEPENDENCYnote now sits at the call site instripServerOnlyExports, namingTransformStage.PARSE + 0.5andTransformStage.COMPILEand stating that a revert of the ordering has to bring the reduction back. Thestagefield carries a pointer to it, and the note also records how the second caller (createSplitterPlugin) meets the same requirement, by stripping inside an esbuildonLoadhook so the bundle step tree-shakes afterwards.Eleven tests encoded the old contract. None were bulk-updated; each was classified first.
leaves an unrelated unused project import exactly as authored(renamed fromkeeps an unrelated unused project import as a side-effect import),deletes a mixed import whose surviving specifier nothing reads(renamed fromkeeps side effects for ordinary imports with mixed hook-only bindings)keeps every specifier of a mixed import the browser still reads, pins the other sidekeeps an unrelated import when ... shadows its namescope cases,does not keep an import binding whose name matches an enum member,does not keep an import shadowed by a hoisted namespace alias,does not keep an import shadowed by a namespace-local enumsecretOnlyis removed while the falsely-live binding is notstill erases an undecorated declared property,still deletes an import a same-scope enum genuinely shadows(renamed fromstill reduces ...)keeps a bare side-effect import untouchedThe two re-anchored cases need calling out, because "update to the new expected output" would have made them vacuous. Both were paired contrasts (
@audit declare idversus plaindeclare id; a block-scoped enum versus a same-scope enum) whose two halves were told apart only by "import demoted" versus "import intact". Once the demotion is gone, both halves produce an intact import and the pair stops discriminating. The fixtures now put the imported binding inside the hook's closure, so the contrast is between "import deleted" and "import intact" instead. Their partners (keeps a decorator on a declared property, which still emits a runtime callanddoes not hoist a block-scoped enum into the enclosing function scope) got the same fixture change for the same reason; without it they would have passed no matter what the walker classified.Fourth commit: the same rule on every source dialect
The third commit left the specifiers the hooks did not own in place on a mixed import, on the reasoning that the compiler erases what is genuinely unused. That reasoning holds only for the
tsandtsxloaders, where a specifier may name a type and elision is permitted..jsmaps to thejsloader,.jsx,.mdand.mdxtojsx, and under those loaders esbuild must preserve the module for its side effects, so it rewrites the remainder into exactly the bare side-effect import this stage forbids. The stage emits byte-identical output forprobe.tsx,probe.jsxandprobe.js, so the loader alone decided the outcome and a.tsx-only fixture could not see it.dropUnusedImportBindingsnow deletes the whole statement whenever the stripped hooks owned at least one specifier and no surviving specifier is read, on every dialect. An import the hooks own nothing of is still left exactly as authored, and a bare side-effect import is still untouched. Tests cross the dialect axis (ts,tsx,js,jsx,mdx) with the import shapes (mixed specifiers, a default-import sibling, both import-equals forms) in dev and prod; the JavaScript-dialect cells fail without the fix and the TypeScript ones pass either way.Fifth commit: exported import-equals
moduleScopeDeclarationsnow seesTSImportEqualsDeclaration, and Babel representsexport import A = require("./a.js")as that same node withisExport: truerather than wrapping it in an export declaration, so the "exported declarations are never candidates" rule needed the flag spelled out. Without it a hook-only exported alias would be pruned out of the module's contract.Behaviour change worth being explicit about
For a mixed import such as
import { initClient, loadSecret } from "./client-setup.ts"where the hook owned onlyloadSecretand nothing browser-side readsinitClient,mainkeepsimport "./client-setup.js"in the artifact and this branch deletes the import. That is the intended consequence of the decision, not an accident: the module was reachable only through server-only code. It is the one shape where the artifact differs frommainin the other direction, and it is a removal, not a leak. A mixed import the browser still reads any specifier of keeps every specifier it was authored with.Verification
Re-measured at this head. The table this replaces reported
maind260ddf4against0e476a84, the third of twelve commits, at 4220 passed and 106 failed withtests/integrationandtests/e2eunfinished. That row is stale in every column: the branch has moved six commits past it,mainhas moved, and the machine that produced those 106 failures was starved by orphaned processes from an unrelated session. Both trees below were built bygit archivefrom a shared object store, run concurrently with a distinctVERYFRONT_BINARYeach, on the same machine at load average 3, with the command and environment fromAGENTS.md.mainc4e93cd838a0a0becli docs extensions react storybook templates rfcssrctests/minuse2eandintegrationThis table has not been re-run at the current SHAs, and the reason is worth stating rather than papering over. In the sandbox available now, running those roots together aborts 582 of 636 files at module load with
The current server runtime does not expose complete node:util/types brand checksfromsrc/platform/compat/native-brand-checks.ts:98. It does so identically onorigin/mainat3a109046d0with no branch content at all, in parallel and serial alike, while the same files pass when run in smaller groups. That is an environment fault, not a signal about either tree, and a number produced through it would say nothing. The table below stands as measured atc4e93cd8against38a0a0be. Hosted CI is the authority for the whole-repository column at the current head.The counts are
deno test's top-level test counts, not steps. They are not expected to be equal and were not equal in the previous measurement either: the branch forked ate37285b3andmainhas gained tests since, so the two trees do not contain the same set of test files. What is comparable is which files fail.19 test files fail across both trees. 18 fail on both. Nothing fails only on this branch.
cli/commands/deploy/command.integration.test.ts,cli/commands/schedule/handler.test.ts,cli/commands/up/up.integration.test.ts,cli/mcp/server.test.ts,src/cache/backend.test.ts,src/embedding/embedding.test.ts,src/oauth/handlers/callback-dispatcher.test.ts,src/oauth/providers/base.test.ts,src/oauth/providers/protocols.test.ts,src/provider/model-registry.test.ts,src/provider/veryfront-cloud/provider.test.ts,src/routing/api/module-loader/esbuild-plugin.test.ts,src/routing/api/openapi/mcp-tools.test.ts,src/routing/api/route-executor.test.ts,src/runs/runs-client.test.ts,src/server/handlers/request/agent-stream.handler.test.ts,src/workflow/blob/veryfront-cloud-storage.test.ts,tests/docs/guide-examples.test.tsmainsrc/transforms/mdx/esm-module-loader/module-fetcher/dependency-recovery.test.tsEvery one of the 18 is a sandbox failure: this machine has no outbound network, so they fail with SOCKS connect failures, 401s from real API endpoints, or request timeouts.
cli/mcp/server.test.tsadditionally names different failing STEPS on each run, because it binds a port and which step loses the race varies; the file fails on both either way.The single asymmetric one is not a
mainregression and not a branch improvement. It is the two concurrent runs colliding on a shared path outside both trees:Both trees write that fixture to the same absolute cache directory, so whichever run cleans up first fails the other. The file is byte-identical on the two trees (4160 bytes), and it passes on either tree run alone.
Two honest gaps, both unchanged from the previous measurement and both symmetric:
tests/integrationandtests/e2eare excluded, not clean. They are server-backed and need the network this sandbox does not have. CI covers them:tests (integration),tests (binary e2e)andtests (rsc browser e2e)run on every push to this branch.scripts/is excluded. It aborts at module load on both trees withImport "#babel/parser" not a dependency and not in import map from scripts/build/dnt-meta-property-safety.ts, identically, so it contributes nothing to either column.Merge-group pre-flight:
origin/mainatc4e93cd8merged into a scratch clone, clean merge,deno checkover the full typecheck entrypoint list clean on the merged tree (40 entrypoints, exit 0).That conflict is resolved.
a4ec5b28(#3861,fix(transforms): prune destructured server values) rewrote parts of the same file while this branch was in review.maintightenedinClosurefrom.someto.everyso a destructuring declaration is only removed when the hooks own every name it binds; this branch added&& !pinned.has(name)tounusedso a classic JSX pragma factory survives. The two are independent and the resolution keeps both.51b31a52c4merged it that way.git merge-tree --write-tree origin/main HEADreturns a tree with zero conflict entries, both atmain3a109046d0and again atmain8d0ce3c2f0.The
pinnedhalf is no longer untested either. Deleting&& !pinned.has(name)and runningbrowser-server-exports-strip.test.tsgives 0 passed, 1 failed, killed bykeeps partially hook-owned destructuring and pinned JSX factory names.deno task typecheckitself still exits non-zero on its first step,generate:manifests:check, with./templates/manifest.generated.ts is stale. Re-measured this time on plainorigin/mainin a fresh tree with no branch content at all: same failure, same message, exit 1. Pre-existing and unrelated. Thedeno checkhalf was run directly and is clean.deno task lint:ci: clean, exit 0.deno fmt --check,deno lint,deno checkon the touched files: clean.src/transforms/at this head: 162 files, 2815 steps, 0 failed.The head has moved well past
38a0a0be. It is now191234f9, after a merge ofmain(51b31a52c4), two API-reference commits, and the two commits described under The pre-parse gate below.src/transforms/was re-run at191234f9: 162 files, 2838 steps, 0 failed, exit 0.deno check,deno lintanddeno fmt --checkon the touched files: exit 0 each.Two CI checks failed on
89d59a5eand both are accounted for.tests (bun)is the JavaScriptCore table drift, fixed by30f34a83.tests (rsc browser e2e)failed in itsInstall Chromiumstep before any test ran, twice with two differentaptfaults (exit 124 retryingazure.archive.ubuntu.com, thenCould not get lock /var/lib/apt/lists/lock): runner infrastructure, unrelated to this branch. Re-running that job alone turned it green.CI/CD at
30f34a83: 20 passed, 7 skipped, 0 failed, includingtests (bun),tests (node),tests (integration),tests (binary e2e)andtests (rsc browser e2e).Security property, measured through the real reordered
runPipelineFor a module with a server hook, a hook-only helper, a server import and a secret, the browser artifact contains no server import, no secret initialiser, and a stubbed hook body. Asserted as a test on this branch.
The five pre-compile cases the spike's challengers found, each re-run end to end against current
mainand against this branch:b0f7653b)typeof KEYin a parameter type must not pinKEYReturnType<typeof schema.parse>must not drag the secret or the server moduleimport { hashOf, type Cfg }deleted, not demoted@jsxImportSourceabove a removed import survives<motion.div>) keeps its bindingexport { x } from "..."must not pin a hook-owned import of the same nameThe first three were closed by #3849. The fifth was fixed in this PR's second commit. The last two are defects 4A and 4B, closed by the seventh commit below as instances of one class rather than one at a time.
The fourth one, now closed
Measured artifact through the real browser pipeline,
ssr: false, production output:30c64eeb, this branch's previous head):var n=Object.defineProperty;var r=(e,t)=>n(e,"name",{value:t,configurable:!0});import"./lib/server-only-lib.js";async function l(){throw new Error("server-only")}...The server module and its transitive graph execute in the browser.var t=Object.defineProperty;var r=(e,n)=>t(e,"name",{value:n,configurable:!0});async function a(){throw new Error("server-only")}...No reference toserver-only-libat all.main: no reference toserver-only-libeither. Parity restored.The end-to-end assertion lives in
does not demote an unrelated unused import to a side-effect import, and its paired protection lives inkeeps a genuine side-effect import in the artifact.Sixth commit: development mode pinned
compilePluginsetstreeShaking: !ctx.dev, so the end-to-end probe above only covered the tree-shaken path. Measured acrossts,tsx,jsandjsxcrossed with dev and prod, every cell matchesmain: the erasure is TypeScript unused-import elision, which both TypeScript loaders perform in either mode, not tree shaking. The development probe is pinned as a test anyway, and the ordering note now names the mechanism so nobody reads "esbuild erases it" as tree shaking.Seventh commit: the reference walkers are exhaustive
Rounds three and four of review each found a real defect, and both fell through the same hole: a generic
visitChildrenfallback that descended into node types nobody had classified and counted their identifiers as runtime reads. TypeScript got an explicit classification in #3849 and has been stable since; everything else was ad hoc. This commit closes the class rather than the two remaining instances.src/transforms/pipeline/stages/reference-classification.tsnow classifies every node type the pinned@babel/types(npm:@babel/types@7.29.0, the specifierextensions/ext-parser-babel/deno.jsonresolves) defines, and both walkers read the same table.The classification
Node types, excluding the 65 Flow nodes (the
flowplugin is never enabled:pickPluginsalways enablestypescript, and the two are mutually exclusive) and the 4 deprecated builder-only aliases:readIdentifier,JSXIdentifiererasedTS-prefixedstructuralOf the 67
TStypes, 52 are erased and 15 emit runtime code. That split is #3849's list, moved but unchanged.Positions, not just node types, decide the answer.
isReferenceChildKeyrecords every child key that holds a fixed name, string-like text or a binding position, so both walkers skip the same subtrees instead of each keeping its own list. The corrected cells:JSXMemberExpression.objectJSXOpeningElement.name/JSXClosingElement.nameJSXIdentifieranywhereExportNamedDeclaration.specifierswhen asourceis presentExportAllDeclaration.*ExportSpecifier.exportedMemberExpression/ObjectProperty/ class member keys,TSEnumDeclaration.id,TSEnumMember.id,TSQualifiedName.right,JSXAttribute.name,JSXNamespacedName.*,JSXMemberExpression.propertyPrivateName.id,ClassPrivateProperty.key,ClassPrivateMethod.keyBreakStatement/ContinueStatement/LabeledStatement.label,MetaProperty.metaand.property,ImportAttribute.keyand.value,DirectivetextClassPrivateMethodalso now gets a function scope in the scope-aware walker, likeClassMethodandObjectMethod, instead of falling through the generic descent.Two positions are deliberately left as reads:
TSModuleDeclaration.idandTSImportEqualsDeclaration.id. They are binding positions, and the flat walker counts them on purpose, which is the over-retaining direction for module-declaration liveness.treats runtime TypeScript declaration names as bindings, not readspins that. Moving them into the table would make the flat walker delete a module-scope declaration that shares the name, so it is a separate decision and not one this PR takes.The default, and why it is asymmetric
An unrecognised node type defaults to
structural: descend, and count the identifiers under it.The two directions are not symmetric. Defaulting to a read over-retains, so a server-only import can survive into the artifact. Defaulting to "not a read" over-deletes, so live code goes and the page fails to load.
structuralis the over-retaining choice, and it is right for these walkers:compilestill elides a genuinely unused import undertsandtsxwhile the bundler still tree-shakes.ReferenceErrorat module evaluation, which no later stage can undo.An unrecognised
TS-prefixed node type keeps the opposite default,erased, unchanged from #3849. The TypeScript grammar inverts the argument: almost everything it adds is type syntax the compiler erases, and descending into an unknown type node would countp: typeof KEYas a use ofKEYand pin the import it came from.Both defaults are fallbacks, not policy. The guard test makes an unclassified node type a build failure, so in a checked build neither one fires.
The guard test
reference-classification.test.tsreadsNODE_FIELDSout of the pinned@babel/typesand asserts the classification names every type in it. A Babel upgrade that adds a node type breaks the build instead of silently leaking. A second case asserts the pinned specifier still equals the one the parser extension resolves, so a parser upgrade that leaves this test behind fails too, and a third asserts the guard reports a type the classification does not name, because a guard that cannot fail proves nothing. Two gaps in that guard, the unpinned parser and the unstated Flow assumption, are closed in the ninth commit below.The two defects, measured through
runPipeline4A, over-deletion.
isIntrinsicJsxNameimplements Babel'sisCompatTag, which is valid only for a bareJSXIdentifierthat is the element name. Run on the object of a member element name it classifiedmotion,styled,uianddialogas string tag text, so the pass deleted the binding the element reads. Three trigger paths, and the second needs no hook relationship at all:import { motion } from "./lib/motion.ts")function t(){return i(motion.div,{children:"x"})}with no import ofmotion:ReferenceErroron rendermotion.divresolvesnode:orveryfrontand hits the droppable-source branch (import { dialog } from "veryfront/ui")a(dialog.Root,...)with no import ofdialogconst styled = makeStyled())<styled.div>left danglingThis repo uses the idiom in
src/react/components/ui/{dialog,drawer,popover,tabs,toolbar,accordion,combobox,toast,toggle-group,alert-dialog,autocomplete}.tsx.4B, leak.
export { token as clientToken } from "./client-utils.js"reads no local binding:tokennames an export of the source module. Neither walker had a case for it, so the generic descent counted both halves of the specifier as free reads and kept a hook-ownedimport { token } from "./lib/server-only-lib.js"alive. Measured artifact on.jsx:import"./lib/server-only-lib.js";import{token as p}from"./lib/client-utils.js";...The server module executes in the browser, and esbuild has already demoted it to the bare side-effect form this stage forbids.server-only-libat all, on.jsxand on.mdx, where thejsxloader elides nothing downstream.The other side is pinned too: without a
source,export { token as clientToken }really is this module's binding and the import that provides it stays.Mutation testing
Every cell of the classification was reverted individually and the suite re-run. 41 distinct mutations, 39 killed:
JSXMemberExpression.objectback to a non-read (4A)keeps a lowercase member object a stripped hook also readdoes not keep a hook-only import that shares an intrinsic tag nameExportNamedDeclaration.specifiersread even with asource(4B)deletes a hook-owned import a re-export clause only looks like it reads on .jsxExportSpecifier.exportedback to a readreads the local half of an export clause with no sourceExportDefaultSpecifierandExportNamespaceSpecifierexportedback to readsreads the exported name of neither default nor namespace re-exportExportAllDeclarationkeys back to readsreads nothing of an export-all declarationJSXAttribute.nameback to a readdoes not keep a hook-only import that shares a jsx attribute nameJSXNamespacedNamehalves back to readsdoes not read either half of a namespaced nameJSXMemberExpression.propertyback to a readdoes not keep a hook-only import that shares a member element propertyMemberExpressionandOptionalMemberExpressionpropertyback to readsdoes not count a matching property name as a referenceObjectProperty.keyback to a readdoes not read an object literal keyObjectMethod.keyback to a readdoes not read a fixed method or class member keyClassMethod,ClassPropertyandClassAccessorPropertykeys back to readsdoes not read a fixed method or class member keyMemberExpressionfamily)reads a computed member property but not a fixed onereads a computed method or class member keyPrivateName.idand private class keys back to readsdoes not read a class private namedoes not read a statement labelMetaPropertyhalves back to readsdoes not read either half of a meta propertyImportAttributekey and value back to readsreads neither half of an import attributeImportSpecifierlocal and imported back to readsreads neither half of an import specifierImportDefaultSpecifier.localback to a readreads neither half of an import specifierImportNamespaceSpecifier.localback to a readreads neither half of an import specifierImportDeclarationkeys back to readsreads neither half of an import specifierFile.commentsandFile.tokensdescended intoreads neither the comments nor the tokens of a parsed filePlaceholder.nameback to a readreads the name of neither placeholder formDirectivetext back to readsreads nothing of a directiveTSEnumDeclaration.idandTSEnumMember.idback to readstreats runtime TypeScript declaration names as bindings, not readsTSQualifiedName.rightback to a readdrops a hook-only binding that matches a qualified-name propertyTSEnumDeclarationremoved from the runtime listkeeps an enum member initialiserTSEnumMemberremoved from the runtime listkeeps an enum member initialiserTSParameterPropertyremoved from the runtime listkeeps a parameter property defaultTSTypeReferenceremoved from the erased listTSTypeQueryremoved from the erased listTupleExpressionremoved from the classificationJSXIdentifierno longer a readkeeps a lowercase member object a stripped hook also readIdentifierno longer a readremoves default parameter dependencies from a function hookerasedtreats an unrecognised node as structuralstructuraltreats an unrecognised TypeScript node as erasedclassifies every node type the pinned @babel/types definesNine of those cells survived the first pass and are the reason for the eighth commit below. None of them was wrong in the implementation: each was a cell the table classified and nothing asserted, so a later edit could have flipped it silently. Cases were added per cell and every one is killed now.
Two mutations survive and always will.
V8IntrinsicIdentifier.name,DirectiveLiteral.valueandInterpreterDirective.valuearestringfields inNODE_FIELDS, andreferenceChildrencollects only node-shaped values, so naming those keys in the table cannot change its output. They are equivalent mutants, not coverage gaps, and the entries stay because they document the answer.Three cells (
ExportAllDeclaration,ImportAttribute,Directive) hold no identifier a walker could report, so a misclassification there is invisible in an artifact. They are pinned by unit assertions onreferenceChildreninstead, which is why they are killed rather than surviving.Suite
browser-server-exports-strip.test.tsgoes from 182 to 202 steps and the newreference-classification.test.tsadds 22, all passing. Suite growth on its own proved nothing here: the 182-step suite passed with both 4A and 4B present, which is exactly why every new case is asserted through the realrunPipelineand every classification cell is mutation-tested.The positive cases stay pinned:
<Card />,<UI.Item />,<UI.Item.Card />,<Table />beside<table />, expression containers, spread attributes, both fragment spellings,<Card as={Bar} />, and<motion.ui.Panel />two levels deep.Eighth commit: every classification cell is pinned
Mutation testing the seventh commit found nine cells the table classified and no test asserted: the fixed key of an object method, a class method, a class property and an auto-accessor, the computed form of each of those, the comments and tokens a parsed file carries beside its program, every import specifier shape, the exported name of a default or namespace re-export, and the identifier-valued
nameof a placeholder.None of them was wrong. Each was a cell a later edit could have flipped without a test noticing, which is the failure mode the classification exists to end. This commit adds one case per cell: the method and class member keys through both walkers, since a fixed key counted as a read pins the import it collides with, and the rest on
referenceChildren, because they hold no identifier a walker could report. Implementation unchanged, tests only.Ninth commit: the predicates are tested against the compiler, not against a second opinion
The seventh and eighth commits close the node-type class. They do not close the class above it, and a review of this branch named the reason exactly: the guard pins the node-type TABLE, it does not pin the PREDICATES that approximate compiler rules. A table entry says "
JSXIdentifieris a read". It says nothing about the rule that decides whichJSXIdentifierin an element-name position is tag text, and that rule was wrong.The defect
isIntrinsicJsxNametested for an identifier with an ASCII-only regular expression:esbuild's rule is: the first character is ASCII
a-z, or the name is not a valid ECMAScript identifier, and its identifier grammar is full Unicode. So every JSX element name that IS a valid identifier and contains a non-ASCII character was classified as intrinsic tag text. Both walkers then skippedJSXOpeningElement.name, and the import or declaration the element named was deleted while the compiled artifact still called it.Measured through the real
runPipeline,ssr: false:import { Café } from "./ui/cafe.tsx", read by the hook and by<Café />jsx(Café, {})with no import ofCafé:ReferenceErroron renderconst Café = () => null, read by the hook and by<Café />jsx(Café, {})left danglingThe rule now delegates to one Unicode-aware identifier test built from
\p{ID_Start}and\p{ID_Continue}, with$and_spelled out because neither carries the Unicode property.The first clause stays deliberately ASCII. esbuild compares the first character against
a-z, soωmega,приветand日本語are binding reads even though a reader would call their first letter lowercase, and widening that clause to\p{Ll}would delete exactly those imports.The sibling, same predicate, same commit
pragmaRootBindinginbrowser-server-exports-strip.tscarried the identical ASCII-only expression. A@jsxRuntime classicmodule whose pragma names a non-ASCII factory root lost the pin, sodropUnusedImportBindingsdeleted the factory import while esbuild emittedĦ.créate("div", null). Both now shareisEcmaScriptIdentifier.The actual work: differential tests
Fixing two instances would leave the class exactly where the node-type class was before the seventh commit.
src/transforms/pipeline/stages/compiler-predicates.test.tscloses it: every predicate in this stage that stands in for a compiler or parser rule is now compared against the real compiler on a corpus, instead of against a second hand-written expectation of the same rule. A hand-written expectation restates the predicate and passes whether or not either one is right. Asking esbuild what it emitted fails when the two disagree, whoever moved.isIntrinsicJsxName<Name />with a factory this test owns, then read the first argument off the parsed artifact: aStringLiteralis tag text, anything else is a binding readisEcmaScriptIdentifierfunction <name>() {}, over every code point up toU+2FFFF, with esbuild settling the disagreements (see below)pragmaRootBinding,jsxPragmaBindingsroots ⊆ pinned ⊆ roots ∪ {React}declareshort-circuit, itsnodeHasDecoratorsexception, theimportKind/exportKindshort-circuits, and the wholeRUNTIME_TS_NODE_TYPES/ erased splitProbeand using it in one position; esbuild's own import elision answers "is this a value read?", and both walkers must give the same answercompilerNameHelperBindingskeepNameshelper shape__nameSOURCE_MAP_SUFFIXThe element-name corpus covers ASCII lowercase and uppercase, leading
_and$, digits, dashes,data-names, Latin-1 accents precomposed and decomposed, Greek, Cyrillic, CJK,ID_Startcode points outside the letter categories (ℕ,Ⅻ,℘), and both joiners in anID_Continueposition. Every one of the 36 names agrees with the artifact.Two families where the parser and the compiler do not agree are recorded rather than hidden, because both directions are fail-safe and neither is obvious:
U+1C8A, added in Unicode 14, parses and is rejected by esbuild's older tables. The predicate answers "binding read", which retains the import, and the build then fails loudly at compile. The over-deleting answer would have removed the import first.𝒞ardis a binding read for esbuild and is rejected by the parser, so the strip stage fails the build before the predicate is consulted.Three rules in this stage are deliberately not differentially tested, and the test header says so rather than substituting a hand-written expectation and calling it verified:
freeReferencedIdentifiersandpatternBoundNamesapproximate ECMAScript scoping. No compiler reports a scope resolution in its output: esbuild resolves scopes internally and the artifact never names the binding a reference resolved to, so there is no answer to compare against. They stay covered by behavioural fixtures.retainLeadingCommentsapproximates the parser's comment attachment. Its differential partner would be Babel, which is also the tool that produced the input, so the comparison would be circular.SERVER_ONLY_EXPORTSandisKnownDroppableSourceare framework policy. Nothing outside this repository owns the rule.Guard hardening
Two gaps in the seventh commit's guard, both closed here.
The parser was not pinned. The guard's oracle is
@babel/types@7.29.0. The nodes are emitted by@babel/parser@7.29.2, pinned separately in the same manifest, and that parser resolves its own@babel/types@7.29.7. The pins already drift. A parser-only bump would leave the guard green while a newTS-prefixed node type fell toDEFAULT_TS_REFERENCE_CLASS = "erased", which is the over-delete direction. The parser pin is now asserted too, so bumping either one fails the build and whoever bumps it re-runs the guard against the new package.The Flow filter rested on an unstated assumption.
parseableNodeTypesremoves the 65 Flow types from the guard's scope, which is sound only whilepickPluginsalways enablestypescript(Babel refuses to enableflowandtypescripttogether). That is now an assertion over every path the parser extension takes (.tsx,.ts,.jsx,.js,.mjs,.cjs,.md,.mdx, and no path at all): each must parse TypeScript-only syntax and refuse Flow-only syntax. A second case asserts the filter is not vacuous. Enabling Flow later fails here instead of silently uncovering 65 unclassified types.Mutation testing
Each fix and each new guard was reverted individually and the two suites re-run. 10 of 11 killed:
isIntrinsicJsxNameback to the ASCII-only regular expression<Café />,<Ab />and<Ab />pragmaRootBindingback to the ASCII-only regular expressiondeclareshort-circuitagrees with the artifact about an ambient class heritage clause, plus the existingignores declare forms and declared function signaturesnodeHasDecoratorsexceptionagrees with the artifact about a decorator on an ambient member, pluskeeps a decorator on a declared property, which still emits a runtime callimportKind/exportKindshort-circuitsagrees with the artifact about a type-only export statementandabout an inline type export specifier, plusignores type-only import and export specifiersTSEnumDeclarationfrom the runtime list to the erased oneagrees with the artifact about an enum initialiser, plus 5 existing enum and namespace cases@babel/parserpin in the extension manifestpins the @babel/parser the nodes are emitted byflowplugin inpickPluginsnever enables the Flow plugin the guard's filter depends onkeepNameshelper by binding name instead of by shaperecognises the keepNames helper in real minified output, plusprunes hook-only helpers from compiled keepNames outputdrops the sourcemap comment esbuild actually writes, plusremoves an external source map reference after strippingThe survivor is an equivalent mutant and is reported as one rather than papered over. V8's
\p{ID_Continue}already matches ZWNJ and ZWJ, so the two explicit escapes change no answer on this runtime. They stay because the ECMAScript grammar lists the joiners separately from the property, and the property is the engine's table rather than the specification's rule. The behaviour they exist for is pinned anyway: the code-point sweep asks the runtime about both joiners, andaccepts both joiners inside an identifier and neither at the startpins the four answers directly. A comment inreference-classification.tsrecords this so a reader does not mistake redundancy for coverage.The differential tests also have to be able to fail for the right reason, not only when a predicate is deleted.
disagrees with the artifact when the identifier test is ASCII-onlyre-runs the whole element-name corpus against the old ASCII predicate inside the test and asserts it fails, so the corpus itself is proven to discriminate.Tenth commit: the engines do not agree with themselves
The code-point sweep first asserted that the predicate agrees with the host runtime's own lexer. CI found that wrong within one push: it holds on V8 and fails on JavaScriptCore.
\p{ID_Start}and\p{ID_Continue}are the ENGINE's regular-expression tables. The engine's lexer is a different table, versioned independently, inside the same runtime. Bun on CI named about twenty code points where its two tables differ (U+088F,U+0897,U+0C5C,U+0CDC,U+1ACFthroughU+1ADB). A Bun of a different version on another machine names three entirely different ones (U+2FE1,U+AA51,U+E8C5). Deno names none.Runtime agreement was never the property that matters, which is the point the failure made. The predicate is asked about names the COMPILER will meet, so the compiler decides whether a table disagreement can reach an artifact. The sweep now collects the disagreements and asserts esbuild refuses every one of them: the module does not build, so nothing can depend on which table was right. If esbuild ever accepts such a name the test fails, because then the two answers differ on a name that ships and one of them deletes a live import.
Checked in both directions against the real compiler: all twenty code points CI's Bun named and all three the other Bun names are rejected by esbuild.
This is the differential principle applied to the differential test itself. The first version compared the predicate against a second opinion that happened to be handy; the second compares it against the thing that decides.
The pre-parse gate reads the grammar, not the spelling
This is the one regression the reorder introduced, and it is closed by
55cd7972and191234f9.stripServerOnlyExportsdecides whether to parse at all from raw source text. Onmainthat text is esbuild's output, where an escaped export name has already been normalized togetServerData, so the substring test finds it and the module is parsed. Run before compile, the gate sees the authored text with the escape intact:The raw text holds neither the substring
getServerDatanor a quote afteras, so the gate returned false, the module was never parsed,exportedHookBindingsnever ran, and the artifact shipped everything this stage exists to remove. Measured through the realrunPipeline,ssr: false, production output, ate41b94476d:The secret initialiser, the real hook body and the
veryfrontserver import are all there, and esbuild has normalized the name back togetServerData, so the runtime lookup ofmod.getServerDatastill finds it.Widening the gate one spelling at a time does not close it. Six shapes were measured, all clean on
mainat3a109046d0and five of them leaking ate41b94476d:main3a109046d0e41b94476d191234f9export { loadIt as get\u0053erverData }on.tsxexport async function get\u0053erverData()export { loadIt as get\u0053erverData }on.jsexport{loadIt as"get\u0053erverData"}, no space afterasasexport { loadIt as "getServerData" }The last leaking row matters for how the fix was chosen. It carries no
\uor\xescape at all, so an escape-specific regular expression closes the other four and leaves it open. The old quote clause required whitespace afteras, whichexport{loadIt as"…"}does not have, and a line continuation spells no hook name in raw text.So the gate now answers from the grammar instead. An exported name is an IdentifierName or a StringLiteral, and in both the only way to write a character as anything other than itself is an escape sequence, which always starts with a backslash. A module whose text holds no backslash therefore spells every name it exports verbatim, and the substring test settles it. Anything else parses, and
exportNamereads the name the way the runtime will.exportitself cannot hide either, because a reserved word written with an escape is a syntax error.The gate is only a performance optimisation, so the alternative worth measuring was to drop it and parse every module that contains
export. Overtemplates/files(60 modules, 54 KB, 20 runs averaged), whole tree: the old gate 1.05 ms, this gate 1.76 ms, parsing every exporting module 3.76 ms. Oversrc/transforms(282 modules, 2.5 MB, a pessimistic regex-heavy upper bound and not app-shaped code): 16.6 ms, 52.8 ms and 67.3 ms. Five of the 54 exporting modules in the app tree carry a backslash and now parse. Soundness costs 0.7 ms on a realistic tree and stays cheaper than parsing everything, so there is no reason to trade it away.The test that closes the class
compiler-predicates.test.tsexcusedSERVER_ONLY_EXPORTSas framework policy with no differential partner. That conflated the SET of hook names, which is policy and has no oracle, with the test for whether a module exports one, which is a parser rule esbuild answers exactly. Every fixture in that file read a predicate's verdict off the artifact. None asked the artifact which names the module exports.That case exists now. It compiles the module, reads the export names off the parsed artifact, and asserts the set of hook-named exports the pass emptied equals the set the artifact actually has. Set equality against the compiler fails for any spelling, so a sixth or seventh way to write the name needs no new fixture. Ten spellings drive it, and a guard case asserts the hidden ones really are hidden, so a
\uthe test file resolved at its own parse time cannot pass for the wrong reason.Five of the ten fail against the gate
55cd7972replaced.Still open, unchanged by this PR
An unrelated unused import in an authored JavaScript page. The erasure above is TypeScript import elision. Under the
jsandjsxloaders nothing elides, so an import the stripped hooks own nothing of survives and esbuild rewrites it into a side-effect import itself. Measured onmain, on30c64eeband on this branch, in dev and prod: identical in all of them, module kept. Contrast the shape8714b733closed, an import the hooks do own a binding of, which is now deleted outright on every loader. Pre-existing onmain, no regression here, and out of scope for the recorded decision.Destructuring shapes (nested object, array, rest, computed key and sibling default) leaked the secret identically before and after the reorder. #3861 closed the nested-object and array shapes on
main, and that fix is merged into this branch by51b31a52c4. Rest, computed key and sibling default still leak, identically onmainat3a109046d0, onmainat8d0ce3c2f0, and on this head. No regression in either direction, and out of scope here.The same predicate shape in two SSR stages, pre-existing on
main. Enumerating the compiler-approximating predicates in the browser strip turned up two more in the SSR pipeline, inssr-css-strip.ts(lines 101, 109, 135) andssr-http-stub.ts(lines 54, 59, 71). Each parses an import clause with the same ASCII-only identifier expression this commit replaced, and each falls through to a comment when the expression does not match. Measured onmainand on this branch, identically:SSR artifact, both trees:
/* css import: /project/pages/Button.module.css */;followed byreturn styl\u00E9s.container;. The proxy stub is never declared, so SSR throws aReferenceError. Those stages run aftercompilePlugin, which escapes a non-ASCII identifier in its output, so the clause text the regular expression sees carries a backslash and cannot match the ASCII class either way.This is the same class and a different stage, on the SSR target rather than the browser one, and it is untouched by this PR in either direction. Fixing it means moving those two stages off regular-expression rewriting of import clauses, which is its own change with its own tests. Recorded here rather than folded in.
Re-measured against current
mainmainhas moved 41 commits since the tables above were taken, from3a109046d0to8d0ce3c2f0, andthose 41 commits are merged in here.
That merge fixes a gate this branch was failing.
deno task typecheckexited 1 at its first step,generate:manifests:check, with./templates/manifest.generated.ts is stale, while the same command exited0 on plain
origin/mainat8d0ce3c2f0in a fresh tree. The earlier note in Verification calling thatfailure pre-existing was true against
mainatc4e93cd8and is no longer true:mainregenerated themanifest in the meantime. This branch never edits that file, so it still carried the merge-base bytes. The
two versions decompress to byte-identical content (990887 characters, same 48 templates and 467 files), so
only the committed gzip encoding was behind, and
generate-templates-manifest.ts --checkcompares theencoded string. Merging
maintakes its copy and the check passes. Nothing was hand-regenerated. That merge is1f1d0c41b4,and the head is now
1f1d0c41b4. The effective diff is unchanged by it: 9 files, +3543/-420 against the newmerge base.
Every
claim that compares the two trees was re-run at the new SHA, through the real
runPipeline({ ssr: false })with production output, on
main8d0ce3c2f0and on this head191234f9. Each artifact was grepped for thesecret, the hook body and the
veryfrontimport.main8d0ce3c2f0export { loadIt as get\u0053erverData }on.tsxexport async function get\u0053erverData()export { loadIt as get\u0053erverData }on.jsexport{loadIt as"get\u0053erverData"}, no space afterasexport { loadIt as "getServerData" }export * from "./lib/server-only-lib.ts"No regression in either direction against current
main. The three destructuring shapes and theexport *shape are pre-existing and out of scope, as Still open records.
Also re-run at the current head:
git merge-tree --write-tree origin/main HEADwithmainat8d0ce3c2f0: exit 0, zero conflict entries.browser-server-exports-strip.ts:158to the forme41b94476dshipped makes five ofthe six escaped spellings leak the secret, the hook body and the
veryfrontimport through the realpipeline, and turns exactly those five steps of
compiler-predicates.test.tsred. Restoring it makes allsix clean again. The gate is load-bearing, and the differential test kills the defect and nothing else.
&& !pinned.has(name)atbrowser-server-exports-strip.ts:1264and runningsrc/transforms/pipeline/stages/fails onkeeps partially hook-owned destructuring and pinned JSX factory names. That mutant stays dead.grep -n $'[\u2014\u2013]'over all nine touched files finds 14 dash characters, all ofthem present unchanged at the merge base and none on a line this PR adds.
Interaction with #3846
Re-checked against current
main. Both PRs touchbrowser-server-exports-strip.ts, and there is no semanticconflict, but the textual one decides the order.
git merge-tree --write-tree origin/main HEAD: exit 0, clean.git merge-tree --write-tree origin/main pr-3846: exit 1, conflicts inbrowser-server-exports-strip.ts,browser-server-exports-strip.test.tsandtemplates/manifest.generated.ts.git merge-tree --write-tree HEAD pr-3846: exit 1, the same three plusdocs/guides/data-fetching.md.#3846 is +9165/-584 against a merge base of
c4e93cd8, whichmainleft far behind. It does not merge withmainas it stands. This branch does. Land this one first and rebase #3846 onto it.Refs veryfront/veryfront-issue-inbox#605. Supersedes the reference to closed veryfront/veryfront-issue-inbox#112.