Repository navigation
Log errors from startViewTransition to onRecoverableError #32540
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3511,6 +3511,7 @@ function commitRoot( | |
| flushAfterMutationEffects, | ||
| flushSpawnedWork, | ||
| flushPassiveEffects, | ||
| reportViewTransitionError, | ||
| ); | ||
| if (!startedViewTransition) { | ||
| // Flush synchronously. | ||
|
|
@@ -3521,6 +3522,16 @@ function commitRoot( | |
| } | ||
| } | ||
|
|
||
| function reportViewTransitionError(error: mixed) { | ||
| // Report errors that happens while preparing a View Transition. | ||
| if (pendingEffectsStatus === NO_PENDING_EFFECTS) { | ||
| return; | ||
| } | ||
| const root = pendingEffectsRoot; | ||
| const onRecoverableError = root.onRecoverableError; | ||
| onRecoverableError(error, makeErrorInfo(null)); | ||
| } | ||
|
|
||
| function flushAfterMutationEffects(): void { | ||
| if (pendingEffectsStatus !== PENDING_AFTER_MUTATION_PHASE) { | ||
| return; | ||
|
|
@@ -3911,6 +3922,7 @@ function commitGestureOnRoot( | |
| pendingTransitionTypes, | ||
| flushGestureMutations, | ||
| flushGestureAnimations, | ||
| reportViewTransitionError, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it make sense to run this in the context of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In general these errors has no owner at all. There are other warnings we can add earlier to prevent some of these errors from happening in the first place. For example, we should have a warning when two ViewTransition components are mounted with the same name. Even if they don't happen to be part of the current transition and those could have owners. |
||
| ); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not following this one. Any chance we can manually trigger this scenario in the fixture?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this switches to useEffect instead of useLayoutEffect it should trigger it.
https://github.com/facebook/react/blob/main/fixtures/view-transition/src/components/App.js#L84-L86
Since the navigation should be blocked by the commit and the navigation blocks the commit from preceding the timing of unblocking the navigation needs to be in useLayoutEffect.