fix: a parenthesised type annotation no longer reads as a top-level call - #1424
Conversation
A parenthesised type annotation on a top-level const read as a top-level call, so a module holding nothing but typed data was classified as running code at module scope. `readonly (readonly [number, number, number])[]` is an identifier immediately followed by `(`, and the module-scope scan matches exactly that shape. The cost was silent: no error, no warning, identical behaviour, and every route module importing such a helper downgraded from inert to shipping whole, with a component carrying one forced to ship along with the display-only children it renders. The analyser now erases type syntax with the framework's own stripper before it scans, at the single point where it reads a file, so the module-scope, template, import, and component scans all read the code that actually runs. Using the stripper rather than a lexical annotation matcher keeps the analyser and the runtime in agreement by construction, and it is position-preserving, so every offset the other scans depend on is unchanged. Non-TypeScript files and a source the stripper rejects are scanned as authored, which is the previous behaviour and therefore the conservative direction.
The module-scope case asserted the page's shipped verdict while the page reached the util through a component, so a wrong verdict on the util was laundered into the component's own ship and the page came back import-only either way. Having the page import the util directly puts the util in the page's whole client closure, which is the shape the report hit and the one that fails when the erasure is reverted.
The elision analysis now goes through the TypeScript stripper, which is the one seam that differs by runtime (Node's built-in module.stripTypeScriptTypes against amaro). A drift there would silently change what a Bun-served app downloads, so the cross-runtime fixture gains a .ts route whose util carries the parenthesised annotation. Its header claimed the analysis used no runtime-specific API, which is no longer true. Docs follow the same fact: the elidability blocker list in the skill and on the docs site now says a type annotation can never be a blocker, and the server package notes where the erasure happens and what a direct caller of the scan functions is still on the hook for.
|
Design rationale: why the stripper, where it runs, and what it costs The issue offered a narrow fix (add The keyword entry is not merely incomplete, it points the wrong way. For the erasure itself I used the framework's own stripper rather than a lexical annotation matcher. A matcher would have to get generics, It runs at The cost is real and small. Over |
…tion The type-erasure helper landed between computeElidableComponents' JSDoc and the function it documents, which orphaned the block. Moved the helper above it. No behaviour change.
vivek7405
left a comment
There was a problem hiding this comment.
Read the whole diff fresh. The change itself holds up: erasing types at analyzeElision's single file read is the right seam, position-preserving erasure means the redaction lexer and every brace-depth offset below it are untouched, and the strip-failure fallback lands on the over-ship side on every scan it feeds. I went looking for the way this could quietly go the WRONG direction, a signal that erasure makes invisible, and did not find one: CALL_RE and the lifecycle and prop() matchers all tolerate whitespace before (, so blanking <Options> out of createConfig<Options>(...) still leaves a detected call, and strip-only does no type-directed import elision, so a value import used only as a type is not dropped.
One thing to fix, and it is about the EXTENSION SET rather than the erasure. Inline.
The regex admitted .cts, .tsx, .mtsx and .ctsx, and the AGENTS.md sentence described a third set again (.ts / .mts / .tsx), so the code, the docs, and the framework all disagreed. WebJs treats exactly .ts and .mts as TypeScript: the MIME map and stripTs, the servable-extension test, the graph walker's file filter, and the router's name convention. There is no .cts and no JSX in any of them. The extra breadth was unreachable rather than harmful, since the graph walker and the router decide which files reach the analysis, but it read as support the framework does not have. The .tsx half of the doc claim was also backwards: real JSX is a parse error for a non-JSX TypeScript parse, so the catch would swallow it and the file would be scanned as authored, which is the opposite of what the sentence promised.
vivek7405
left a comment
There was a problem hiding this comment.
Scoped this round to f400f865 and traced what it touches outward. The extension narrowing does not hold up: it was made on a claim about which files reach the analysis that turns out to be false, and it reinstates the bug this PR exists to fix on one file class. Details inline.
One more, path-level because it is outside the diff. .agents/skills/webjs/references/runtime.md:23 says WebJs "serves .ts / .tsx as ES modules by erasing the types in place". It does not: dev/serve.js:259 serves js|mjs|ts|mts. Pre-existing, in a file this PR does not touch, and it ships into every scaffolded app through scripts/sync-scaffold-skill.mjs, so it is worth a decision rather than a silent pass.
The previous commit narrowed the erasure set to the SERVABLE extensions on the reasoning that the graph walker and the router decide which files reach the analysis. They do not. analyzeElision seeds allFiles from the COMPONENT set first, and scanComponents admits /\.m?[jt]sx?$/, which is wider than either. So a .tsx component arrives whatever those two admit, and narrowing put the #1423 verdict back for exactly the file class no other filter would have let through. The set is now every TypeScript-carrying extension in the union of the three seeding filters, and the comment says to derive it that way rather than from what the server serves. .cts stays out because no filter admits it. A .tsx holding real JSX still falls back to being scanned as authored, since the stripper parses as non-JSX TypeScript, and the fall-through paragraph now says so instead of naming a .js / .mjs set that was not the real one. Also reattaches hasModuleScopeSideEffect's JSDoc, which PURE_DATA_CONSTRUCTORS sat between. The direct-caller contract AGENTS.md sends readers to was in that block and reached neither hover nor param inference.
|
Deferred: Recording this so it is not lost with the round it came from.
Deferred rather than folded in because it is a pre-existing error in a file this PR does not touch, and the standing rule is that out-of-scope findings go to you rather than into the diff. Worth a decision either way: that file is copied into the scaffold at prepack by It is a one-line fix. Say the word and it goes in here, or it can be its own issue. |
The type-stripper seam listed `.ts` / `.tsx` as what the framework serves. It serves `.ts` / `.mts`: that is the MIME map, the servable-extension test, the graph walker's filter, and the router's name convention, and there is no JSX anywhere in the framework. Found while deriving the erasure set for #1423, so it is the same fact that change rests on. This file is copied into the scaffold at prepack, so the claim shipped to every generated app, where an agent following it would name a component .tsx and get a file the server will not serve.
It named the docs site as website/app/docs/<topic>/page.tsx, and the step-5 note said a .tsx doc page. Every one of the 45 doc pages is page.ts and there are no .tsx files, so an agent following the skill would grep for a path that does not exist. Same class as the runtime.md claim in the previous commit, and found by the same sweep.
vivek7405
left a comment
There was a problem hiding this comment.
Scoped to the three commits nobody had read. Three things, and the first two are mine from ca3d360d rather than anything older.
Three things the delta round surfaced. The per-package AGENTS.md still said the erasure covers .ts / .mts, which is the narrow set ca3d360 deliberately rejected. It now states the union rule the code follows, so the one doc surface asserting an extension set agrees with the regex. A missing stripper BACKEND was swallowed by the same catch as a per-file syntax rejection. Those are not the same failure: no backend means no TypeScript in the app is erased, so every module is scanned as authored and the #1423 verdict returns app-wide. ensureStripper had exactly one non-test caller, on the dev request path, so webjs check and webjs elision never resolved it and would degrade in silence. The backend is now resolved once per run and warns once when it cannot be, while a per-file failure stays silent because it is one file's problem. The erasure also ran on every file on every call, and analyzeElision is on the dev rebuild path. Measured on website, 179 files read in 2.0ms and stripped in 94ms, so it cost about 50x the read it was added beside on every save. Results are now memoized per file and validated against the exact source, which a same-mtime rewrite cannot defeat the way an mtime key could. Steady-state re-analysis drops from about 270ms to about 124ms.
resolveScanStripper's JSDoc had a let declaration between it and the function, which is the same detachment the previous commit fixed for hasModuleScopeSideEffect, reintroduced two hunks later. Moved the flag above the block and gave it its own line of doc. STRIP_CACHE moves above its only reader too, matching how every other constant in this file is ordered. No behaviour change.
Closes #1423
A parenthesised TypeScript type annotation on a top-level
constread as a top-level call, so a module holding nothing but typed data was classified as running code at module scope.readonly (readonly [number, number, number])[]is an identifier immediately followed by(, which is exactly the shapehasModuleScopeSideEffect's call matcher looks for. The cost was silent: no error, no warning, identical behaviour, and every route module importing such a helper downgraded from inert to shipping whole, with a component carrying one forced to ship along with the display-only children it renders.What changed
The analyser now erases type syntax before it scans, at the single point where it reads a file (
analyzeElision's loop), so the module-scope, template, import, and component scans all read the code that actually runs. A non-TypeScript file, and a source the stripper rejects, are scanned as authored, which is the previous behaviour and therefore the conservative direction.Why the stripper and not the keyword
The issue offered two fixes. Adding
readonlytoNOT_A_CALLis one line and kills the common case, but it does not kill the class (as (readonly T[])and every other identifier-before-paren in a type position still trip it), and it trades a false positive for a false NEGATIVE, sincereadonlyis a legal function name and a real top-levelreadonly(x)call would then be missed. That is the one direction this analyser may not take. Erasing types removes the class outright and leaves a genuine call visible, so the keyword entry buys nothing once the erasure is in and is not included.Erasure uses the framework's own stripper rather than a lexical annotation matcher. It is the same erasure the browser is served, so the analyser and the runtime agree by construction, where a hand-rolled matcher would meet generics,
as,satisfies, and conditional types and get some of them wrong. It is position-preserving whitespace replacement, so every offset the other scans depend on is unchanged.Which extensions get erased, and why it is not the servable set
This is the part that took two passes to get right, so it is worth stating plainly. The erasure set is the union of the TypeScript-carrying extensions in the three filters that seed
analyzeElision'sallFiles, NOT the set the server serves.allFilesstarts from the COMPONENT set, andscanComponentsadmits/\.m?[jt]sx?$/, which is wider than both the module-graph walker's/\.(js|ts|mjs|mts)$/and the router'sjs|mjs|ts|mts. So a.tsxcomponent reaches the analysis whatever those two admit. Narrowing the erasure to the servable.ts/.mtstherefore reinstates the #1423 verdict for exactly the file class no other filter would have let through, and it is the class with no other coverage. Hence/\.m?tsx?$/..ctsstays out because no filter admits it. A.tsxholding real JSX falls back to being scanned as authored, since the stripper parses as non-JSX TypeScript and rejects it.Test plan
packages/server/test/elision/type-annotations.test.js, eight cases driving the real pipeline over a real app on disk. Four are the acceptance criteria (including the.tsxcase above); four are the conservative-direction counterweight (a real call, a non-datanew, a dynamicimport(), a top-levelawait, a browser global, a genuine call to a function namedreadonly, and a module the stripper rejects). Full Node suite: 4468 tests, 0 failures..tsxcase independently reds under a servable-set-only regex and passes under the union one, which is what makes the extension reasoning above testable rather than asserted.WEBJS_E2E=1 node --test test/e2e/e2e.test.mjsgreen.node scripts/run-bun-tests.jsgreen, andtest/bun/elision-report.mjsgains a.tsroute carrying the annotation, run under bothnodeandbun. This is the row that matters, since the erasure goes through the one seam that differs by runtime (Node's built-in stripper against amaro).webjs elision --jsonbyte-identical tomainongallery,examples/blog, andwebsite, compared on an identical working tree.webjs elision --verify:examples/blog26 routes identical,website54 routes identical.galleryreports one divergence on/features/caching, which is that demo page rendering the current clock (22:10:33against22:10:39) rather than an elision difference. It reproduces identically on unmodifiedorigin/main, so it is pre-existing and not from this PR.webjs checkclean on all three apps;webjs doctor11 passed, 2 warnings, 0 failed on each, matchingmain.websiteboots in dist mode and serves 200 on/,/docs/elision,/docs/components,/ui,/ui/buttonwith no broken modulepreload hints;gallerythe same on/and/features/caching;examples/blogcovered by the e2e run.Docs surfaces
.agents/skills/webjs/references/components.md(the elidability blocker list),website/app/docs/elision/page.ts(the same list on the docs site),packages/server/AGENTS.md(where the erasure happens and what a direct caller of the scan functions is still on the hook for).webjs createoutput is unchanged and the CLI copies the skill from the repo root at prepack (scripts/sync-scaffold-skill.mjs), so the one edited copy is what ships. MCP, since no tool projection changed. Editor plugins, marketing copy, and README, since nothing they describe moved. Changelog, since there is no version bump here..agents/skills/webjs/references/runtime.md:23claims WebJs serves.ts/.tsx. The.tsxhalf is false. Pre-existing, in a file this PR does not touch. Recorded in a comment below.