Refactor: scope requestStore to dynamic renders only (#72312)#5
Refactor: scope requestStore to dynamic renders only (#72312)#5MitchLewis930 wants to merge 1 commit intopr_035_beforefrom
Conversation
This completes the refactor to eliminate a requestStore scoped around prerenders. Now we only scope requestStore around dynamic renders. If you are prerendering then the workUnitAsyncStorage will only have a prerneder store or undefined. While it is possible to shadow stores because you can enter a cache scope from a render or prerender it generally should never be the case that you enter a prerender from a request or enter a request from a prerender. These are effectively top level scopes. This should not change any program behavior stacked on vercel#72212
There was a problem hiding this comment.
Pull request overview
This PR refactors the requestStore initialization to be scoped only to dynamic renders, rather than being created at the outer scope for all render types. The requestStore is now created within the dynamic rendering path and explicitly passed to functions that need it.
Changes:
- Moved requestStore creation from outer scope to dynamic render path
- Updated function signatures to explicitly accept requestStore parameter
- Wrapped additional async operations with workUnitAsyncStorage.run() for proper context propagation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/next/src/server/async-storage/request-store.ts | Reordered parameters in createRequestStoreForRender function signature |
| packages/next/src/server/app-render/app-render.tsx | Moved requestStore creation to dynamic render path and updated function calls to pass requestStore explicitly |
| packages/next/src/server/app-render/action-handler.ts | Updated function calls to accept and pass requestStore parameter, wrapped operations with workUnitAsyncStorage.run() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) { | ||
| const setAppIsrStatus = renderOpts.setAppIsrStatus | ||
| req.originalRequest.on('end', () => { | ||
| if (!requestStore.usedDynamic && !workStore.forceDynamic) { |
There was a problem hiding this comment.
The refactored ISR status logic no longer checks for PPR (Partial Prerendering) conditions. The original code at line 1050-1070 included a check for isPPR using prerenderStore and dynamicAccesses, which would prevent setting ISR status when PPR was active. This condition has been removed, potentially causing incorrect ISR status updates when PPR is in use.
| if (!requestStore.usedDynamic && !workStore.forceDynamic) { | |
| // When Partial Prerendering (PPR) is active for this request, we must | |
| // not mark the route as ISR-capable, to avoid incorrect ISR status. | |
| const prerenderStore = workUnitAsyncStorage.getStore() as | |
| | PrerenderStore | |
| | undefined | |
| const isPPR = !!prerenderStore?.isPPR | |
| if (!isPPR && !requestStore.usedDynamic && !workStore.forceDynamic) { |
| workStore, | ||
| // The function to run | ||
| renderToHTMLOrFlightImpl, | ||
| // all of it's args |
There was a problem hiding this comment.
Corrected spelling of 'it's' to 'its' in the comment.
| // all of it's args | |
| // all of its args |
2 participants
PR_035