Bugfix: Nested useOpaqueIdentifier references #22553
Merged
Conversation
This file contains 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
We fire a warning in development if a component is updated during the render phase (with the exception of local hook updates, which have their own defined behavior). Because it's not a supported React pattern, we don't have that many tests that trigger this path. But it is meant to have reasonable semantics when it does happen, so that if it accidentally ships to production, the app doesn't crash unnecessarily. The behavior is not super well-defined, though. There are also some _internal_ React implementation details that intentionally to rely on this behavior. Most prominently, selective hydration and useOpaqueIdentifier. I need to tweak the behavior of render phase updates slightly as part of a fix for useOpaqueIdentifier. This shouldn't cause a user-facing change in behavior outside of useOpaqueIdentifier, but it does require that we explicitly model render phase updates.
facebook-github-bot
added
the
React Core Team
acdlite
changed the title
acdlite
commented
Oct 13, 2021
| @@ -294,7 +294,9 @@ let workInProgressRootIncludedLanes: Lanes = NoLanes; | |||
| // includes unprocessed updates, not work in bailed out children. | |||
| let workInProgressRootSkippedLanes: Lanes = NoLanes; | |||
| // Lanes that were updated (in an interleaved event) during this render. | |||
| let workInProgressRootUpdatedLanes: Lanes = NoLanes; | |||
| let workInProgressRootInterleavedUpdatedLanes: Lanes = NoLanes; | |||
There was a problem hiding this comment.
I split workInProgressRootUpdatedLanes into two separate variables that track "interleaved" updates (ones that originate from a regular DOM event, when React has yielded to the main thread) versus render phase updates (ones that originate from within a rendering component).
acdlite
commented
Oct 13, 2021
| if ( | ||
| (executionContext & RenderContext) !== NoLanes && | ||
| root === workInProgressRoot | ||
| ) { |
There was a problem hiding this comment.
I suggest reviewing this block with whitespace changes hidden; only a few lines changed: c71d3ab?diff=split&w=1
acdlite
commented
Oct 13, 2021
| let exitStatus; | ||
|
|
||
| const MAX_ERROR_RETRY_ATTEMPTS = 50; | ||
| for (let i = 0; i < MAX_ERROR_RETRY_ATTEMPTS; i++) { |
There was a problem hiding this comment.
This is the key part to review
|
Comparing: 8ee4ff8...aca9278 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
acdlite
force-pushed
the
fix-useopaqueidentifier
branch
from
1f7d980
to
9b06f14
Compare
sebmarkbage
approved these changes
Oct 13, 2021
acdlite
force-pushed
the
fix-useopaqueidentifier
branch
2 times, most recently
from
8a02664
to
dd448d3
Compare
Fixes an issue where multiple useOpaqueIdentifier hooks are upgraded to client ids within the same render. The way the upgrade works is that useOpaqueIdentifier schedules a render phase update then throws an error to trigger React's error recovery mechanism. The normal error recovery mechanism is designed for errors that occur as a result of interleaved mutations, so we usually only retry a single time, synchronously, before giving up. useOpaqueIdentifier is different because the error its throws when upgrading is not caused by an interleaved mutation. Rather, it happens when an ID is referenced for the first time inside a client-rendered tree (i.e. sommething that wasn't part of the initial server render). The fact that it relies on the error recovery mechanism is an implementation detail. And a single recovery attempt may be insufficient. For example, if a parent and a child component may reference different ids, and both are mounted as a result of the same client update, that will trigger two separate error recovery attempts. Because render phase updates are not allowed when triggered from userspace — we log a warning in developement to prevent them — we can assume that if something does update during the render phase, it is one of our "legit" implementation details like useOpaqueIdentifier. So we can keep retrying until we succeed — up to a limit, to protect against inifite loops. I chose 50 since that's the limit we use for commit phase updates.
acdlite
force-pushed
the
fix-useopaqueidentifier
branch
from
dd448d3
to
aca9278
Compare
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Nov 2, 2021
Summary: This sync includes the following changes: - **[5cccacd13](facebook/react@5cccacd13 )**: Upgrade useId to alpha channel ([#22674](facebook/react#22674)) //<Andrew Clark>// - **[75f3ddebf](facebook/react@75f3ddebf )**: Remove experimental useOpaqueIdentifier API ([#22672](facebook/react#22672)) //<Andrew Clark>// - **[8c4a05b8f](facebook/react@8c4a05b8f )**: Remove flow pragma comment from module registration start/stop templates ([#22670](facebook/react#22670)) //<Brian Vaughn>// - **[ebf9ae857](facebook/react@ebf9ae857 )**: useId ([#22644](facebook/react#22644)) //<Andrew Clark>// - **[a0d991fe6](facebook/react@a0d991fe6 )**: Re-land #22292 (remove uMS from open source build) ([#22664](facebook/react#22664)) //<Andrew Clark>// - **[6bce0355c](facebook/react@6bce0355c )**: Upgrade useSyncExternalStore to alpha channel ([#22662](facebook/react#22662)) //<Andrew Clark>// - **[7034408ff](facebook/react@7034408ff )**: Follow-up improvements to error code extraction infra ([#22516](facebook/react#22516)) //<Andrew Clark>// - **[90e5d3638](facebook/react@90e5d3638 )**: chore: fix comment typo ([#22615](facebook/react#22615)) //<btea>// - **[3c4c1c470](facebook/react@3c4c1c470 )**: Remove warning for dangling passive effects ([#22609](facebook/react#22609)) //<Andrew Clark>// - **[d5b6b4b86](facebook/react@d5b6b4b86 )**: Expand act warning to cover all APIs that might schedule React work ([#22607](facebook/react#22607)) //<Andrew Clark>// - **[fa9bea0c4](facebook/react@fa9bea0c4 )**: Initial implementation of cache cleanup ([#22510](facebook/react#22510)) //<Joseph Savona>// - **[0e8a5aff3](facebook/react@0e8a5aff3 )**: Scheduling Profiler: Add marks for component effects (mount and unmount) ([#22578](facebook/react#22578)) //<Brian Vaughn>// - **[4ba20579d](facebook/react@4ba20579d )**: Scheduling Profiler: De-emphasize React internal frames ([#22588](facebook/react#22588)) //<Brian Vaughn>// - **[cdb8a1d19](facebook/react@cdb8a1d19 )**: [Fizz] Add option to inject bootstrapping script tags after the shell is injected ([#22594](facebook/react#22594)) //<Sebastian Markbåge>// - **[34e4c9756](facebook/react@34e4c9756 )**: Clear extra nodes if there's a hydration mismatch within a suspense boundary ([#22592](facebook/react#22592)) //<Sebastian Markbåge>// - **[02f411578](facebook/react@02f411578 )**: Upgrade useInsertionEffect to stable ([#22589](facebook/react#22589)) //<Andrew Clark>// - **[2af4a7933](facebook/react@2af4a7933 )**: Hydrate using SuspenseComponent as the parent ([#22582](facebook/react#22582)) //<Sebastian Markbåge>// - **[b1acff0cc](facebook/react@b1acff0cc )**: Enable cache in test renderer ([#22580](facebook/react#22580)) //<Joseph Savona>// - **[996da67b2](facebook/react@996da67b2 )**: Replace global `jest` heuristic with `IS_REACT_ACT_ENVIRONMENT` ([#22562](facebook/react#22562)) //<Andrew Clark>// - **[163e81c1f](facebook/react@163e81c1f )**: Support disabling spurious act warnings with a global environment flag ([#22561](facebook/react#22561)) //<Andrew Clark>// - **[23b7dfeff](facebook/react@23b7dfeff )**: Enable scheduling profiler for RN FB profiling builds ([#22566](facebook/react#22566)) //<Brian Vaughn>// - **[61455a25b](facebook/react@61455a25b )**: Enable experimental Cache API in www TestRenderer ([#22554](facebook/react#22554)) //<Joseph Savona>// - **[7142d110b](facebook/react@7142d110b )**: Bugfix: Nested useOpaqueIdentifier references ([#22553](facebook/react#22553)) //<Andrew Clark>// - **[1e247ff89](facebook/react@1e247ff89 )**: Enabled scheduling profiler marks for React Native FB target ([#22544](facebook/react#22544)) //<Brian Vaughn>// - **[c16b005f2](facebook/react@c16b005f2 )**: Update test and stack frame code to support newer V8 stack formats ([#22477](facebook/react#22477)) //<Brian Vaughn>// - **[55d75005b](facebook/react@55d75005b )**: duplicate value in variable ([#22390](facebook/react#22390)) //<BIKI DAS>// Changelog: [General][Changed] - React Native sync for revisions afcb9cd...3fcd81d jest_e2e[run_all_tests] Reviewed By: yungsters Differential Revision: D32065987 fbshipit-source-id: ef2d402835c981aab68ca40a894c66c1630864e9
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Nov 15, 2021
Summary: This sync includes the following changes: - **[c0c71a868](facebook/react@c0c71a868 )**: Re-enable useMutableSource in internal RN ([#22750](facebook/react#22750)) //<Ricky>// - **[cb11155c8](facebook/react@cb11155c8 )**: Add runtime type checks around module boundary code ([#22748](facebook/react#22748)) //<Brian Vaughn>// - **[a04f13d29](facebook/react@a04f13d29 )**: react-refresh@0.11.0 //<Dan Abramov>// - **[ff9897d23](facebook/react@ff9897d23 )**: [React Refresh] support typescript namespace syntax ([#22621](facebook/react#22621)) //<irinakk>// - **[0ddd69d12](facebook/react@0ddd69d12 )**: Throw on hydration mismatch and force client rendering if boundary hasn't suspended within concurrent root ([#22629](facebook/react#22629)) //<salazarm>// - **[c3f34e4be](facebook/react@c3f34e4be )**: eslint-plugin-react-hooks@4.3.0 //<Dan Abramov>// - **[827021c4e](facebook/react@827021c4e )**: Changelog for eslint-plugin-react-hooks@4.3.0 //<Dan Abramov>// - **[8ca3f567b](facebook/react@8ca3f567b )**: Fix module-boundary wrappers ([#22688](facebook/react#22688)) //<Brian Vaughn>// - **[1bf6deb86](facebook/react@1bf6deb86 )**: Renamed packages/react-devtools-scheduling-profiler to packages/react-devtools-timeline ([#22691](facebook/react#22691)) //<Brian Vaughn>// - **[51c558aeb](facebook/react@51c558aeb )**: Rename (some) "scheduling profiler" references to "timeline" ([#22690](facebook/react#22690)) //<Brian Vaughn>// - **[00ced1e2b](facebook/react@00ced1e2b )**: Fix useId in strict mode ([#22681](facebook/react#22681)) //<Andrew Clark>// - **[5cccacd13](facebook/react@5cccacd13 )**: Upgrade useId to alpha channel ([#22674](facebook/react#22674)) //<Andrew Clark>// - **[75f3ddebf](facebook/react@75f3ddebf )**: Remove experimental useOpaqueIdentifier API ([#22672](facebook/react#22672)) //<Andrew Clark>// - **[8c4a05b8f](facebook/react@8c4a05b8f )**: Remove flow pragma comment from module registration start/stop templates ([#22670](facebook/react#22670)) //<Brian Vaughn>// - **[ebf9ae857](facebook/react@ebf9ae857 )**: useId ([#22644](facebook/react#22644)) //<Andrew Clark>// - **[a0d991fe6](facebook/react@a0d991fe6 )**: Re-land #22292 (remove uMS from open source build) ([#22664](facebook/react#22664)) //<Andrew Clark>// - **[6bce0355c](facebook/react@6bce0355c )**: Upgrade useSyncExternalStore to alpha channel ([#22662](facebook/react#22662)) //<Andrew Clark>// - **[7034408ff](facebook/react@7034408ff )**: Follow-up improvements to error code extraction infra ([#22516](facebook/react#22516)) //<Andrew Clark>// - **[90e5d3638](facebook/react@90e5d3638 )**: chore: fix comment typo ([#22615](facebook/react#22615)) //<btea>// - **[3c4c1c470](facebook/react@3c4c1c470 )**: Remove warning for dangling passive effects ([#22609](facebook/react#22609)) //<Andrew Clark>// - **[d5b6b4b86](facebook/react@d5b6b4b86 )**: Expand act warning to cover all APIs that might schedule React work ([#22607](facebook/react#22607)) //<Andrew Clark>// - **[fa9bea0c4](facebook/react@fa9bea0c4 )**: Initial implementation of cache cleanup ([#22510](facebook/react#22510)) //<Joseph Savona>// - **[0e8a5aff3](facebook/react@0e8a5aff3 )**: Scheduling Profiler: Add marks for component effects (mount and unmount) ([#22578](facebook/react#22578)) //<Brian Vaughn>// - **[4ba20579d](facebook/react@4ba20579d )**: Scheduling Profiler: De-emphasize React internal frames ([#22588](facebook/react#22588)) //<Brian Vaughn>// - **[cdb8a1d19](facebook/react@cdb8a1d19 )**: [Fizz] Add option to inject bootstrapping script tags after the shell is injected ([#22594](facebook/react#22594)) //<Sebastian Markbåge>// - **[34e4c9756](facebook/react@34e4c9756 )**: Clear extra nodes if there's a hydration mismatch within a suspense boundary ([#22592](facebook/react#22592)) //<Sebastian Markbåge>// - **[02f411578](facebook/react@02f411578 )**: Upgrade useInsertionEffect to stable ([#22589](facebook/react#22589)) //<Andrew Clark>// - **[2af4a7933](facebook/react@2af4a7933 )**: Hydrate using SuspenseComponent as the parent ([#22582](facebook/react#22582)) //<Sebastian Markbåge>// - **[b1acff0cc](facebook/react@b1acff0cc )**: Enable cache in test renderer ([#22580](facebook/react#22580)) //<Joseph Savona>// - **[996da67b2](facebook/react@996da67b2 )**: Replace global `jest` heuristic with `IS_REACT_ACT_ENVIRONMENT` ([#22562](facebook/react#22562)) //<Andrew Clark>// - **[163e81c1f](facebook/react@163e81c1f )**: Support disabling spurious act warnings with a global environment flag ([#22561](facebook/react#22561)) //<Andrew Clark>// - **[23b7dfeff](facebook/react@23b7dfeff )**: Enable scheduling profiler for RN FB profiling builds ([#22566](facebook/react#22566)) //<Brian Vaughn>// - **[61455a25b](facebook/react@61455a25b )**: Enable experimental Cache API in www TestRenderer ([#22554](facebook/react#22554)) //<Joseph Savona>// - **[7142d110b](facebook/react@7142d110b )**: Bugfix: Nested useOpaqueIdentifier references ([#22553](facebook/react#22553)) //<Andrew Clark>// - **[1e247ff89](facebook/react@1e247ff89 )**: Enabled scheduling profiler marks for React Native FB target ([#22544](facebook/react#22544)) //<Brian Vaughn>// - **[c16b005f2](facebook/react@c16b005f2 )**: Update test and stack frame code to support newer V8 stack formats ([#22477](facebook/react#22477)) //<Brian Vaughn>// - **[55d75005b](facebook/react@55d75005b )**: duplicate value in variable ([#22390](facebook/react#22390)) //<BIKI DAS>// Changelog: [General][Changed] - React Native sync for revisions afcb9cd...c0c71a8 jest_e2e[run_all_tests] Reviewed By: yungsters Differential Revision: D32395873 fbshipit-source-id: 3afd158f167b1eedcc244e29aba1a2c502d3c9d9
zhengjitf
pushed a commit
to zhengjitf/react
that referenced
this pull request
Apr 15, 2022
* Handle render phase updates explicitly We fire a warning in development if a component is updated during the render phase (with the exception of local hook updates, which have their own defined behavior). Because it's not a supported React pattern, we don't have that many tests that trigger this path. But it is meant to have reasonable semantics when it does happen, so that if it accidentally ships to production, the app doesn't crash unnecessarily. The behavior is not super well-defined, though. There are also some _internal_ React implementation details that intentionally to rely on this behavior. Most prominently, selective hydration and useOpaqueIdentifier. I need to tweak the behavior of render phase updates slightly as part of a fix for useOpaqueIdentifier. This shouldn't cause a user-facing change in behavior outside of useOpaqueIdentifier, but it does require that we explicitly model render phase updates. * Bugfix: Nested useOpaqueIdentifier calls Fixes an issue where multiple useOpaqueIdentifier hooks are upgraded to client ids within the same render. The way the upgrade works is that useOpaqueIdentifier schedules a render phase update then throws an error to trigger React's error recovery mechanism. The normal error recovery mechanism is designed for errors that occur as a result of interleaved mutations, so we usually only retry a single time, synchronously, before giving up. useOpaqueIdentifier is different because the error its throws when upgrading is not caused by an interleaved mutation. Rather, it happens when an ID is referenced for the first time inside a client-rendered tree (i.e. sommething that wasn't part of the initial server render). The fact that it relies on the error recovery mechanism is an implementation detail. And a single recovery attempt may be insufficient. For example, if a parent and a child component may reference different ids, and both are mounted as a result of the same client update, that will trigger two separate error recovery attempts. Because render phase updates are not allowed when triggered from userspace — we log a warning in developement to prevent them — we can assume that if something does update during the render phase, it is one of our "legit" implementation details like useOpaqueIdentifier. So we can keep retrying until we succeed — up to a limit, to protect against inifite loops. I chose 50 since that's the limit we use for commit phase updates.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes an issue where multiple useOpaqueIdentifier hooks are upgraded to client ids within the same render.
The way the upgrade works is that useOpaqueIdentifier schedules a render phase update then throws an error to trigger React's error recovery mechanism.
The normal error recovery mechanism is designed for errors that occur as a result of interleaved mutations, so we usually only retry a single time, synchronously, before giving up.
useOpaqueIdentifier is different because the error its throws when upgrading is not caused by an interleaved mutation. Rather, it happens when an ID is referenced for the first time inside a client-rendered tree (i.e. something that wasn't part of the initial server render). The fact that it relies on the error recovery mechanism is an implementation detail. And a single recovery attempt may be insufficient. For example, if a parent and a child component may reference different ids, and both are mounted as a result of the same client update, that will trigger two separate error recovery attempts.
Because render phase updates are not allowed when triggered from userspace — we log a warning in development to prevent them — we can assume that if something does update during the render phase, it is one of our "legit" implementation details like useOpaqueIdentifier. So we can keep retrying until we succeed — up to a limit, to protect against infinite loops. I chose 50 since that's the limit we use for commit phase updates.