refactor(app-router): introduce AppElementsWire boundary#1059
Conversation
App Router payload ownership currently leaks through raw helper calls for route ids, cache keys, metadata writes, and decode paths. That makes the flat AppElements transport act like router semantics instead of a narrow wire boundary, which violates the cloudflare#726-WIRE-01 ownership target. Introduce AppElementsWire as the single codec facade for AppElements wire encoding, decoding, metadata creation, and outgoing payload shaping. Route the current production encode/decode/write call sites through that facade while keeping the legacy helper names re-exported only where existing tests cover compatibility.
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.
Review: refactor(app-router): introduce AppElementsWire boundary
Clean extraction. The move from scattered key-constant + helper-function imports to a single AppElementsWire codec object is a net readability win — call sites like AppElementsWire.encodeRouteId(...) are self-documenting compared to the previous createAppPayloadRouteId(...) + raw APP_ROUTE_KEY assignments. The barrel re-export strategy in app-elements.ts is the right call for a non-breaking migration.
A few things worth addressing before merge:
-
AppElementsWire.keysis dead code in this PR — it's defined and typed but has zero consumers. If it's speculative API surface for WIRE-02+, that's fine, but worth a comment saying so. Otherwise it should be dropped to keep the public surface minimal. -
isLayoutFlagsRecordaccepts{}as valid LayoutFlags — this is correct behavior (empty flags = no layouts probed), but it also means any empty object passes the predicate. That's inherited from before this PR, just noting it since the function moved to a new codec boundary where the contract is being tightened. -
Minor: the barrel in
app-elements.tsre-exportscreateAppPayloadRouteIdandcreateAppPayloadCacheKeyas named exports for backward compat, butcreateAppPayloadPageIdwas dropped. That's intentional (no external callers), but worth a brief inline comment in the barrel to document the deliberate omission, since it's the only one of the threecreate*helpers that got fully privatized.
Otherwise the migration is mechanically correct — all 16 internal consumers import through the barrel, the generated entry template correctly uses __AppElementsWire, and the new tests cover the codec round-trip well.
| readonly layoutFlags: typeof APP_LAYOUT_FLAGS_KEY; | ||
| readonly rootLayout: typeof APP_ROOT_LAYOUT_KEY; | ||
| readonly route: typeof APP_ROUTE_KEY; | ||
| }; |
There was a problem hiding this comment.
keys is typed and populated but has zero consumers in this PR or anywhere else in the codebase. Is this speculative surface for WIRE-02? If so, a brief comment would help. If not, consider dropping it — smaller public surface = fewer things to maintain.
| } | ||
|
|
||
| function isLayoutFlagsRecord(value: unknown): value is LayoutFlags { | ||
| if (!value || typeof value !== "object" || Array.isArray(value)) return false; |
There was a problem hiding this comment.
Nit: !value rejects 0, "", false (all non-objects anyway) but also null — which is already handled by typeof value !== "object" being falsy for null. The !value guard is technically redundant for the null case since the typeof check would need to come first to exclude null. As written, the short-circuit order is fine because !null is true and bails before the typeof check, so it works correctly — but it reads like the null guard is the typeof check rather than !value.
No action needed, just noting the inherited subtlety for when this boundary gets revisited.
| type AppOutgoingElements, | ||
| type AppWireElements, | ||
| type LayoutFlags, | ||
| } from "./app-elements-wire.js"; |
There was a problem hiding this comment.
The barrel re-exports createAppPayloadRouteId and createAppPayloadCacheKey for backward compat, but createAppPayloadPageId was intentionally dropped (no external callers). Consider adding a brief comment here documenting that omission, so future readers don't wonder whether it was accidental:
| } from "./app-elements-wire.js"; | |
| } from "./app-elements-wire.js"; | |
| // NOTE: createAppPayloadPageId is intentionally not re-exported — it is only | |
| // used internally through AppElementsWire.encodePageId. |
| rootLayoutTreePath, | ||
| routeId, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Using Object.assign to merge metadata entries into a mutable elements record works, but it's a different pattern from the other call sites (boundary render, element builder, RSC entry) which all use object spread (...AppElementsWire.createMetadataEntries(...)).
This is fine — elements is already mutable here — but the inconsistency stands out. Not blocking, just flagging for awareness.
|
Review submitted. Here's a summary of the feedback: Overall: Clean extraction — the Key points raised:
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: refactor(app-router): introduce AppElementsWire boundary
This is a well-executed extraction. The codec object pattern (AppElementsWire.encodeRouteId(...), AppElementsWire.decode(...)) is a clear improvement over the previous scattered imports of bare key constants and helper functions. The migration is mechanically correct — all 16 internal consumers import through the barrel, no direct app-elements-wire.js imports leak outside the barrel, and the generated entry template correctly uses the __AppElementsWire alias.
Things I verified:
- No behavioral change in
isLayoutFlagsRecord: The guard changed from!value || typeof value !== "object"totypeof value !== "object" || value === null. These are equivalent —!valuerejectsnull,undefined,0,"",false,NaN, andtypeof null === "object"means the explicitnullcheck is needed after thetypeofgate. The new version is actually clearer. - No stale references:
normalizeAppElementsPromisewas fully renamed todecodeAppElementsPromise(zero remaining refs).createAppPayloadPageIdis correctly privatized with zero external callers. - Barrel completeness: All production-facing symbols are re-exported from
app-elements.ts. No file outsideapp-elements.tsimports directly fromapp-elements-wire.ts. - No raw key string usage in production code:
"__route","__interceptionContext", etc. only appear as constant definitions insideapp-elements-wire.ts. - Tests pass: All 31 tests in
app-elements.test.tspass, including the 3 newAppElementsWirecodec round-trip tests.
Two small notes below — neither is blocking.
| readonly layoutFlags: typeof APP_LAYOUT_FLAGS_KEY; | ||
| readonly rootLayout: typeof APP_ROOT_LAYOUT_KEY; | ||
| readonly route: typeof APP_ROUTE_KEY; | ||
| }; |
There was a problem hiding this comment.
The previous review flagged keys as dead code — I agree it has zero consumers today, but the comment on line 229-230 explains the intent well enough. No action needed; just noting I looked at this and the comment is sufficient.
One thought for WIRE-02: if the plan is for external consumers to use AppElementsWire.keys.route instead of importing APP_ROUTE_KEY directly, it'd be worth adding a test that asserts the key values match the raw constants. That way if someone renames a constant without updating keys, the test catches it. Not needed in this PR since keys is unused.
| } | ||
|
|
||
| function isLayoutFlagsRecord(value: unknown): value is LayoutFlags { | ||
| if (typeof value !== "object" || value === null || Array.isArray(value)) return false; |
There was a problem hiding this comment.
Nit: the guard refactor from !value || typeof value !== "object" to typeof value !== "object" || value === null || Array.isArray(value) is a clarity improvement. The old !value was doing double duty (rejecting falsy non-objects AND null), which read oddly since typeof null === "object". The explicit null check here is better. Just noting this was intentional, not accidental.
|
Review submitted and approved. Here's the summary: Approved — clean extraction with no behavioral changes. The Key verifications:
|
| routePath: string, | ||
| interceptionContext: string | null, | ||
| ): string { | ||
| return appendInterceptionContext(`route:${routePath}`, interceptionContext); |
There was a problem hiding this comment.
wasn't there a util in one of hte other prs for the route:${...} etc. stuff?
What this changes
Introduces #726-WIRE-01 by making AppElements wire transport pass through an explicit AppElementsWire codec boundary.
Production App Router callers now use AppElementsWire for route ids, page ids, cache keys, metadata entries, outgoing payload layout flags, wire decode, and metadata reads. The existing app-elements module remains as the compatibility barrel for current helper imports, but current production encode/decode/write paths no longer assemble raw AppElements wire metadata directly.
Why
Issue #726 calls out the flat keyed AppElements payload as a transport bridge, not the router brain. Before this PR, route identity, cache identity, metadata writes, unmatched-slot transport, and payload parsing were spread across call sites as individual helper calls and raw keys. That makes later ownership work harder because there is no single place to reason about the RSC/HTML payload boundary.
Correctness oracle: Vinext internal invariant. This PR preserves current observable App Router behavior while introducing an enforceable wire owner for the next #726 waves.
Approach
Add server/app-elements-wire.ts as the codec owner for:
Then route the current production call sites through AppElementsWire:
This intentionally does not promote topology, slot ownership, lifecycle authority, or cache semantics. Those remain later #726 waves.
Validation
The focused test matrix passed 530 tests. The build completed with the repo's existing virtual-module unresolved-import warnings.
Risks / follow-ups
Open PR overlap risk exists around App Router generated entry and browser navigation files, especially #1057, #922, #698, #647, #488, #404 and older dirty PRs. This branch is based on current upstream/main and currently publishes cleanly from there.
WIRE-02 should remove or lint against raw wire-key construction outside AppElementsWire. This PR deliberately leaves legacy helper exports where covered by existing compatibility tests so the ownership migration stays reviewable.