Avoid proxying React modules through workUnitStore (#85486)#8
Open
MitchLewis930 wants to merge 1 commit intopr_038_beforefrom
Open
Avoid proxying React modules through workUnitStore (#85486)#8MitchLewis930 wants to merge 1 commit intopr_038_beforefrom
MitchLewis930 wants to merge 1 commit intopr_038_beforefrom
Conversation
Today the `captureOwnerStack()` function is provided to shared utilities through an AsyncLocalStorage that scopes the method from the appropriate React instance. This is so that external code like patches to sync IO methods can still generate errors with the appropriate React owner information even when the patched code itself is not bundled and can be called from etiher SSR or RSC contexts. This works but it makes plumbing the React instances around tricky. There is a simpler way. Most of the time you can just try both React's. If one gives you a non-null/undefined result then you know you are in that scope. If neither do then you're outside a React scope altogether. In this change I remove `captureOwnerStack()` from the workUnitStore types and just call it from the shared server runtime which gives even external code access to the appropriate React instances for bundled code
There was a problem hiding this comment.
Pull request overview
This PR refactors how React module instances are accessed in external modules by introducing a centralized registration system instead of proxying through the workUnitStore.
Changes:
- Introduced
runtime-reacts.external.tsto provide centralized getter/setter functions for client and server React instances - Removed
captureOwnerStackandcacheSignalfromworkUnitStore, accessing them directly through the registered React instances instead - Updated registration logic in
app-page/module.tsto use the new centralized system
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/next/src/server/runtime-reacts.external.ts | New file providing registration and getter functions for client/server React instances |
| packages/next/src/server/route-modules/app-page/module.ts | Updated to register React instances using the new centralized system |
| packages/next/src/server/node-environment-extensions/console-dim.external.tsx | Refactored to access React's cacheSignal through centralized getters instead of registered callbacks |
| packages/next/src/server/node-environment-extensions/utils.tsx | Updated applyOwnerStack to access captureOwnerStack through centralized React getters |
| packages/next/src/server/app-render/work-unit-async-storage.external.ts | Removed captureOwnerStack property from PrerenderStoreModernCommon interface |
| packages/next/src/server/app-render/app-render.tsx | Removed captureOwnerStack assignments from work unit store initializations |
| packages/next/src/server/route-modules/app-route/module.ts | Removed captureOwnerStack assignments from work unit store initializations |
| packages/next/src/server/node-environment-extensions/console-dim.external.test.ts | Updated test to use new registration API with mock React instances |
| test/production/next-server-nft/next-server-nft.test.ts | Added new runtime-reacts.external.js file to expected NFT output |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+189
to
+190
| const signal = | ||
| getClientReact()?.cacheSignal() ?? getServerReact()?.cacheSignal() |
There was a problem hiding this comment.
The logic change removes the loop that checked multiple signals, but doesn't handle the case where both getClientReact() and getServerReact() return non-null React instances. Previously, the code would iterate through all registered signals until finding one. Now it will only use the first non-null result. Consider whether both React instances could be active simultaneously and whether the fallback logic is correct.
Suggested change
| const signal = | |
| getClientReact()?.cacheSignal() ?? getServerReact()?.cacheSignal() | |
| const clientReact = getClientReact() | |
| const serverReact = getServerReact() | |
| const clientSignal = clientReact?.cacheSignal?.() | |
| const serverSignal = serverReact?.cacheSignal?.() | |
| let signal = | |
| clientSignal !== null && clientSignal !== undefined | |
| ? clientSignal | |
| : serverSignal !== null && serverSignal !== undefined | |
| ? serverSignal | |
| : null | |
| if ( | |
| clientSignal && | |
| serverSignal && | |
| clientSignal.aborted !== serverSignal.aborted | |
| ) { | |
| // If both client and server signals are present but disagree on `aborted`, | |
| // prefer the aborted one so that logs are dimmed/hidden conservatively. | |
| signal = clientSignal.aborted ? clientSignal : serverSignal | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR_038