feat(ppr): add PPR fallback-shell render lifecycle tests#1715
Conversation
commit: |
|
Replacing with stacked version |
NathanDrake2406
force-pushed
the
nathan/pr-3-render-lifecycle
branch
2 times, most recently
from
d30655b to
99eab93
Compare
…lution seams Address review feedback on PR cloudflare#1702: - Explain why run-prerender reuses the built BUILD_ID instead of re-resolving one (config.buildId feeds prerendered-output identity). - Note that the fallback-shell prerender-params header path described in app-ppr-fallback-shell.ts is honored by the cloudflare#1715 follow-up; this generation-only PR still resolves params from the URL placeholder.
…#1702) * fix(app-router): prerender cacheComponents root-param fallback shells PR 1 of 4: core model + build integration. Model: app-ppr-fallback-shell, pregenerated-concrete-paths, prerender-manifest Build: prerender/run-prerender fallback shell artifact generation Tests: createAppPprFallbackShells, pregenerated-concrete-paths core * test(prerender-route-params): add unit tests for fallbackParamNames and validation Adds focused unit tests for the new prerender-route-params logic introduced in this PR: - encodePrerenderRouteParams round-trips fallbackParamNames and omits them when empty. - prerenderRouteParamsPayloadMatchesRoute rejects payloads whose fallbackParamNames contain unknown params or duplicates. - prerenderRouteParamsPayloadMatchesRoute returns false for valid fallback-shell matches (only exact matches are accepted today). Also adds a comment in app-ppr-fallback-shell.ts noting that placeholder brackets in shell pathnames are percent-encoded by new URL() at fetch time, so the render path must supply params via the prerender-params header rather than URL matching. * docs(prerender): clarify BUILD_ID reuse and fallback-shell param-resolution seams Address review feedback on PR #1702: - Explain why run-prerender reuses the built BUILD_ID instead of re-resolving one (config.buildId feeds prerendered-output identity). - Note that the fallback-shell prerender-params header path described in app-ppr-fallback-shell.ts is honored by the #1715 follow-up; this generation-only PR still resolves params from the URL placeholder. --------- Co-authored-by: James <james@eli.cx>
NathanDrake2406
force-pushed
the
nathan/pr-3-render-lifecycle
branch
from
99eab93 to
bccba8f
Compare
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the fallback-shell render-lifecycle shim and the thenable-params integration. The cache-task tracking state machine (trackPprFallbackShellCacheTask / ignoreCacheTask / scheduleCacheReadyIfSettled) is well thought out — the ALS task-stack approach correctly stops waiting for cache work that suspends on fallback params while letting independent child cache work complete, and the counter is guarded against double-decrement (task.isPending). Tests pass locally (vp test run tests/ppr-fallback-shell.test.ts tests/thenable-params.test.ts → 25 passed).
Since the new lifecycle exports (trackPprFallbackShellCacheTask, waitForPprFallbackShellCacheReady, preparePprFallbackShellFinalRender, isPprFallbackShellAbortError) are not yet wired into any production render path (only thenable-params.ts consumes createPprFallbackShellSuspensePromise / getPprFallbackShellState), the runtime impact is gated behind runWithPprFallbackShellState, which today only tests invoke. That matches the stated non-goals, so the comments below are mostly about the contract this PR establishes for #1716 to build on.
Main concern: the thenable-params integration makes side effects and await params behavior eager rather than per-access, which diverges from both the PR's stated contract ("suspend only when actually accessed") and from Next.js. Details inline.
| return typeof prop === "string" && (fallbackParamNames?.has(prop) ?? false); | ||
| } | ||
|
|
||
| const promise = fallbackShellPromise ?? Promise.resolve(plain); |
There was a problem hiding this comment.
await params will hang for any route that has fallback params, even when only known params are read.
Replacing the base promise with the hanging fallbackShellPromise means await params no longer resolves to the plain object during fallback-shell prerendering — it hangs until abort. But the common server-component pattern is const { locale } = await params, where locale is a known root param. Under this change that awaits the hanging promise and never reaches the destructure.
The existing contract (tests at lines 9-17, 72-93, 95-108) is that await params resolves. The new test only covers synchronous Reflect.get(params, "slug"), so this regression isn't caught.
In Next.js (see the params.ts#L197-L367 reference in the PR description), awaiting params during fallback prerender resolves to a proxy; only accessing a fallback key on the resolved object throws the hanging promise. Suggest keeping the base promise resolvable and only throwing on fallback-key access:
| const promise = fallbackShellPromise ?? Promise.resolve(plain); | |
| const promise = Promise.resolve(plain); |
If the resolved object also needs to suspend on fallback-key access, that should be done by wrapping the resolved value, not by hanging the whole promise. Please add a test for const { locale } = await params returning the known param without hanging.
| Object.keys(plain).some((key) => fallbackShellState.fallbackParamNames.has(key)) | ||
| ? fallbackShellState.fallbackParamNames | ||
| : null; | ||
| const fallbackShellPromise = fallbackParamNames |
There was a problem hiding this comment.
Eager construction triggers lifecycle side effects before any fallback param is accessed.
createPprFallbackShellSuspensePromise is called here at makeThenableParams construction time, and it has two non-lazy side effects: it sets state.hasDynamicBoundary = true and calls ignoreCacheTask(...) on every task in the current ALS stack. So merely building the params object for a route that has a fallback param marks the shell as having a dynamic boundary and stops waiting for in-scope cache work — even if the component only ever reads the known root param and never touches the fallback key.
That contradicts the PR's stated contract ("fallback params suspend only on access" / "suspend lazily"). Consider constructing the suspense promise lazily on first fallback-key access instead, so the dynamic-boundary marking and cache-task ignoring only happen when a fallback param is genuinely read. A null test asserting state.hasDynamicBoundary === false when only locale is accessed would lock this in.
| } | ||
|
|
||
| state.isAbortScheduled = true; | ||
| scheduleAfterTask(() => { |
There was a problem hiding this comment.
The cleanup function returned by scheduleAfterTask is discarded here, so a scheduled abort timer can't be cancelled. In practice preparePprFallbackShellFinalRender sets isAbortScheduled = false and resets hasDynamicBoundary, and the inner guard re-checks current state before aborting, so a stale timer firing after reset is a no-op rather than a wrong abort. Still, this leaves an uncancellable pending timer per scheduled abort. Consider storing the cleanup (e.g. alongside pendingCacheReadyCleanup) and clearing it in preparePprFallbackShellFinalRender, matching how cache-ready cleanup is handled.
| }); | ||
|
|
||
| await delay(5); | ||
| expect(isReady).toBe(false); |
There was a problem hiding this comment.
These await delay(5) + expect(isReady).toBe(false) assertions rely on the negative case (the resolver genuinely hasn't been called), so they should be stable. But the cache-ready settle path uses two nested setTimeout(0) (scheduleAfterTask), and the positive-case tests that do depend on it firing within a fixed window could be slow under CI load. The tests that await readyPromise directly (e.g. line 60's test) are more robust. No change required, just flagging the timer dependency in case of intermittent CI flakes.
|
Review submitted successfully. Here's a summary of what I found in PR #1715. Review summaryThe PR adds a fallback-shell prerender lifecycle shim ( Findings posted (4 inline comments)
The state-machine work is genuinely good; the main actionable issue is the |
- Make await params resolvable during fallback-shell prerendering by keeping the base promise as Promise.resolve(plain) and only throwing the hanging promise on sync fallback-key access. - Defer createPprFallbackShellSuspensePromise to first fallback-key access so hasDynamicBoundary and cache-task ignoring only fire when a fallback param is actually read, matching the suspend only on access contract. - Store and clear pendingAbortCleanup in preparePprFallbackShellFinalRender to avoid leaking uncancellable abort timers. - Add tests for await params resolving known params and for lazy dynamic-boundary marking when only non-fallback params are accessed.
NathanDrake2406
left a comment
NathanDrake2406
left a comment
There was a problem hiding this comment.
Review feedback addressed
1. await params hangs for fallback-shell routes
Fixed. The base promise is now always Promise.resolve(plain), so await params resolves to the plain object even when fallback params exist. The hanging promise is only thrown on synchronous access to a fallback key (via get / getOwnPropertyDescriptor). Added a test verifying await params resolves correctly and known params remain readable.
2. Eager construction triggers lifecycle side effects
Fixed. createPprFallbackShellSuspensePromise is now deferred to the first actual fallback-key access via getFallbackShellPromise(). The side effects (hasDynamicBoundary = true and cache-task ignoring) only fire when a fallback param is genuinely read. Added a test asserting state.hasDynamicBoundary === false after accessing only known params, and true after accessing a fallback param.
3. Leaked timer cleanup
Fixed. Added pendingAbortCleanup to PprFallbackShellState. scheduleAbortIfReady now stores the returned cleanup function, and preparePprFallbackShellFinalRender cancels it before resetting state. This matches the existing pendingCacheReadyCleanup pattern and prevents leaking uncancellable timers.
4. Timer-based test stability note
Acknowledged, no code change. The negative-case assertions (expect(isReady).toBe(false)) rely on the resolver not having been called, so they are stable. The positive cases that await readyPromise directly are robust because they resolve via the cache-ready resolver, not timer races.
All targeted tests pass (vp test run tests/ppr-fallback-shell.test.ts tests/thenable-params.test.ts -> 27 passed) and vp check is clean.
…tOwnPropertyDescriptor fully lazy - Promise now resolves to a Proxy (resolvedParams) instead of raw plain. The inner Proxy throws a hanging promise when a fallback param is accessed after await, so `const resolved = await params; resolved.slug` suspends. - Both outer and inner Proxy getOwnPropertyDescriptor traps return a getter that creates the hanging promise lazily. Object.keys(params) no longer triggers side effects merely by enumerating keys. - Added tests for awaited-params suspension and for enumeration not marking hasDynamicBoundary eagerly.
…onditional observer semantics - Extract createResolvedParamsProxy with ProxyHandler<T> (no cast needed) - Extract createThenableParamsProxy with single as ThenableParams<T> boundary cast - Remove all as unknown as double casts - In fallback-shell mode, Promise continuations no longer observe all keys; let the resolved params proxy report actual property access lazily - Fix getOwnPropertyDescriptor to return throwing getter instead of throwing synchronously, preventing Object.keys from triggering suspension - Restructure awaited-params test to wrap entire async flow in ALS scope - Add test: awaiting params during fallback-shell observes only accessed known params
…cess in fallback-shell observer test
|
/bigbonk review |
There was a problem hiding this comment.
Review summary
The three actionable items from the prior ask-bonk review are all properly addressed, and I verified the fixes against the current checkout:
await paramshang — Fixed.makeThenableParamsnow doesPromise.resolve(resolvedParams)(thenable-params.ts:225), soawait paramsresolves to a proxy and only fallback-key access on the resolved object throws the hanging promise. Matches Next.js semantics.- Eager lifecycle side effects — Fixed.
createPprFallbackShellSuspensePromiseis now deferred behindgetFallbackShellPromise()(thenable-params.ts:211-216), sohasDynamicBoundary = true/ cache-task ignoring only fire on actual fallback-param access. The new tests at thenable-params.test.ts:230 and :246 lock this in. - Leaked abort timer — Fixed.
pendingAbortCleanupis stored inscheduleAbortIfReadyand cancelled inpreparePprFallbackShellFinalRender(ppr-fallback-shell.ts:97, :215), matching thependingCacheReadyCleanuppattern.
Verified locally: vp test run tests/ppr-fallback-shell.test.ts tests/thenable-params.test.ts → 29 passed, and vp check on the four changed files is clean (format + lint + types). I also confirmed the fallback-param getOwnPropertyDescriptor accessor does not violate proxy invariants — spread/JSON.stringify correctly propagate the hanging promise rather than throwing a TypeError.
The cache-task state machine remains well structured. The lifecycle exports are still only invoked by tests (gated behind runWithPprFallbackShellState), matching the stated non-goals for #1716.
One minor contract note for #1716 below; not blocking.
| const p = getFallbackShellPromise(); | ||
| if (p) throw p; | ||
| } | ||
| return Reflect.get(plain, prop); |
There was a problem hiding this comment.
Contract note for #1716 (not blocking): fallbackParamNames is captured at construction time, but getFallbackShellPromise() re-reads the ALS state lazily via createPprFallbackShellSuspensePromise → getPprFallbackShellState(). If a fallback-param object escapes the shell ALS scope and a fallback key is then accessed, getPprFallbackShellState() returns null, so createPprFallbackShellSuspensePromise returns null, and if (p) throw p falls through — silently returning the raw placeholder (e.g. "[slug]") here, and undefined from the getOwnPropertyDescriptor accessor at line 151-154. Two slightly different fallthrough values for the same out-of-scope condition.
For a tests-only PR this is purely theoretical, but when this is wired into a real render path in #1716, an escaped params object reading a fallback key after the shell scope exits won't suspend and won't error — it will leak the [slug] placeholder. Worth a deliberate decision (assert in-scope, or normalize the fallthrough) before the runtime serving layer depends on this.
|
fatal: could not read Username for 'https://github.com': No such device or address |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review |
There was a problem hiding this comment.
Review summary
Verified the current checkout: vp test run tests/ppr-fallback-shell.test.ts tests/thenable-params.test.ts → 29 passed, and vp check on all 4 changed files is clean (format + lint + types). The three actionable items from the prior review are confirmed fixed:
await paramsno longer hangs —makeThenableParamsnow doesPromise.resolve(resolvedParams)(thenable-params.ts:225); awaiting resolves to a proxy and only fallback-key access suspends. VerifiedPromise.resolvetreats the proxy as non-thenable (itsthenresolves toundefined), so the awaited value is the proxy itself.- Lazy lifecycle side effects —
createPprFallbackShellSuspensePromiseis deferred behindgetFallbackShellPromise()(thenable-params.ts:211-216);hasDynamicBoundary/cache-task-ignoring only fire on real fallback-param access. Locked in by the newhasDynamicBoundary === falsetests. - Abort timer leak —
pendingAbortCleanupstored inscheduleAbortIfReadyand cancelled inpreparePprFallbackShellFinalRender.
The cache-task state machine is sound: I traced the ALS task-stack so a parent suspending on a fallback param is ignored while an independent child cache task still completes and settles cache-ready (confirmed by the childCompleted test). Proxy invariants check out — spread/Object.keys/getOwnPropertyDescriptor on the fallback-key proxy don't throw TypeError and correctly route suspension through the get trap.
Lifecycle exports remain test-only (gated behind runWithPprFallbackShellState), matching the stated non-goals for #1716.
Two non-blocking notes for #1716 below — both only matter once this is wired into a real render path.
| get() { | ||
| const p = getFallbackShellPromise(); | ||
| if (p) throw p; | ||
| }, |
There was a problem hiding this comment.
Non-blocking (#1716): Inconsistent fallthrough between the get and getOwnPropertyDescriptor traps for the same out-of-scope condition.
When a fallback-param object escapes the shell ALS scope and a fallback key is then read, getFallbackShellPromise() → createPprFallbackShellSuspensePromise() → getPprFallbackShellState() returns null, so the promise is null and if (p) throw p falls through. The get trap then returns Reflect.get(plain, prop) (the raw "[slug]" placeholder), but this getOwnPropertyDescriptor accessor returns undefined (the getter has no return). Same out-of-scope condition, two different observable values, and neither suspends.
For a tests-only PR this is theoretical, but once #1716 wires this into a render path an escaped params object reading a fallback key after the shell scope exits will silently leak the [slug] placeholder instead of suspending or erroring. Worth a deliberate decision (assert in-scope, or normalize the fallthrough) before the serving layer depends on it.
| if (state.phase === "final") { | ||
| scheduleCacheReadyIfSettled(state); | ||
| scheduleAbortIfReady(state); | ||
| } |
There was a problem hiding this comment.
Non-blocking (#1716): During warmup, createPprFallbackShellSuspensePromise ignores in-scope cache tasks (via ignoreCacheTask above) but does not call scheduleCacheReadyIfSettled itself — it relies on the completeCacheTask chain inside ignoreCacheTask to schedule the settle. That works when there's at least one in-scope task to ignore, but if pprFallbackShellCacheTaskStackAls.getStore() is empty at this point (a dynamic API accessed outside any tracked cache task), warmup cache-ready won't be re-scheduled by this call. In the current tests cache-ready is always driven by a tracked task settling, so this is latent. When #1716 drives real warmup renders, confirm a bare headers()/cookies() access with no surrounding cache task still settles the warmup waitForPprFallbackShellCacheReady correctly.
|
fatal: could not read Username for 'https://github.com': No such device or address |
…dy without in-scope task Addresses bonk review on cloudflare#1715: - Capture the originating fallback-shell state at params construction and create the suspense promise against it (via the new createPprFallbackShellSuspensePromiseForState), instead of re-reading the ALS store lazily. A params object that escapes the shell scope now still suspends on fallback-key access rather than leaking the raw `[slug]` placeholder, and the `get` / `getOwnPropertyDescriptor` traps no longer diverge on the out-of-scope path. - During warmup, always call scheduleCacheReadyIfSettled when a dynamic boundary is hit (not only in the final phase), so a bare headers()/cookies() access with an empty cache-task stack still re-schedules the warmup cache-ready settle. The call is a no-op while cache tasks are pending. Adds focused tests for both paths.
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the fallback-shell render-lifecycle shim and the thenable-params integration on the current checkout. The three actionable items from the earlier ask-bonk reviews are genuinely fixed and I verified each:
await paramsno longer hangs:makeThenableParamsdoesPromise.resolve(resolvedParams)andPromise.resolvetreats the proxy as non-thenable (thenis a well-known prop returningundefined), so awaiting yields the proxy and only fallback-key access suspends.- Lazy side effects:
createPprFallbackShellSuspensePromiseForStateis deferred behindgetFallbackShellPromise(), sohasDynamicBoundary/cache-task ignoring only fire on real fallback-param access. Locked in by the newhasDynamicBoundary === falsetests. - The
get/getOwnPropertyDescriptorfallthrough divergence is now moot: becausefallbackShellStateandfallbackParamNamesare captured together at construction and the fallback branch is only entered when both are non-null,getFallbackShellPromise()always returns a non-null promise inside that branch. The escaped-scope test confirms both traps suspend identically.
Verified locally: vp test run tests/ppr-fallback-shell.test.ts tests/thenable-params.test.ts -> 31 passed.
The cache-task state machine is sound and the lifecycle exports remain test-only (gated behind runWithPprFallbackShellState), matching the stated non-goals. Two forward-looking notes for #1716 below; neither is blocking for a tests-only delta.
| state.pendingAbortCleanup(); | ||
| state.pendingAbortCleanup = null; | ||
| } | ||
| state.abortController = new AbortController(); |
There was a problem hiding this comment.
Non-blocking (#1716): negative pendingCacheTasks if a warmup cache task settles after the final-render transition.
preparePprFallbackShellFinalRender unconditionally resets pendingCacheTasks = 0, but a tracked warmup task's .finally (ppr-fallback-shell.ts:174) calls completeCacheTask, which only guards on task.isPending — not on the current phase. If a warmup task is still in flight when the transition happens, its later settle decrements the freshly-reset counter to -1.
Consequences while latent: resolveCacheReadyIfSettled checks pendingCacheTasks !== 0, so a -1 counter would block cache-ready from ever resolving, while scheduleAbortIfReady's > 0 guard would still allow abort. Once #1716 drives real warmup->final transitions with in-flight cache work, this can wedge the final render. Worth either draining/marking outstanding warmup tasks as ignored in preparePprFallbackShellFinalRender, or guarding completeCacheTask against decrementing below zero.
| // fire on first actual fallback-key access. | ||
| if (!fallbackParamNames || !fallbackShellState) return null; | ||
| if (!fallbackShellPromise) { | ||
| fallbackShellPromise = createPprFallbackShellSuspensePromiseForState<T>( |
There was a problem hiding this comment.
Non-blocking (#1716): cached fallbackShellPromise binds to a stale AbortController across the warmup->final transition.
The hanging promise is created once and memoized (fallbackShellPromise). It captures state.abortController.signal at first fallback-key access (ppr-fallback-shell.ts:200-201). But preparePprFallbackShellFinalRender replaces state.abortController with a brand-new controller (ppr-fallback-shell.ts:232). So a params object built during warmup that is read again after the transition keeps throwing a promise wired to the old signal, which the lifecycle never aborts again — only the new final controller is.
In practice each render call builds a fresh params object, so this only bites if a params object outlives the phase it was created in. Still, when #1716 wires this into a real render path, consider not memoizing across phases (or re-deriving the signal from state.abortController at throw time) so suspension always tracks the live controller.
|
Review submitted successfully. Here's a summary of what I found in PR #1715. Review summaryThe PR adds a fallback-shell render lifecycle shim plus thenable-params integration. I verified all three actionable items from the prior reviews are genuinely fixed, ran the targeted tests (31 passed), and traced the proxy/state-machine behavior. Confirmed fixes
Two new forward-looking notes (#1716, non-blocking)
Both are gated behind the test-only |
…oss phase transition Addresses follow-up bonk review on cloudflare#1715: - Add a per-state cache epoch, bumped on the warmup->final transition. Cache tasks capture the epoch at creation; a task that settles in a later epoch no longer decrements the freshly-reset pendingCacheTasks counter below zero (which would permanently block resolveCacheReadyIfSettled). - Re-derive the memoized fallback-shell hanging promise when the live AbortController changes (preparePprFallbackShellFinalRender swaps it on the phase transition), so suspension always tracks the controller the lifecycle will actually abort instead of a stale warmup signal. Adds tests for the late warmup-task settle and the post-transition re-derive.
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-reviewed the current checkout. This revision is in good shape and notably resolves both forward-looking #1716 notes from the prior bonk reviews — they are no longer just "latent," they're fixed here:
- Negative
pendingCacheTasks— now prevented by thecacheEpochmechanism.preparePprFallbackShellFinalRenderbumpscacheEpoch, andcompleteCacheTaskearly-returns (task.epoch !== state.cacheEpoch) for any task created in an earlier phase, so a late-settling warmup task can't decrement the reset counter below zero. Covered by the newdoes not drive pendingCacheTasks negative...test. - Stale
AbortControllerin memoizedfallbackShellPromise— now handled by re-deriving the hanging promise ingetFallbackShellPromisewheneverfallbackShellPromiseController !== fallbackShellState.abortController, so suspension always tracks the live controller across a warmup->final transition. Covered by the newre-derives fallback suspension against the live controller...test.
Verified locally:
vp test run tests/ppr-fallback-shell.test.ts tests/thenable-params.test.ts-> 33 passedvp checkon all 4 changed files -> clean (format + lint + types)- Confirmed proxy invariants hold: spread/
Object.keys/getOwnPropertyDescriptoron the fallback-key proxy route suspension through the traps rather than throwingTypeError, andawait paramsresolves to the proxy (non-thenable,thenis a well-known prop) with only fallback-key access suspending.
The cache-task ALS state machine is sound, and the lifecycle exports remain test-only (gated behind runWithPprFallbackShellState), matching the stated non-goals for #1716. One small non-blocking observation inline; no required changes.
| return fallbackShellPromise; | ||
| } | ||
| fallbackShellPromiseController = fallbackShellState.abortController; | ||
| fallbackShellPromise = createPprFallbackShellSuspensePromiseForState<T>( |
There was a problem hiding this comment.
Non-blocking observation (#1716): Re-deriving here correctly fixes the stale-controller bug, but each re-derivation also re-runs the full side effects in createPprFallbackShellSuspensePromiseForState — hasDynamicBoundary = true, re-iterating the cache-task stack to ignoreCacheTask, and scheduleCacheReadyIfSettled. For an escaped params object read after the original scope exits, pprFallbackShellCacheTaskStackAls.getStore() will be empty so the ignore loop is a harmless no-op, and hasDynamicBoundary/scheduleCacheReadyIfSettled against the new (final) phase is the intended effect. So this is correct today.
The only thing to keep in mind once #1716 wires real render paths: re-deriving on controller change means a single long-lived params object can mark hasDynamicBoundary on each phase it's read in, which is the desired behavior, but it does couple the params-object lifetime to the shell phase. Worth a deliberate confirmation that params objects are never intentionally reused across phases in the real serving layer (the current design assumes a fresh params object per render, which makes the memoization-by-controller a belt-and-suspenders safety net rather than a hot path). No change needed for this tests-only delta.
There was a problem hiding this comment.
Confirmed and intended. In both the warmup and final render paths a params object is constructed fresh per render via makeThenableParams — the App Router RSC entry builds the route param object as part of constructing the React tree for each render pass, so a single params object is never deliberately carried across a warmup->final transition.
The controller-keyed re-derivation is therefore a belt-and-suspenders safety net rather than a hot path: in the normal flow the warmup object is discarded before preparePprFallbackShellFinalRender runs, and the final render constructs a new one whose first fallback-key access derives the suspense against the live (final) controller anyway. The re-derive only matters for an escaped/long-lived object, where re-running the side effects against the current phase (marking hasDynamicBoundary, the no-op ignore loop on an empty cache-task stack, and scheduleCacheReadyIfSettled) is exactly the desired behavior.
When #1716 wires the real serving layer it will keep the per-render fresh-params-object invariant, so the memoization stays a safety net.
|
Review submitted successfully. Here's a summary of what I found in PR #1715. Review summaryThis revision is in good shape. The PR adds a fallback-shell render lifecycle shim plus thenable-params integration, all gated behind the test-only Key finding: prior #1716 notes are now actually fixedThe two forward-looking concerns from earlier bonk reviews are no longer just latent — this revision resolves both:
Verification
One non-blocking notePosted inline: the controller re-derivation re-runs The cache-task ALS state machine is sound. No blocking issues for this tests-only delta. |
2 participants
Stack position
PR 3 of the original
cacheComponentsPPR root-param fallback-shell stack.#1702 and #1714 are now merged, so this PR is rebased onto current
mainand should be reviewed as the next standalone delta: fallback-shell render lifecycle.Remaining follow-ups in the stack:
Problem
The intended invariant is not “wait for the whole SSR tree.”
For a root-param fallback shell, vinext should wait for public
"use cache"work that can complete for known params, but it should not block forever on work that depends on fallback params or request APIs such asheaders()/cookies().Known scoped params must remain synchronously readable. Fallback params should suspend only when they are actually accessed.
What changed
headers()/cookies()Contract covered
During fallback-shell prerendering:
This keeps the root-param shell invariant narrow: cached known-param content can be ready in the shell without converting the whole document into an
allReadywait.Deliberate non-goals
Those remain isolated in #1716.
Validation
After rebasing onto current
upstream/main:vp test run tests/ppr-fallback-shell.test.ts tests/thenable-params.test.tsvp checkReferences
References #1359