-
Notifications
You must be signed in to change notification settings - Fork 49.3k
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
Conversation
Comparing: 99e1024...16c46fa 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
|
@@ -3911,6 +3922,7 @@ function commitGestureOnRoot( | |||
pendingTransitionTypes, | |||
flushGestureMutations, | |||
flushGestureAnimations, | |||
reportViewTransitionError, |
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.
Does it make sense to run this in the context of finishedWork
or is that not a relevant Owner of a potential error?
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.
finishedWork
is just the HostRoot
so it has no owner at all.
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.
'This can happen if a Navigation is blocked on React itself. ' + | ||
"Such as if it's resolved inside useEffect. " + | ||
'This can be solved by moving the resolution to useLayoutEffect.', |
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.
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.
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.
sick
…2540) We customize the messages only in DEV to keep it small in prod. We skip some messages that are not really errors but more like information.
[diff facebook/react@0ca3deeb...6aa8254b](facebook/react@0ca3dee...6aa8254) <details> <summary>React upstream changes</summary> - facebook/react#32465 - facebook/react#32540 - facebook/react#32565 </details>
Follow up to #32540. We do allow gestures to be cancelled early (we call skipTransition) if the gesture stops before it has even started. This happens in the fixture when we auto-scroll.
We customize the messages only in DEV to keep it small in prod.
We skip some messages that are not really errors but more like information.