refactor(app-router): fence AppElements wire-key construction#1088
refactor(app-router): fence AppElements wire-key construction#1088james-elicx merged 2 commits intocloudflare:mainfrom
Conversation
AppElements transport keys could still be constructed or recognized through scattered raw string prefixes outside the wire codec. That kept the flat payload format acting as an informal semantic API and left legacy compatibility behavior covered only by indirect route tests. Make AppElementsWire the exported constructor and parser boundary for route, page, layout, template, slot, cache, metadata, and unmatched-slot wire values. Route graph semantic ID minting stays separate, while render wiring, layout classification, browser slot merging, and prefetch cache tests now consume the codec boundary. Tests cover canonical key construction and parsing, legacy metadata read/write behavior, unmatched-slot marker compatibility, and a source-boundary check that rejects new raw AppElements wire-key construction outside the codec and route graph.
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good boundary-fencing work. The codec ownership is clear: constructors and parsers live in app-elements-wire.ts, the route graph keeps its own semantic ID minting, and the source-boundary test enforces the invariant. A few things to consider:
-
isSlotIdperformance: The current implementation delegates to the fullparseAppElementsWireElementKeyparser, which tries five prefix branches and allocates intermediate strings/objects for every call.isSlotIdis called per-key innormalizeAppElements(two loops),mergeElements(two loops), andgetMountedSlotIds— all hot per-navigation paths. See inline comment for a cheaper alternative. Not blocking, but worth a follow-up if profiling shows it matters. -
Boundary test is a strong addition. The filesystem-walking test that rejects raw wire-key construction outside the allowed set is a good structural invariant. One gap noted inline — the regex doesn't catch string concatenation patterns.
-
Stale comment in
app-elements.ts:24: The comment sayscreateAppPayloadPageId stays privatebut this PR also madecreateAppPayloadCacheKeyandcreateAppPayloadRouteIdprivate. Consider updating it to mention all three, or dropping it since theAppElementsWireCodectype signature already documents the public surface.
Overall the change is clean and well-scoped. No correctness issues found.
| } | ||
|
|
||
| function isAppElementsWireSlotId(key: string): boolean { | ||
| return parseAppElementsWireElementKey(key)?.kind === "slot"; |
There was a problem hiding this comment.
This full-parses the key through all five prefix branches + allocates a result object just to check kind === "slot". Every call to isSlotId pays this cost, and it's called per-key in normalizeAppElements (×2 loops), mergeElements (×2 loops), and getMountedSlotIds.
A cheaper alternative that stays inside the codec boundary:
| return parseAppElementsWireElementKey(key)?.kind === "slot"; | |
| function isAppElementsWireSlotId(key: string): boolean { | |
| if (!key.startsWith("slot:")) return false; | |
| const body = key.slice("slot:".length); | |
| const sep = body.indexOf(":"); | |
| return sep > 0 && body.charCodeAt(sep + 1) === 0x2f; // next char is '/' | |
| } |
This validates the same structural invariant (slot:<name>:/<treePath>) without allocating. Not blocking — just a perf consideration if this becomes measurably hot.
| } | ||
|
|
||
| function parseTreePath(input: string): string | null { | ||
| return input.startsWith("/") ? input : null; |
There was a problem hiding this comment.
Nit: parseTreePath requires a leading /, which means bare segment names like blog are rejected. This is correct for the current wire format where tree paths are always absolute (/, /blog, /(marketing)/blog/[slug]), but it's an implicit contract. A brief doc-comment on this function would help future contributors understand the invariant.
| path.join(sourceRoot, "server/app-elements-wire.ts"), | ||
| ]); | ||
| const rawWireConstruction = | ||
| /`(?:route|page|layout|template):\$\{|`slot:\$\{|\.startsWith\("(?:slot|layout|page|route|template):"\)/; |
There was a problem hiding this comment.
The regex catches template-literal construction and .startsWith() recognition, but wouldn't catch string concatenation patterns like "slot:" + name + ":" + path. That's probably fine for the current codebase since nobody uses concat, but worth noting in a comment near the regex documenting its intended scope — a future contributor might reach for concat and bypass the fence.
|
Review posted successfully. Here's a summary of what I flagged: 3 inline comments:
1 top-level note:
No correctness issues found — the change is clean and well-scoped. |
Make slot-key recognition a cheap structural predicate, document the absolute tree-path parser contract, and broaden the boundary coverage to catch raw wire-key concatenation. Also route layout classification ids through AppElementsWire so raw layout-key construction stays fenced inside the codec boundary.
|
/bigbonk review |
There was a problem hiding this comment.
All prior review items addressed cleanly in the second commit. Tests pass (all 204 across 9 affected files). The boundary is well-drawn and the source-fence test is a strong structural invariant.
Verified:
isAppElementsWireSlotIduses the cheapstartsWith+charCodeAtpredicate (no allocation)parseTreePathhas a doc-comment explaining the absolute-path contract- Boundary test regex now catches concatenation patterns, with self-test assertions proving coverage
- Stale comment updated to "Raw constructor helpers stay private"
app-page-dispatch.tslayout ID construction fenced throughAppElementsWire.encodeLayoutId- Route graph functions intentionally kept separate from wire codec (graph-level semantic IDs, not transport) —
allowedset in boundary test correctly includes both files AppElementsWireElementKeytype is module-private but inferrable from the codec return type — fine for now, future callers can narrow via TypeScript inference- No raw wire-key construction remains outside the two allowed files
One minor observation (non-blocking):
The route graph functions (createAppRouteGraphRouteId, createAppRouteGraphSlotId, etc. in routing/app-route-graph.ts) produce byte-identical strings to the wire codec constructors. The separation is conceptually correct — graph IDs are route-topology facts, wire IDs are transport keys — but if the wire format ever changes, these would need to change in lockstep. The boundary test catches construction outside the allowed set, but doesn't enforce that the two sets produce the same format. A future follow-up could add a shared format assertion (e.g., a test verifying createAppRouteGraphSlotId(name, path) === AppElementsWire.encodeSlotId(name, path)) to prevent silent drift. Low priority since any divergence would immediately break routing.
|
Review submitted successfully. Here's a summary: Approved — the PR cleanly addresses all prior review feedback and the code is correct. What I verified:
One non-blocking observation: The route graph and wire codec produce byte-identical strings from separate functions. A future cross-format assertion test could guard against silent drift if the wire format ever changes. |
…ound-trip expectation readAppElementsMetadata always returns an artifactCompatibility envelope after cloudflare#1062, but the round-trip test added in cloudflare#1088 still asserted the pre-cloudflare#1062 shape, breaking once both landed on main.
What this changes
Implements task
#726-WIRE-02/03from issue #726 by moving AppElements wire-key construction and parsing behindAppElementsWire.The codec now owns constructors and parsers for route, page, layout, template, slot, cache, metadata, and unmatched-slot wire values. Render wiring, layout classification, browser slot merging, mounted-slot header construction, and prefetch cache tests now call the codec instead of assembling or recognizing raw
layout:,page:,template:, orslot:keys directly.Why
Issue #726 calls out that the flat AppElements payload should stay as the transport, but raw wire keys must stop becoming router authority. Before this change, several code paths still constructed or interpreted wire keys with string templates or prefix checks outside the wire boundary, which made it easy for future router work to rebuild semantics around incidental payload strings.
Correctness oracle: Vinext internal invariant from #726. The wire format remains byte-compatible for existing payload keys and legacy metadata behavior, but ownership moves to
AppElementsWire.Approach
AppElementsWireconstructors for layout, template, and slot element IDs, plus an element-key parser and slot-key predicate.server/app-elements-wire.tsandrouting/app-route-graph.ts.Non-goals:
Bonk: please read issue #726 before reviewing this PR so the big-picture AppElementsWire boundary and router-spine migration context is visible.
Validation
vp test run tests/app-elements.test.ts tests/prefetch-cache.test.ts tests/app-page-element-builder.test.ts tests/slot.test.ts tests/app-page-route-wiring.test.ts tests/layout-classification.test.ts tests/app-browser-entry.test.ts tests/app-page-render.test.ts tests/app-page-execution.test.tsvp checkexits 0. It still reports the existing unrelatedtypescript-eslint(no-redundant-type-constituents)warning inpackages/vinext/src/server/request-pipeline.ts:604.vp check --fixandknip --no-progresssuccessfully.Risks / follow-ups
The parser deliberately recognizes the existing legacy wire strings and rejects malformed keys as non-wire entries. Future #726 work can build stronger semantic IDs and planner decisions on top of this without relying on missing payload entries or raw key prefixes as proof.