feat(app-router): add artifact compatibility metadata#1062
feat(app-router): add artifact compatibility metadata#1062james-elicx merged 5 commits intocloudflare:mainfrom
Conversation
App Router payloads now carry a parsed compatibility envelope at the existing payload metadata boundary. Without this skeleton, future cache and skip work would have no typed place to record graph, deployment, schema, root-boundary, or render-epoch proof. Add a small constructor/parser pair, attach a default unknown-proof envelope to outgoing record payloads, and reject malformed envelope metadata when reading payload metadata. Tests cover explicit envelopes, legacy missing envelopes, malformed envelopes, and non-mutating payload writes.
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: feat(app-router): add artifact compatibility envelope
Clean skeleton PR. The typed envelope, fail-closed parse boundary, and unknown-proof default are all well-structured. Tests pass (32/32). A few items to address:
Issues
-
Redundant schema versions baked into every payload —
appElementsSchemaVersionandrscPayloadSchemaVersionare hardcoded constants that always equal1. They're redundant withschemaVersion(which already versions the envelope format). If the app-elements wire shape changes, you'd bump the envelopeschemaVersionanyway. Carrying three version numbers on every outgoing RSC row adds wire overhead for no benefit. Consider dropping these two fields and relying onschemaVersionalone to version the envelope, or document why per-layer versioning is needed. -
parseArtifactCompatibilityEnvelopesilently coerces future schema versions tonull— If a newer server sendsschemaVersion: 2, the parser returnsnull, and the caller throws. That's correct fail-closed behavior. But the error message says "Invalid __artifactCompatibility" which doesn't distinguish between corruption and a legitimate version mismatch. When#726-COMPAT-02lands with actual cache-hit paths, you'll want to tell the difference. Worth a comment or a follow-up item now. -
buildOutgoingAppPayloadalways attaches the envelope even when no caller passes it — The single production callsite inapp-page-render.ts:257does not passartifactCompatibility, so every payload gets a default all-nulls envelope. That's fine for the skeleton, but it means the wire format changes for every App Router response right now with no consumer. If this is intentional (to start populating the field so readers can depend on it), that's fine, just worth noting.
Nits
-
Minor: the
withArtifactCompatibilityEnvelopehelper is private and only called in one place insidebuildOutgoingAppPayload. The indirection is fine for symmetry withwithLayoutFlags, but unlikewithLayoutFlags(which is exported and tested independently), this one could be inlined. Not blocking. -
Test coverage is solid — the three cases (present envelope, missing/legacy, malformed) cover the parse boundary well. One gap: there's no test for a future/unknown
schemaVersion(e.g.,schemaVersion: 99). That's the most realistic failure mode in production and worth an explicit test case.
| @@ -0,0 +1,71 @@ | |||
| export const ARTIFACT_COMPATIBILITY_SCHEMA_VERSION = 1; | |||
| export const APP_ELEMENTS_SCHEMA_VERSION = 1; | |||
| export const RSC_PAYLOAD_SCHEMA_VERSION = 1; | |||
There was a problem hiding this comment.
These two constants (APP_ELEMENTS_SCHEMA_VERSION, RSC_PAYLOAD_SCHEMA_VERSION) are always 1 and always match ARTIFACT_COMPATIBILITY_SCHEMA_VERSION. Every outgoing payload carries all three, but they're never checked independently — hasCurrentSchemaVersions requires all three to match their constants simultaneously.
What's the versioning scenario where appElementsSchemaVersion and rscPayloadSchemaVersion would diverge from the envelope's schemaVersion? If there isn't one yet, consider collapsing to a single version field to keep the wire format lean. If there is a planned use case (e.g., the RSC payload schema evolves independently of the elements schema), a short comment here explaining the intent would help future readers.
| value: unknown, | ||
| ): ArtifactCompatibilityEnvelope | null { | ||
| if (!isRecord(value)) return null; | ||
| if (!hasCurrentSchemaVersions(value)) return null; |
There was a problem hiding this comment.
When hasCurrentSchemaVersions returns false, this returns null, which the caller turns into an unconditional throw. That's correct fail-closed behavior, but it conflates two very different scenarios:
- Version mismatch (e.g.,
schemaVersion: 2from a newer deployment) — expected during rolling deploys - Corruption (e.g.,
schemaVersion: "banana") — a bug
Worth at least a comment here noting that once cache-hit or skip-transport paths depend on this, the caller should distinguish these cases (e.g., version-mismatch → treat as cache miss, corruption → log and reject).
| return withLayoutFlags(input.element, input.layoutFlags); | ||
| return withArtifactCompatibilityEnvelope( | ||
| withLayoutFlags(input.element, input.layoutFlags), | ||
| input.artifactCompatibility ?? createArtifactCompatibilityEnvelope(), |
There was a problem hiding this comment.
This default means every App Router response now carries __artifactCompatibility on the wire even though the only production callsite (app-page-render.ts:257) never passes it. The envelope is all nulls (unknown proof), so it's semantically inert, but it does increase payload size.
Is the intent to always emit the field so that readers can unconditionally depend on its presence? If so, a brief comment here would clarify that for future readers. If not, consider making the attachment conditional (only when artifactCompatibility is explicitly provided).
| }, | ||
| }), | ||
| ).toThrow("[vinext] Invalid __artifactCompatibility in App Router payload"); | ||
| }); |
There was a problem hiding this comment.
Good coverage for the type-validation case (graphVersion: 123). Missing a test for the most realistic production failure mode: future schema version. A payload with schemaVersion: 99 (all other fields valid) should also be rejected. This would exercise the hasCurrentSchemaVersions guard path specifically:
| }); | |
| ).toThrow("[vinext] Invalid __artifactCompatibility in App Router payload"); | |
| }); | |
| it("rejects artifact compatibility with an unrecognized schema version", () => { | |
| expect(() => | |
| readAppElementsMetadata({ | |
| ...normalizeAppElements({ | |
| [APP_ROOT_LAYOUT_KEY]: "/", | |
| [APP_ROUTE_KEY]: "route:/dashboard", | |
| }), | |
| [APP_ARTIFACT_COMPATIBILITY_KEY]: { | |
| schemaVersion: 99, | |
| graphVersion: null, | |
| deploymentVersion: null, | |
| appElementsSchemaVersion: APP_ELEMENTS_SCHEMA_VERSION, | |
| rscPayloadSchemaVersion: RSC_PAYLOAD_SCHEMA_VERSION, | |
| rootBoundaryId: null, | |
| renderEpoch: null, | |
| }, | |
| }), | |
| ).toThrow("[vinext] Invalid __artifactCompatibility in App Router payload"); | |
| }); |
|
Review posted. Here's a summary of the feedback: Issues raised:
|
The review correctly identified that the envelope parser had malformed-field coverage but no direct test for the version guard path. Add the future-schema-version case and document why the Wave01 skeleton emits separate schema versions and an unknown-proof envelope without yet splitting mismatch from corruption behavior.
|
/bigbonk review |
There was a problem hiding this comment.
Review: feat(app-router): add artifact compatibility envelope
The skeleton is well-structured overall — typed envelope, fail-closed parse boundary, unknown-proof default, and clean test coverage. But there are a few issues that should be addressed before merging, mostly around unnecessary wire overhead and silent consumer breakage.
Must-fix
-
Every consumer of
readAppElementsMetadatanow receivesartifactCompatibilitybut none of them use it.app-browser-entry.ts(lines 427, 1135),app-browser-state.ts(line 215), andapp-ssr-entry.ts(line 221) all destructure or read specific fields from the returned metadata — none readartifactCompatibility. This is fine for a skeleton, but thereadAppElementsMetadataparse path now throws on malformed envelopes (line 244), which means a corrupt or future-version__artifactCompatibilityvalue on the wire will crash the browser entry, SSR entry, and browser state — all for a field nobody consumes yet. The fail-closed behavior is correct in principle, but crashing the entire render pipeline for an unused field is aggressive. Consider either (a) making the parse non-throwing when no consumer exists (log + default to unknown proof), or (b) documenting why a hard crash on an unused field is the intentional tradeoff. -
parseArtifactCompatibilityEnvelopereconstructs the envelope from constants instead of preserving parsed values. Lines 70-76 overwriteschemaVersion,appElementsSchemaVersion, andrscPayloadSchemaVersionwith the local constants after validating them. This is technically correct today (the version check on line 63 already ensures equality), but it means the function's return type can never carry a different schema version — making it impossible to extend for forward-compat without changing the function signature. If this is intentional ("we always normalize to current"), add a comment. If not, returnvalue.schemaVersionetc. directly.
Should-fix
-
Three version fields that always have the same value.
schemaVersion,appElementsSchemaVersion, andrscPayloadSchemaVersionare all1, checked together inhasCurrentSchemaVersions, and constructed together increateArtifactCompatibilityEnvelope. The PR description and comment on lines 3-5 say they could diverge in the future, but there's no concrete scenario described. Every outgoing RSC row now carries three extra fields for this speculative use case. Recommend collapsing to a singleschemaVersionuntil there's an actual need for per-layer versioning — it's easy to split later when the semantics are clear. -
Missing test: non-object
__artifactCompatibilityvalue. The tests covergraphVersion: 123(wrong field type) andschemaVersion: 99(future version), but don't test a non-object value like__artifactCompatibility: "garbage"or__artifactCompatibility: 42. TheisRecordguard on line 59 handles this, but it should have an explicit test.
Nit
withArtifactCompatibilityEnvelopeis private, called once, and follows the same pattern as the exportedwithLayoutFlags. The symmetry is fine, but unlikewithLayoutFlagsit has no independent tests and no external consumers. If it stays private and single-use, consider inlining it intobuildOutgoingAppPayloadto reduce indirection.
| ? createArtifactCompatibilityEnvelope() | ||
| : parseArtifactCompatibilityEnvelope(artifactCompatibilityValue); | ||
| if (!artifactCompatibility) { | ||
| throw new Error("[vinext] Invalid __artifactCompatibility in App Router payload"); |
There was a problem hiding this comment.
This throw is reachable from three production callsites (app-browser-entry.ts:427, app-browser-entry.ts:1135, app-ssr-entry.ts:221) — none of which read artifactCompatibility from the returned metadata. A hard crash for an unused field is a surprising failure mode.
If the intent is defense-in-depth ("reject payloads that are structurally invalid even if we don't consume the field yet"), that's a valid choice but should be documented with a comment here explaining why. Otherwise, consider degrading gracefully:
| throw new Error("[vinext] Invalid __artifactCompatibility in App Router payload"); | |
| if (!artifactCompatibility) { | |
| // TODO(#726-COMPAT-02): throw once consumers depend on this field. | |
| // For now, degrade to unknown proof so a malformed envelope doesn't | |
| // crash the render pipeline for a field nobody reads yet. | |
| // throw new Error("[vinext] Invalid __artifactCompatibility in App Router payload"); | |
| } |
(Or keep the throw but add a comment explaining the intentional tradeoff.)
| // but future rolling deploy work can bump the envelope shape independently | ||
| // from the flat AppElements record or the serialized RSC payload. | ||
| export const APP_ELEMENTS_SCHEMA_VERSION = 1; | ||
| export const RSC_PAYLOAD_SCHEMA_VERSION = 1; |
There was a problem hiding this comment.
The comment on lines 3-5 says these "can" diverge, but there's no concrete scenario where they would. Today hasCurrentSchemaVersions checks all three together, createArtifactCompatibilityEnvelope sets all three to 1, and the type pins them to typeof their respective constants.
Carrying two extra version fields on every outgoing RSC row for a speculative future use case adds wire overhead now. If/when a real scenario emerges (e.g., the RSC payload format evolves independently of the elements record), it's easy to add the field then.
Recommend collapsing to a single schemaVersion and re-introducing per-layer versions when there's a concrete need. At minimum, if you want to keep them, document a specific scenario in the comment rather than the generic "future rolling deploy work can bump...".
| if (!isStringOrNull(value.renderEpoch)) return null; | ||
|
|
||
| return { | ||
| schemaVersion: ARTIFACT_COMPATIBILITY_SCHEMA_VERSION, |
There was a problem hiding this comment.
This reconstructs the return value from local constants (ARTIFACT_COMPATIBILITY_SCHEMA_VERSION, APP_ELEMENTS_SCHEMA_VERSION, RSC_PAYLOAD_SCHEMA_VERSION) instead of using the validated values from the input (value.schemaVersion, etc.).
Since hasCurrentSchemaVersions on line 63 already verified exact equality, this is semantically identical today. But it means the function's return type is structurally locked to the current version — a future reader might expect that parsing schemaVersion: 2 could return an envelope with schemaVersion: 2, but it can't.
If this is intentional normalization ("we always stamp the current constants regardless of input"), add a brief comment. If not:
| schemaVersion: ARTIFACT_COMPATIBILITY_SCHEMA_VERSION, | |
| return { | |
| schemaVersion: value.schemaVersion as typeof ARTIFACT_COMPATIBILITY_SCHEMA_VERSION, | |
| graphVersion: value.graphVersion, | |
| deploymentVersion: value.deploymentVersion, | |
| appElementsSchemaVersion: value.appElementsSchemaVersion as typeof APP_ELEMENTS_SCHEMA_VERSION, | |
| rscPayloadSchemaVersion: value.rscPayloadSchemaVersion as typeof RSC_PAYLOAD_SCHEMA_VERSION, | |
| rootBoundaryId: value.rootBoundaryId, | |
| renderEpoch: value.renderEpoch, | |
| }; |
| }, | ||
| }), | ||
| ).toThrow("[vinext] Invalid __artifactCompatibility in App Router payload"); | ||
| }); |
There was a problem hiding this comment.
Good — this test was missing from the initial commit and was added per the prior review. Consider also adding a test for non-object values to exercise the isRecord guard:
| }); | |
| ).toThrow("[vinext] Invalid __artifactCompatibility in App Router payload"); | |
| }); | |
| it("rejects non-object artifact compatibility values", () => { | |
| expect(() => | |
| readAppElementsMetadata({ | |
| ...normalizeAppElements({ | |
| [APP_ROOT_LAYOUT_KEY]: "/", | |
| [APP_ROUTE_KEY]: "route:/dashboard", | |
| }), | |
| [APP_ARTIFACT_COMPATIBILITY_KEY]: "not-an-object", | |
| }), | |
| ).toThrow("[vinext] Invalid __artifactCompatibility in App Router payload"); | |
| }); |
|
Review posted. Here's a summary of the feedback: Must-fix:
Should-fix:
Nit:
|
|
@james-elicx Bonk can't see the big picture so I'll just do compat02 as well |
NathanDrake2406
changed the title
App Router payloads previously carried only the COMPAT-01 unknown envelope unless a caller manually supplied metadata. That left COMPAT-02/03 without concrete graph, deployment, root-boundary, and fallback behavior in the PR. The render lifecycle now attaches compatibility metadata for record payloads from route/root/deployment facts, while renderEpoch remains unknown until an epoch owner exists. The compatibility evaluator returns explicit compatible, unknown, or incompatible decisions so unknown proof always falls back to render fresh instead of becoming reuse proof. Tests cover render-boundary metadata attachment and unknown graph, deployment, root-boundary, and render-epoch decisions.
…1-envelope # Conflicts: # packages/vinext/src/server/app-elements.ts # packages/vinext/src/server/app-page-render.ts # tests/app-elements.test.ts
|
/bigbonk review |
There was a problem hiding this comment.
Review: feat(app-router): add artifact compatibility metadata
The envelope design, fail-closed parser, and unknown-proof default are well-structured. The evaluator logic is clean and the test coverage is solid. Two issues need addressing before this merges.
Must-fix
-
Hard throw on malformed envelope crashes consumers that never read the field.
readAppElementsMetadatanow throws on line 247 ofapp-elements-wire.tswhenparseArtifactCompatibilityEnvelopereturnsnull. Every consumer ofreadMetadatahits this path —app-browser-entry.ts:457(initial mount),app-browser-entry.ts:1165(client navigation),app-ssr-entry.ts:201(SSR render),app-browser-state.ts:358(pending nav commit). None of these readartifactCompatibilityfrom the returned metadata. A future schema version (schemaVersion: 2) or a corrupted field on the wire will crash the entire render pipeline for a value nobody consumes. ThelayoutFlagsfield sets the right precedent here —parseLayoutFlagsdegrades gracefully to{}on invalid input. The artifact compatibility parse should do the same until consumers exist. -
evaluateArtifactCompatibilityis dead code with no callsite. The function is exported, tested, but never called from any production code path. It's fine to land it alongside the envelope as scaffolding, but it should have a brief comment noting it's pre-positioned for#726-COMPAT-04/05consumers — otherwise it looks like an oversight.
Should-fix
-
Three version fields that always have the same value.
schemaVersion,appElementsSchemaVersion, andrscPayloadSchemaVersionare all1, always checked together inhasCurrentSchemaVersions, and always constructed together. The comment on lines 5-7 says they "can" diverge in the future, but no concrete scenario is described. Recommend either (a) collapsing to a singleschemaVersionand splitting later when the semantics are clear, or (b) adding a concrete example of when they'd diverge to justify the wire overhead. -
Parser reconstructs from constants instead of preserving validated input. Lines 114-122 of
artifact-compatibility.tsoverwriteschemaVersion,appElementsSchemaVersion, andrscPayloadSchemaVersionwith local constants after thehasCurrentSchemaVersionscheck already verified equality. This structurally locks the return type to the current version and prevents forward-compat extension without a function signature change. If this is intentional normalization, add a comment. If not, returnvalue.schemaVersionetc. directly.
Nit
- Missing test for non-object
__artifactCompatibilityvalues. TheisRecordguard on line 104 handles non-object inputs, but there's no test exercising that path (e.g.,__artifactCompatibility: "garbage"or42).
| ? createArtifactCompatibilityEnvelope() | ||
| : parseArtifactCompatibilityEnvelope(artifactCompatibilityValue); | ||
| if (!artifactCompatibility) { | ||
| throw new Error("[vinext] Invalid __artifactCompatibility in App Router payload"); |
There was a problem hiding this comment.
This throw is reachable from four production callsites (app-browser-entry.ts:457, app-browser-entry.ts:1165, app-ssr-entry.ts:201, app-browser-state.ts:358) — none of which read artifactCompatibility from the returned metadata.
A malformed or future-version __artifactCompatibility on the wire will crash the entire render pipeline for a field nobody consumes yet. Compare with layoutFlags directly above — parseLayoutFlags degrades gracefully to {} when the value is invalid, which is the right pattern for a field with no active consumers.
| throw new Error("[vinext] Invalid __artifactCompatibility in App Router payload"); | |
| if (!artifactCompatibility) { | |
| // TODO(#726-COMPAT-02): throw once consumers depend on this field. | |
| // For now, degrade to unknown proof so a malformed envelope doesn't | |
| // crash the render pipeline for a field nobody reads yet. | |
| artifactCompatibility = createArtifactCompatibilityEnvelope(); | |
| } |
| return null; | ||
| } | ||
|
|
||
| export function evaluateArtifactCompatibility( |
There was a problem hiding this comment.
This function is exported and well-tested, but has zero callsites in production code. Worth adding a brief comment noting it's pre-positioned for #726-COMPAT-04/05 so it doesn't look like dead code that was accidentally left behind.
| export function evaluateArtifactCompatibility( | |
| /** Pre-positioned for #726-COMPAT-04/05 cache/skip callers. No production callsite yet. */ | |
| export function evaluateArtifactCompatibility( |
| // These versions describe separate protocol layers. They start in lockstep, | ||
| // but future rolling deploy work can bump the envelope shape independently | ||
| // from the flat AppElements record or the serialized RSC payload. | ||
| export const APP_ELEMENTS_SCHEMA_VERSION = 1; |
There was a problem hiding this comment.
Both APP_ELEMENTS_SCHEMA_VERSION and RSC_PAYLOAD_SCHEMA_VERSION are always 1, always checked together with schemaVersion in hasCurrentSchemaVersions, and always set together in createArtifactCompatibilityEnvelope. Every outgoing RSC row now carries three version fields with identical values.
The comment says they "can" diverge, but no concrete scenario is described. If the app-elements wire shape changes, you'd bump the envelope schemaVersion anyway. Recommend either collapsing to a single schemaVersion (easy to split later when the need is concrete), or adding a specific scenario where they'd diverge to justify the overhead.
| return { | ||
| schemaVersion: ARTIFACT_COMPATIBILITY_SCHEMA_VERSION, |
There was a problem hiding this comment.
These lines overwrite the version fields with local constants after hasCurrentSchemaVersions already verified equality. This means the function can structurally never return a different schema version — a forward-compat concern if you later want to parse-and-preserve a schemaVersion: 2 envelope.
If this is intentional normalization ("always stamp the current constants regardless of input"), add a brief comment. Otherwise, return value.schemaVersion etc. directly:
| return { | |
| schemaVersion: ARTIFACT_COMPATIBILITY_SCHEMA_VERSION, | |
| schemaVersion: value.schemaVersion as typeof ARTIFACT_COMPATIBILITY_SCHEMA_VERSION, | |
| graphVersion: value.graphVersion, | |
| deploymentVersion: value.deploymentVersion, | |
| appElementsSchemaVersion: value.appElementsSchemaVersion as typeof APP_ELEMENTS_SCHEMA_VERSION, | |
| rscPayloadSchemaVersion: value.rscPayloadSchemaVersion as typeof RSC_PAYLOAD_SCHEMA_VERSION, |
| }, | ||
| }), | ||
| ).toThrow("[vinext] Invalid __artifactCompatibility in App Router payload"); | ||
| }); |
There was a problem hiding this comment.
Good coverage for future schema version and wrong field type. Missing a test for a non-object value to exercise the isRecord guard at line 104 of artifact-compatibility.ts:
| }); | |
| ).toThrow("[vinext] Invalid __artifactCompatibility in App Router payload"); | |
| }); | |
| it("rejects non-object artifact compatibility values", () => { | |
| expect(() => | |
| readAppElementsMetadata({ | |
| ...normalizeAppElements({ | |
| [APP_ROOT_LAYOUT_KEY]: "/", | |
| [APP_ROUTE_KEY]: "route:/dashboard", | |
| }), | |
| [APP_ARTIFACT_COMPATIBILITY_KEY]: "not-an-object", | |
| }), | |
| ).toThrow("[vinext] Invalid __artifactCompatibility in App Router payload"); | |
| }); |
|
Review posted. Here's a summary of the feedback: Must-fix:
Should-fix:
Nit:
|
App Router metadata parsing crashed when __artifactCompatibility was malformed or from a future schema, even though current render and navigation callers do not consume that field yet. The parser treated compatibility scaffolding like required route metadata. That made an unused proof capable of failing initial mount, SSR, and client navigation paths. Default invalid compatibility metadata to the unknown proof for Wave01, document the planned hard-fail point for cloudflare#726-COMPAT-04/05, and cover malformed, future-version, and non-object payloads.
|
/bigbonk review |
There was a problem hiding this comment.
Review: feat(app-router): add artifact compatibility metadata
The latest revision addresses the critical must-fix from prior reviews — readArtifactCompatibilityMetadata now degrades gracefully to an unknown-proof envelope instead of throwing, matching the parseLayoutFlags precedent. The fail-closed parser, typed envelope, evaluator logic, and test coverage are all clean.
Two remaining items from prior reviews that I agree with in principle but are fine to defer:
-
Three version fields that are always 1 —
schemaVersion,appElementsSchemaVersion,rscPayloadSchemaVersion. The comment on lines 5-7 explains the intent (independent layer versioning for rolling deploys), which is a reasonable forward-looking choice. If this ends up never diverging, it's easy to collapse later. Not blocking. -
evaluateArtifactCompatibilityhas no production callsite — Already addressed with the comment at line 157 noting it's pre-positioned for#726-COMPAT-04/05.
One small observation and one nit below. Overall this is a well-structured skeleton that does what it says — adds the envelope, emits it on outgoing payloads, and keeps the render pipeline safe from parse failures in a field nobody reads yet.
| ...input.element, | ||
| [APP_LAYOUT_FLAGS_KEY]: input.layoutFlags, | ||
| [APP_ARTIFACT_COMPATIBILITY_KEY]: | ||
| input.artifactCompatibility ?? createArtifactCompatibilityEnvelope(), |
There was a problem hiding this comment.
When artifactCompatibility is not passed, the default envelope is created twice — once here via createArtifactCompatibilityEnvelope(), and once on the read side in readArtifactCompatibilityMetadata when the value round-trips through the parser successfully. This is fine (the envelope is cheap to construct), but worth noting that the write side always emits the envelope even for non-record payloads where createAppPageArtifactCompatibility returns undefined — because buildOutgoingAppPayload only reaches this line for record elements (the early return on line 199 handles non-records). So the fallback here only fires when the production callsite in app-page-render.ts doesn't pass artifactCompatibility, which currently only happens if isAppElementsRecord returns false for the element (which then means this line is unreachable since we already returned on line 200). In practice, the ?? fallback is dead code for the current callsite — it would only matter for future callers that pass a record element without artifactCompatibility. Not blocking, just a note for context.
| const outgoingElement = AppElementsWire.encodeOutgoingPayload({ | ||
| element: options.element, | ||
| layoutFlags, | ||
| ...(artifactCompatibility ? { artifactCompatibility } : {}), |
There was a problem hiding this comment.
Nit: The conditional spread ...(artifactCompatibility ? { artifactCompatibility } : {}) is equivalent to just artifactCompatibility since buildOutgoingAppPayload already has artifactCompatibility?: (optional) in its input type, and undefined properties are ignored by the spread. This could be simplified to:
| ...(artifactCompatibility ? { artifactCompatibility } : {}), | |
| artifactCompatibility, |
The conditional spread pattern suggests the field might be harmful if passed as undefined, but the implementation in buildOutgoingAppPayload handles the missing case with ?? createArtifactCompatibilityEnvelope() anyway. Minor style nit, not blocking.
|
Review posted. Here's a summary: Approved. The latest revision addresses the critical must-fix from prior rounds — malformed/future-version envelopes now degrade gracefully to unknown-proof instead of crashing the render pipeline. The typed envelope, fail-closed parser, evaluator logic, and test coverage (65 tests passing) are all clean. Feedback posted:
Deferred items acknowledged as reasonable: the three version fields (collapsible later if they never diverge) and |
…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
#726-COMPAT-01plus#726-COMPAT-02/03from #726.This PR adds the App Router artifact compatibility envelope, attaches concrete render-boundary metadata to outgoing record payloads, and defines a conservative compatibility evaluator for future cache/skip callers.
Why
Issue #726 requires every reusable or skippable artifact to carry compatibility proof before cache reuse or skip transport can depend on it. The rule for this slice is deliberately strict: no proof, no reuse. Unknown compatibility is represented as a render-fresh fallback, never as positive compatibility proof.
Approach
packages/vinext/src/server/artifact-compatibility.tswith schema constants,ArtifactCompatibilityEnvelope, constructor/parser helpers, a route/root graph fingerprint helper, andevaluateArtifactCompatibility().renderAppPageLifecyclewhen the outgoing App Router payload is a record:process.env.__VINEXT_BUILD_IDrootBoundaryIdfrom__rootLayoutwhen availablerenderEpoch: nulluntil a real epoch owner existsCorrectness oracle
Vinext internal invariant from #726: no reusable or skippable artifact should gain implicit compatibility proof from cache presence, artifact presence, or wire shape. Unknown graph/deployment/root/epoch compatibility must return a render-fresh fallback, and only fully known matching metadata can produce
compatible.A focused Next.js source/test search for
artifact compatibility,compatibility envelope,renderEpoch,deploymentVersion, andgraphVersionfound no public Next.js behavior to port for this internal Vinext protocol. This PR does not claim public Next.js behavior parity for rolling deploys.Validation
vp test run tests/app-elements.test.ts tests/app-page-render.test.tsvp check packages/vinext/src/server/artifact-compatibility.ts packages/vinext/src/server/app-elements.ts packages/vinext/src/server/app-page-render.ts tests/app-elements.test.ts tests/app-page-render.test.tsvp run vinext#buildRisks / follow-ups
graphVersionhelper is a narrow route/root fingerprint until the RouteManifest graphVersion from GRAPH-02/03 exists.renderEpochis carried but remainsnull, so current artifacts cannot become positive reuse proof solely from matching graph/deployment/root metadata.#726-COMPAT-04/05should add old-client/new-server coverage and hard-navigation loop prevention before any broader deployment fallback behavior depends on this envelope.Refs #726