-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
Add onRecoverableError option to hydrateRoot, createRoot #23207
Conversation
6af98f4
to
17f66a9
Compare
Comparing: 5318971...24abe60 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
|
fe8c95f
to
ba4e106
Compare
ba4e106
to
e6a1df0
Compare
error: mixed, | ||
): void { | ||
const onRecoverableError = config; | ||
if (onRecoverableError !== null) { |
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.
This feels like a reconciler specific concern to me. Especially since this is likely to grow with other types of callbacks that will have more advanced semantics than this callback will handle.
One case I'd expect we might want is to scope this to subtree. Like error boundaries but for recoverable errors.
The reconciler can make this choice and schedule an IdlePriority callback itself if it wants to. Although it feels a bit presumptuous to do that for the user since you can schedule it in the callback. Although paternalism is on brand for us I guess.
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.
Yeah I went back and forth on that. The main reason I ended up adding a host config is because it needs a default fallback behavior; didn't feel right to assume that throwing in a Scheduler task is necessarily the correct default for each environment (although I suppose that's what we do for uncaught errors). An alternative is to resolve the default option within the renderer before passing it to the root constructor but that feels like a superficial difference. Don't mind either way.
Related comment:
react/packages/react-reconciler/src/ReactFiberRoot.new.js
Lines 114 to 117 in e6a1df0
// TODO: We have several of these arguments that are conceptually part of the | |
// host config, but because they are passed in at runtime, we have to thread | |
// them through the root constructor. Perhaps we should put them all into a | |
// single type, like a DynamicHostConfig that is defined by the renderer. |
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.
Yea I don't think it's always correct to throw in a task so having host config makes sense for the default fallback behavior. It's the main API behavior that doesn't seem right to determine in the host config. In other words, resolving the default option within the renderer would make sense. That also avoids leaking the type to all the host configs.
// Default behavior is to rethrow the error in a separate task. This will | ||
// trigger a browser error event. | ||
scheduleCallback(IdlePriority, () => { | ||
throw 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.
There's a handleErrorInNextTick function in this file that has different semantics but I guess is similarly a fallback default behavior? Should they be unified?
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.
Looks like that method is only used by the queueMicrotask
fallback... I'm unsure why that's necessary, to be honest. Feels wrong.
@@ -374,6 +379,25 @@ export function getCurrentEventPriority(): * { | |||
return getEventPriority(currentEvent.type); | |||
} | |||
|
|||
export function logRecoverableError( |
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.
For my own memory later, the reason this wasn't already something that existed for throwing top level is because other top level errors are handled by throwing at the root of the work loop and letting whatever is above handle it.
config: ErrorLoggingConfig, | ||
error: mixed, | ||
): void { | ||
// noop |
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.
console.error or something maybe to start with?
// probably want to log any error that is recovered from without | ||
// triggering an error boundary — or maybe even those, too. Need to | ||
// figure out the right API. | ||
logRecoverableError(root.errorLoggingConfig, value); |
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 think we probably don't want to log errors to this if they also end up surfacing a user error afterwards. It seems like kind of a misnomer in the API if it also gets passed errors that end up erroring. Even though you could maybe try to dedupe them this seems tricky to do in user space. The idle callback (if we keep it) also makes this even trickier since the error boundaries are not idle.
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 agree but I can't think of a reliable way to implement it that doesn't fail in some edge case. Once it falls back to client rendering, we can't know for sure if a subsequent error is the same one or a different one. So here I erred on the side of logging too much.
To argue for the other approach, you could say that when that situation does happen, it's ok that the first error gets unintentionally silenced because it will be unsilenced after the second error is fixed.
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.
It could be a follow up but another thing missing here is the scenario where the Suspense boundary was in "client render mode" or ended up being put in client render mode by the server. I.e. a server errored.
Similarly, there's still this case where an update forces a hydration but if it can't get the bumped priority or that one failed last time. Then it gets client rendered without an error on either side but that's arguably an error that we can create and pass to the callback.
This case in particular suggests that we might want to log this upon commit.
This one I was planning to do in a follow-up. Could also push to a minor release since at least they are already logged on the server.
Ah yeah forgot about that one. I can add that. |
Scheduler.unstable_flushAll(); | ||
// Hydration error is logged | ||
expect(Scheduler).toFlushAndYield([ | ||
'An error occurred during hydration. The server HTML was replaced ' + |
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.
This is actually not a real error. What happens is that since <Child>
suspended the hydration pointer is off for the remainder where we "attempt to hydrate" (Really we're just "warming up" the components instead of bailing) so everything after a component suspends leads to a hydration mismatch.
5f37862
to
073ecfd
Compare
function commitRootImpl(root, renderPriorityLevel) { | ||
function commitRootImpl( | ||
root: FiberRoot, | ||
recoverableErrors: null | Array<mixed>, |
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.
This is kind of annoying but there's no obvious place to store these. We don't usually pass things directly from the render phase to the commit phase because we stash it on a fiber.
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 don't get this part. All calls to commitRoot call this with the global workInProgressRootRecoverableErrors
which you have access to here too. Other workInProgress
globals are accessed here too.
So can't you just avoid passing it and instead access workInProgressRootRecoverableErrors
here?
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.
There's an annoying edge case when both 1) you're in a multi-root app 2) the commit phase is delayed with setTimeout
(this used to be more common but now it's really just the Suspense train model). During the timeout delay, the next root can start rendering. So you can't access work loop variables directly; you need to stash data somewhere in the fiber tree or on the FiberRoot.
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.
Ah yes, and with suspense images probably. That is annoying.
This is not the final API but I'm pushing it for discussion purposes. When an error is thrown during hydration, we fallback to client rendering, without triggering an error boundary. This is good because, in many cases, the UI will recover and the user won't even notice that something has gone wrong behind the scenes. However, we shouldn't recover from these errors silently, because the underlying cause might be pretty serious. Server-client mismatches are not supposed to happen, even if UI doesn't break from the users perspective. Ignoring them could lead to worse problems later. De-opting from server to client rendering could also be a significant performance regression, depending on the scope of the UI it affects. So we need a way to log when hydration errors occur. This adds a new option for `hydrateRoot` called `onHydrationError`. It's symmetrical to the server renderer's `onError` option, and serves the same purpose. When no option is provided, the default behavior is to schedule a browser task and rethrow the error. This will trigger the normal browser behavior for errors, including dispatching an error event. If the app already has error monitoring, this likely will just work as expected without additional configuration. However, we can also expose additional metadata about these errors, like which Suspense boundaries were affected by the de-opt to client rendering. (I have not exposed any metadata in this commit; API needs more design work.) There are other situations besides hydration where we recover from an error without surfacing it to the user, or notifying an error boundary. For example, if an error occurs during a concurrent render, it could be due to a data race, so we try again synchronously in case that fixes it. We should probably expose a way to log these types of errors, too. (Also not implemented in this commit.)
This expands the scope of onHydrationError to include all errors that are not surfaced to the UI (an error boundary). In addition to errors that occur during hydration, this also includes errors that recoverable by de-opting to synchronous rendering. Typically (or really, by definition) these errors are the result of a concurrent data race; blocking the main thread fixes them by prevents subsequent races. The logic for de-opting to synchronous rendering already existed. The only thing that has changed is that we now log the errors instead of silently proceeding. The logging API has been renamed from onHydrationError to onRecoverableError.
If the render is interrupted and restarts, we don't want to log the errors multiple times. This change only affects errors that are recovered by de-opting to synchronous rendering; we'll have to do something else for errors during hydration, since they use a different recovery path.
073ecfd
to
667e431
Compare
Similar to previous step. When an error occurs during hydration, we only want to log it if falling back to client rendering _succeeds_. If client rendering fails, the error will get reported to the nearest error boundary, so there's no need for a duplicate log. To implement this, I added a list of errors to the hydration context. If the Suspense boundary successfully completes, they are added to the main recoverable errors queue (the one I added in the previous step.)
408c5f8
to
1ee9f60
Compare
If onRecoverableError is not provided, we default to rethrowing the error in a separate task. Originally, I scheduled the task with idle priority, but @sebmarkbage made the good point that if there are multiple errors logs, we want to preserve the original order. So I've switched it to a microtask. The priority can be lowered in userspace by scheduling an additional task inside onRecoverableError.
Redefines the contract of the host config's logRecoverableError method to be a default implementation for onRecoverableError if a user-provided one is not provided when the root is created.
@@ -2091,6 +2106,15 @@ function commitRootImpl(root, renderPriorityLevel) { | |||
// additional work on this root is scheduled. | |||
ensureRootIsScheduled(root, now()); | |||
|
|||
if (recoverableErrors !== null) { |
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.
The timing of these are interesting given that we were talking about logging order. If it's uncaught it'll be logged after this, but if it's caught the logging will happen in an error boundary who is supposed to log it in componentDidCatch which is a layout effect.
So it seems to me that this needs to be logged before the layout effect phase.
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.
That's a good point, though I don't know if we can be sure of the order unless we start tracking the recoverable errors per subtree. If there were recoverable errors and regular errors (componentDidCatch
) in the same commit, the current implementation can't tell which happened first because the recoverable error array is shared across the whole tree.
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.
Yea but if they're separate subtrees they're more likely independent and we could've rendered them in any order.
The cases I'm mostly thinking about is when you have something like a hydration that succeeded and then errored because it wasn't quite fixed. However, I can't think of a case where that wouldn't be two separate consecutive commits.
That would be more of an issue with the throw vs next task throw thing, but not this case.
@@ -601,10 +607,28 @@ function resetHydrationState(): void { | |||
isHydrating = false; | |||
} | |||
|
|||
export function queueRecoverableHydrationErrors(): void { |
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.
These two methods sound the same. Maybe "apply" or "upstream"...?
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.
upgradeHydrationErrorsToRecoverable?
// Default behavior is to rethrow the error in a separate task. This will | ||
// trigger a browser error event. | ||
queueMicrotask(() => { | ||
throw 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.
Any worries about the Safari bug that can execute this on unclean stack if iframe is added?
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.
Sigh. Yeah.
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 don't really know what the workaround is, though, other than to avoid queueMicrotask. I'm tempted to leave it as-is and leave it to Safari to fix their shit.
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.
Maybe a user space one. It doesn't catch on break etc. but gives you something in the DEV console and in prod.
console.error(...);
window.dispatchEvent(new ErrorEvent(...));
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.
Is one of those options better than the other?
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.
It seems to me that queueMicrotask doesn't actually solve the problem, does it? Because the rethrown error will be logged before the microtask. So in the default case, the order is still hard error followed by recovered error. So there's no difference from postTask.
Where as the user space log + dispatch would be recovered error followed by hard 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.
If you have a recovered error and then a useLayoutEffect or anything causing a second commit in the same task, that then errors because the recovering didn't quite work. Then recovered errors should ideally come first.
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.
Ah when we were discussing error ordering I was thinking about other errors that happen in subsequent events, not the hard error that we throw at the top when there's no error boundary. (I don't think about those usually because I assume most apps have a top-level error boundary.) That makes sense.
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 think everything else is nits or possible follow ups.
I'm going to collect the follow ups here so I don't forget: Will do these before merging:
Will do these as follow ups:
|
537d1ca
to
2d8ef49
Compare
In modern browsers, reportError will dispatch an error event, emulating an uncaught JavaScript error. We can do this instead of rethrowing recoverable errors in a microtask, which is nice because it avoids any subtle ordering issues. In older browsers and test environments, we'll fall back to console.error.
queueRecoverableHydrationErrors -> upgradeHydrationErrorsToRecoverable
2d8ef49
to
24abe60
Compare
should we add as followups:
|
Summary: This sync includes the following changes: - **[a3bde7974](facebook/react@a3bde7974 )**: Exclude react-dom/unstable_testing entry point from stable releases ([#23258](facebook/react#23258)) //<Sebastian Markbåge>// - **[569093276](facebook/react@569093276 )**: Add onErrorShell Callback ([#23247](facebook/react#23247)) //<Sebastian Markbåge>// - **[0dedfcc68](facebook/react@0dedfcc68 )**: Update the exports field ([#23257](facebook/react#23257)) //<Sebastian Markbåge>// - **[9d4e8e84f](facebook/react@9d4e8e84f )**: React Native raw event EventEmitter - intended for app-specific perf listeners and debugging ([#23232](facebook/react#23232)) //<Joshua Gross>// - **[1dece5235](facebook/react@1dece5235 )**: Add back warning with component stack on Hydration mismatch ([#23241](facebook/react#23241)) //<salazarm>// - **[cd4eb116c](facebook/react@cd4eb116c )**: Revert "update node.js version for CI ([#23236](facebook/react#23236))" ([#23239](facebook/react#23239)) //<dan>// - **[1d7728bf9](facebook/react@1d7728bf9 )**: update node.js version for CI ([#23236](facebook/react#23236)) //<sunderls>// - **[848e802d2](facebook/react@848e802d2 )**: Add onRecoverableError option to hydrateRoot, createRoot ([#23207](facebook/react#23207)) //<Andrew Clark>// - **[5318971f5](facebook/react@5318971f5 )**: Remove logic for multiple error recovery attempts ([#23227](facebook/react#23227)) //<Andrew Clark>// - **[3a4462129](facebook/react@3a4462129 )**: Disable avoidThisFallback support in Fizz ([#23224](facebook/react#23224)) //<salazarm>// - **[0318ac2c4](facebook/react@0318ac2c4 )**: Revert 4f5449 //<Ricky>// - **[4f5449eb4](facebook/react@4f5449eb4 )**: Remove main from scheduler `index.js` //<Ricky>// - **[3f5ff16c1](facebook/react@3f5ff16c1 )**: [Hydration] Fallback to client render if server rendered extra nodes ([#23176](facebook/react#23176)) //<salazarm>// - **[529dc3ce8](facebook/react@529dc3ce8 )**: Fix context providers in SSR when handling multiple requests ([#23171](facebook/react#23171)) //<Fran Dios>// - **[505c15c9e](facebook/react@505c15c9e )**: Don't inject timeline hooks unless React supports profiling ([#23151](facebook/react#23151)) //<Brian Vaughn>// - **[e12a9dfc9](facebook/react@e12a9dfc9 )**: Fix production-only updateSyncExternalStore() crash when doing setState in render ([#23150](facebook/react#23150)) //<Douglas Armstrong>// - **[e48940255](facebook/react@e48940255 )**: Warn when a callback ref returns a function ([#23145](facebook/react#23145)) //<Dan Abramov>// - **[d8cfeaf22](facebook/react@d8cfeaf22 )**: Fix context propagation for offscreen/fallback trees ([#23095](facebook/react#23095)) //<Dan Abramov>// - **[d50482478](facebook/react@d50482478 )**: Enable scheduling profiler flag in react-dom/testing builds ([#23142](facebook/react#23142)) //<Brian Vaughn>// - **[790b5246f](facebook/react@790b5246f )**: Fix setState ignored in Safari when iframe is added to DOM in the same commit ([#23111](facebook/react#23111)) //<Dan Abramov>// Changelog: [General][Changed] - React Native sync for revisions 51947a1...a3bde79 jest_e2e[run_all_tests] Reviewed By: ShikaSD Differential Revision: D34108924 fbshipit-source-id: 96acb66c1a7da79d6ef9403490cd0e29ad23d9fb
) * [RFC] Add onHydrationError option to hydrateRoot This is not the final API but I'm pushing it for discussion purposes. When an error is thrown during hydration, we fallback to client rendering, without triggering an error boundary. This is good because, in many cases, the UI will recover and the user won't even notice that something has gone wrong behind the scenes. However, we shouldn't recover from these errors silently, because the underlying cause might be pretty serious. Server-client mismatches are not supposed to happen, even if UI doesn't break from the users perspective. Ignoring them could lead to worse problems later. De-opting from server to client rendering could also be a significant performance regression, depending on the scope of the UI it affects. So we need a way to log when hydration errors occur. This adds a new option for `hydrateRoot` called `onHydrationError`. It's symmetrical to the server renderer's `onError` option, and serves the same purpose. When no option is provided, the default behavior is to schedule a browser task and rethrow the error. This will trigger the normal browser behavior for errors, including dispatching an error event. If the app already has error monitoring, this likely will just work as expected without additional configuration. However, we can also expose additional metadata about these errors, like which Suspense boundaries were affected by the de-opt to client rendering. (I have not exposed any metadata in this commit; API needs more design work.) There are other situations besides hydration where we recover from an error without surfacing it to the user, or notifying an error boundary. For example, if an error occurs during a concurrent render, it could be due to a data race, so we try again synchronously in case that fixes it. We should probably expose a way to log these types of errors, too. (Also not implemented in this commit.) * Log all recoverable errors This expands the scope of onHydrationError to include all errors that are not surfaced to the UI (an error boundary). In addition to errors that occur during hydration, this also includes errors that recoverable by de-opting to synchronous rendering. Typically (or really, by definition) these errors are the result of a concurrent data race; blocking the main thread fixes them by prevents subsequent races. The logic for de-opting to synchronous rendering already existed. The only thing that has changed is that we now log the errors instead of silently proceeding. The logging API has been renamed from onHydrationError to onRecoverableError. * Don't log recoverable errors until commit phase If the render is interrupted and restarts, we don't want to log the errors multiple times. This change only affects errors that are recovered by de-opting to synchronous rendering; we'll have to do something else for errors during hydration, since they use a different recovery path. * Only log hydration error if client render succeeds Similar to previous step. When an error occurs during hydration, we only want to log it if falling back to client rendering _succeeds_. If client rendering fails, the error will get reported to the nearest error boundary, so there's no need for a duplicate log. To implement this, I added a list of errors to the hydration context. If the Suspense boundary successfully completes, they are added to the main recoverable errors queue (the one I added in the previous step.) * Log error with queueMicrotask instead of Scheduler If onRecoverableError is not provided, we default to rethrowing the error in a separate task. Originally, I scheduled the task with idle priority, but @sebmarkbage made the good point that if there are multiple errors logs, we want to preserve the original order. So I've switched it to a microtask. The priority can be lowered in userspace by scheduling an additional task inside onRecoverableError. * Only use host config method for default behavior Redefines the contract of the host config's logRecoverableError method to be a default implementation for onRecoverableError if a user-provided one is not provided when the root is created. * Log with reportError instead of rethrowing In modern browsers, reportError will dispatch an error event, emulating an uncaught JavaScript error. We can do this instead of rethrowing recoverable errors in a microtask, which is nice because it avoids any subtle ordering issues. In older browsers and test environments, we'll fall back to console.error. * Naming nits queueRecoverableHydrationErrors -> upgradeHydrationErrorsToRecoverable
) * [RFC] Add onHydrationError option to hydrateRoot This is not the final API but I'm pushing it for discussion purposes. When an error is thrown during hydration, we fallback to client rendering, without triggering an error boundary. This is good because, in many cases, the UI will recover and the user won't even notice that something has gone wrong behind the scenes. However, we shouldn't recover from these errors silently, because the underlying cause might be pretty serious. Server-client mismatches are not supposed to happen, even if UI doesn't break from the users perspective. Ignoring them could lead to worse problems later. De-opting from server to client rendering could also be a significant performance regression, depending on the scope of the UI it affects. So we need a way to log when hydration errors occur. This adds a new option for `hydrateRoot` called `onHydrationError`. It's symmetrical to the server renderer's `onError` option, and serves the same purpose. When no option is provided, the default behavior is to schedule a browser task and rethrow the error. This will trigger the normal browser behavior for errors, including dispatching an error event. If the app already has error monitoring, this likely will just work as expected without additional configuration. However, we can also expose additional metadata about these errors, like which Suspense boundaries were affected by the de-opt to client rendering. (I have not exposed any metadata in this commit; API needs more design work.) There are other situations besides hydration where we recover from an error without surfacing it to the user, or notifying an error boundary. For example, if an error occurs during a concurrent render, it could be due to a data race, so we try again synchronously in case that fixes it. We should probably expose a way to log these types of errors, too. (Also not implemented in this commit.) * Log all recoverable errors This expands the scope of onHydrationError to include all errors that are not surfaced to the UI (an error boundary). In addition to errors that occur during hydration, this also includes errors that recoverable by de-opting to synchronous rendering. Typically (or really, by definition) these errors are the result of a concurrent data race; blocking the main thread fixes them by prevents subsequent races. The logic for de-opting to synchronous rendering already existed. The only thing that has changed is that we now log the errors instead of silently proceeding. The logging API has been renamed from onHydrationError to onRecoverableError. * Don't log recoverable errors until commit phase If the render is interrupted and restarts, we don't want to log the errors multiple times. This change only affects errors that are recovered by de-opting to synchronous rendering; we'll have to do something else for errors during hydration, since they use a different recovery path. * Only log hydration error if client render succeeds Similar to previous step. When an error occurs during hydration, we only want to log it if falling back to client rendering _succeeds_. If client rendering fails, the error will get reported to the nearest error boundary, so there's no need for a duplicate log. To implement this, I added a list of errors to the hydration context. If the Suspense boundary successfully completes, they are added to the main recoverable errors queue (the one I added in the previous step.) * Log error with queueMicrotask instead of Scheduler If onRecoverableError is not provided, we default to rethrowing the error in a separate task. Originally, I scheduled the task with idle priority, but @sebmarkbage made the good point that if there are multiple errors logs, we want to preserve the original order. So I've switched it to a microtask. The priority can be lowered in userspace by scheduling an additional task inside onRecoverableError. * Only use host config method for default behavior Redefines the contract of the host config's logRecoverableError method to be a default implementation for onRecoverableError if a user-provided one is not provided when the root is created. * Log with reportError instead of rethrowing In modern browsers, reportError will dispatch an error event, emulating an uncaught JavaScript error. We can do this instead of rethrowing recoverable errors in a microtask, which is nice because it avoids any subtle ordering issues. In older browsers and test environments, we'll fall back to console.error. * Naming nits queueRecoverableHydrationErrors -> upgradeHydrationErrorsToRecoverable
Summary: This sync includes the following changes: - **[a3bde7974](facebook/react@a3bde7974 )**: Exclude react-dom/unstable_testing entry point from stable releases ([facebook#23258](facebook/react#23258)) //<Sebastian Markbåge>// - **[569093276](facebook/react@569093276 )**: Add onErrorShell Callback ([facebook#23247](facebook/react#23247)) //<Sebastian Markbåge>// - **[0dedfcc68](facebook/react@0dedfcc68 )**: Update the exports field ([facebook#23257](facebook/react#23257)) //<Sebastian Markbåge>// - **[9d4e8e84f](facebook/react@9d4e8e84f )**: React Native raw event EventEmitter - intended for app-specific perf listeners and debugging ([facebook#23232](facebook/react#23232)) //<Joshua Gross>// - **[1dece5235](facebook/react@1dece5235 )**: Add back warning with component stack on Hydration mismatch ([facebook#23241](facebook/react#23241)) //<salazarm>// - **[cd4eb116c](facebook/react@cd4eb116c )**: Revert "update node.js version for CI ([facebook#23236](facebook/react#23236))" ([facebook#23239](facebook/react#23239)) //<dan>// - **[1d7728bf9](facebook/react@1d7728bf9 )**: update node.js version for CI ([facebook#23236](facebook/react#23236)) //<sunderls>// - **[848e802d2](facebook/react@848e802d2 )**: Add onRecoverableError option to hydrateRoot, createRoot ([facebook#23207](facebook/react#23207)) //<Andrew Clark>// - **[5318971f5](facebook/react@5318971f5 )**: Remove logic for multiple error recovery attempts ([facebook#23227](facebook/react#23227)) //<Andrew Clark>// - **[3a4462129](facebook/react@3a4462129 )**: Disable avoidThisFallback support in Fizz ([facebook#23224](facebook/react#23224)) //<salazarm>// - **[0318ac2c4](facebook/react@0318ac2c4 )**: Revert 4f5449 //<Ricky>// - **[4f5449eb4](facebook/react@4f5449eb4 )**: Remove main from scheduler `index.js` //<Ricky>// - **[3f5ff16c1](facebook/react@3f5ff16c1 )**: [Hydration] Fallback to client render if server rendered extra nodes ([facebook#23176](facebook/react#23176)) //<salazarm>// - **[529dc3ce8](facebook/react@529dc3ce8 )**: Fix context providers in SSR when handling multiple requests ([facebook#23171](facebook/react#23171)) //<Fran Dios>// - **[505c15c9e](facebook/react@505c15c9e )**: Don't inject timeline hooks unless React supports profiling ([facebook#23151](facebook/react#23151)) //<Brian Vaughn>// - **[e12a9dfc9](facebook/react@e12a9dfc9 )**: Fix production-only updateSyncExternalStore() crash when doing setState in render ([facebook#23150](facebook/react#23150)) //<Douglas Armstrong>// - **[e48940255](facebook/react@e48940255 )**: Warn when a callback ref returns a function ([facebook#23145](facebook/react#23145)) //<Dan Abramov>// - **[d8cfeaf22](facebook/react@d8cfeaf22 )**: Fix context propagation for offscreen/fallback trees ([facebook#23095](facebook/react#23095)) //<Dan Abramov>// - **[d50482478](facebook/react@d50482478 )**: Enable scheduling profiler flag in react-dom/testing builds ([facebook#23142](facebook/react#23142)) //<Brian Vaughn>// - **[790b5246f](facebook/react@790b5246f )**: Fix setState ignored in Safari when iframe is added to DOM in the same commit ([facebook#23111](facebook/react#23111)) //<Dan Abramov>// Changelog: [General][Changed] - React Native sync for revisions 51947a1...a3bde79 jest_e2e[run_all_tests] Reviewed By: ShikaSD Differential Revision: D34108924 fbshipit-source-id: 96acb66c1a7da79d6ef9403490cd0e29ad23d9fb
When an error is thrown during hydration, we fallback to client rendering, without triggering an error boundary. This is good because, in many cases, the UI will recover and the user won't even notice that something has gone wrong behind the scenes.
However, we shouldn't recover from these errors silently, because the underlying cause might be pretty serious. Server-client mismatches are not supposed to happen, even if UI doesn't break from the users perspective. Ignoring them could lead to worse problems later. De-opting from server to client rendering could also be a significant performance regression, depending on the scope of the UI it affects.
So we need a way to log when hydration errors occur.
This adds a new option for
hydrateRoot
calledonRecoverableError
. It's symmetrical to the server renderer'sonError
option, and serves the same purpose.When no option is provided, the default behavior is to schedule a browser task and rethrow the error. This will trigger the normal browser behavior for errors, including dispatching an error event. If the app already has error monitoring, this likely will just work as expected without additional configuration.
However, we can also expose additional metadata about these errors, like which Suspense boundaries were affected by the de-opt to client rendering. (I have not exposed any metadata in this PR, but we intend to add it in the future.)
In addition to errors that occur during hydration, we also log other types of recoverable errors. For example, some errors that are a result of a concurrent data race can be recovered by de-opting to synchronous rendering; blocking the main thread fixes them by prevents subsequent races. The logic for de-opting to synchronous rendering already existed. The only thing that has changed is that we now log the errors instead of silently proceeding.