Skip to content

dogfood: a parenthesised type annotation reads as a top-level call in elision #1423

Description

@vivek7405

Problem

A parenthesised TYPE ANNOTATION on a top-level const is read by the elision analyser as a top-level call, so the module is classified as doing client work. Type annotations are erased before anything runs, so this is a false positive on syntax that cannot have a runtime effect.

Minimal repro, one line apart:

// modules/x/utils/lines.ts
export const A: readonly (number[])[] = [[1]];   // every importing route module -> ships whole
export const B: readonly number[][] = [[1]];     // every importing route module -> inert

Found while dogfooding a tic-tac-toe app. webjs elision reported all three page routes as shipped, blocked by a utils module containing nothing but export type, export interface, two export const data literals, and pure functions:

shipped  app/page.ts  blocked by modules/tic-tac-toe/utils/game.ts, which references a browser
                      global at module scope, runs code at module scope, or has a bare
                      side-effect import

The offending declaration was export const LINES: readonly (readonly [number, number, number])[] = [...], the eight winning triples of a tic-tac-toe board. Rewriting the same data as as const (which types it MORE precisely) returned all three routes to inert / import-only with no other edit.

The cost is silent. Nothing warns, the app behaves identically, and the only symptom is bytes: an affected module downgrades every route module that imports it from inert to shipping whole. A COMPONENT carrying such an annotation is worse, since it is forced to ship and can never elide, and by the "rendered by a component that ships" rule it takes its display-only children with it.

Design / approach

The scan is a lexical heuristic over source that has had strings and template holes redacted, but NOT type annotations. In hasModuleScopeSideEffect (packages/server/src/component-elision.js), depth-0 text is matched against:

const CALL_RE = /(?:([A-Za-z_$][\w$]*)|[)\]])\s*\(/g;   // L374

readonly ( is an identifier immediately followed by (, and readonly is absent from the NOT_A_CALL keyword set at L377 (if, for, while, typeof, function, ...), so the loop at L383 falls through to return true and the module is marked as running code at module scope.

Two candidate fixes, and they are not equivalent:

  1. Add readonly to NOT_A_CALL. One line, kills the common case (readonly (T)[] is the annotation people actually write). It does NOT kill the class: as (readonly T[]) and any other identifier-before-paren inside an annotation still trip it.
  2. Strip type annotations before the depth-0 scan. Correct, and it removes the whole class. redactToPlaceholders (packages/server/src/js-scan.js L721) is the existing precedent for blanking syntax the scan must not read, added for the string/template false positives in fix: false positives in component scanner and elision analyzer due to string/template literals in code samples #634; annotation redaction is the same move for the same reason.

My read is that 1 is the safe immediate fix and 2 is the real one, and that they can ship together (the keyword entry is harmless once annotations are stripped). Whoever picks this up should decide whether annotation-stripping is tractable lexically or wants the TS-strip output the framework already produces.

This is the third of a family: #623 (# alias, inline-script globals, new Set) and #634 (string / template literals in code samples) were the same shape of false positive in the same scan. Worth a look at those fixes for the pattern they established.

Implementation notes (for the implementing agent)

Where to edit.

  • packages/server/src/component-elision.js, hasModuleScopeSideEffect at L321. CALL_RE is L374, NOT_A_CALL is L377, the match loop is L383 to L406. Note the loop already carries several targeted exemptions (function name(, register / define, extends WebComponent(, new X(), so an added exemption fits the existing shape.
  • packages/server/src/js-scan.js, redactToPlaceholders at L721, if fix 2 is taken.

Landmines.

  • The scan reads TypeScript, not stripped JS. That is the root of this bug and the thing to keep in mind for a fix: any heuristic here meets readonly, satisfies, as, generics, and parameter properties, none of which exist at runtime.
  • Over-detection is deliberately safe here, under-detection is not. The file says so repeatedly (an unterminated string, unbalanced braces, and an unresolvable construct all return true). So a fix must not widen the exemption set in a way that lets REAL top-level work through. A call to a genuine function named readonly is legal JS; if that matters, scope the exemption to a type position rather than the bare identifier.
  • The verdict has no user-visible symptom. There is no error and no warning, so a regression here is invisible outside webjs elision output. That is an argument for a differential test (below) over an assertion on the reason string alone.
  • Bun parity. The analyser runs in the request path on both runtimes, so the test/bun/elision-report.mjs matrix covers it; check whether the new case needs a row there.

Invariants to respect.

  • Elision must not change served bytes. webjs elision --verify renders every static route with elision on and off and diffs the output; run it.
  • A conservative verdict (ship when unsure) stays the fallback for anything the scan cannot resolve.

Tests + docs surfaces.

  • packages/server/test/elision/analyze.test.js is where the hasModuleScopeSideEffect cases live, including the factory-form exemption from dogfood: factory-form extends WebComponent({...}) defeats display-only elision #604. Add the annotation cases beside them.
  • packages/server/test/elision/residual-contract.test.js L230 already reasons about what this function exempts and is the natural home for the route-module consequence (an importing page stays inert).
  • Counterfactual: revert the fix and prove the new test reds, per the Definition of done.
  • Docs: references/components.md documents the elidability blocker list ("code that runs at module load (a top-level call, non-data new, dynamic import(...), top-level await)"). If the fix changes what counts, that list needs to agree. No user-facing docs-site change is expected for a pure false-positive fix.

Acceptance criteria

  • export const A: readonly (number[])[] = [[1]] no longer marks its module as running code at module scope
  • A page importing only such a module is inert, not shipped, in the webjs elision report
  • A component whose only module-scope construct is a parenthesised annotation is elided
  • A REAL top-level call, new, dynamic import(), and top-level await still ship (the conservative direction is preserved)
  • A counterfactual proves the new test fires (revert the fix, test reds)
  • webjs elision --verify passes, so served bytes are unchanged
  • The decision between the narrow keyword fix and annotation-stripping is recorded on the PR, with the reason

Metadata

Metadata

Assignees

Labels

bugSomething isn't working

Type

No type

Projects

Status
Done

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions