Skip to content
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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Feb 4, 2022
+530 −62
Merged

Add onRecoverableError option to hydrateRoot, createRoot #23207

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cb1c764
[RFC] Add onHydrationError option to hydrateRoot
acdlite Jan 28, 2022
0f23548
Log all recoverable errors
acdlite Jan 31, 2022
667e431
Don't log recoverable errors until commit phase
acdlite Feb 3, 2022
1ee9f60
Only log hydration error if client render succeeds
acdlite Feb 3, 2022
2ab18ff
Log error with queueMicrotask instead of Scheduler
acdlite Feb 3, 2022
73ff091
Only use host config method for default behavior
acdlite Feb 4, 2022
61ef8f4
Log with reportError instead of rethrowing
acdlite Feb 4, 2022
24abe60
Naming nits
acdlite Feb 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 42 additions & 18 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import type {SuspenseState} from './ReactFiberSuspenseComponent.new';
import type {StackCursor} from './ReactFiberStack.new';
import type {Flags} from './ReactFiberFlags';
import type {FunctionComponentUpdateQueue} from './ReactFiberHooks.new';
import type {EventPriority} from './ReactEventPriorities.new';

import {
warnAboutDeprecatedLifecycles,
Expand Down Expand Up @@ -297,7 +298,11 @@ let workInProgressRootInterleavedUpdatedLanes: Lanes = NoLanes;
let workInProgressRootRenderPhaseUpdatedLanes: Lanes = NoLanes;
// Lanes that were pinged (in an interleaved event) during this render.
let workInProgressRootPingedLanes: Lanes = NoLanes;
// Errors that are thrown during the render phase.
let workInProgressRootConcurrentErrors: Array<mixed> | null = null;
// These are errors that we recovered from without surfacing them to the UI.
// We will log them once the tree commits.
let workInProgressRootRecoverableErrors: Array<mixed> | null = null;

// The most recent time we committed a fallback. This lets us ensure a train
// model where we don't commit new loading states in too quick succession.
Expand Down Expand Up @@ -896,16 +901,21 @@ function recoverFromConcurrentError(root, errorRetryLanes) {
}
}

const errorsFromFirstAttempt = workInProgressRootConcurrentErrors;
const exitStatus = renderRootSync(root, errorRetryLanes);
const recoverableErrors = workInProgressRootConcurrentErrors;
if (exitStatus !== RootErrored) {
// Successfully finished rendering
if (recoverableErrors !== null) {
// Although we recovered the UI without surfacing an error, we should
// still log the errors so they can be fixed.
for (let j = 0; j < recoverableErrors.length; j++) {
const recoverableError = recoverableErrors[j];
logRecoverableError(root.errorLoggingConfig, recoverableError);
// Successfully finished rendering on retry
if (errorsFromFirstAttempt !== null) {
// The errors from the failed first attempt have been recovered. Add
// them to the collection of recoverable errors. We'll log them in the
// commit phase.
if (workInProgressRootConcurrentErrors === null) {
workInProgressRootRecoverableErrors = errorsFromFirstAttempt;
} else {
workInProgressRootConcurrentErrors = workInProgressRootConcurrentErrors.push.apply(
null,
errorsFromFirstAttempt,
);
}
}
} else {
Expand All @@ -929,7 +939,7 @@ function finishConcurrentRender(root, exitStatus, lanes) {
case RootErrored: {
// We should have already attempted to retry this tree. If we reached
// this point, it errored again. Commit it.
commitRoot(root);
commitRoot(root, workInProgressRootRecoverableErrors);
break;
}
case RootSuspended: {
Expand Down Expand Up @@ -969,14 +979,14 @@ function finishConcurrentRender(root, exitStatus, lanes) {
// lower priority work to do. Instead of committing the fallback
// immediately, wait for more data to arrive.
root.timeoutHandle = scheduleTimeout(
commitRoot.bind(null, root),
commitRoot.bind(null, root, workInProgressRootRecoverableErrors),
msUntilTimeout,
);
break;
}
}
// The work expired. Commit immediately.
commitRoot(root);
commitRoot(root, workInProgressRootRecoverableErrors);
break;
}
case RootSuspendedWithDelay: {
Expand Down Expand Up @@ -1007,20 +1017,20 @@ function finishConcurrentRender(root, exitStatus, lanes) {
// Instead of committing the fallback immediately, wait for more data
// to arrive.
root.timeoutHandle = scheduleTimeout(
commitRoot.bind(null, root),
commitRoot.bind(null, root, workInProgressRootRecoverableErrors),
msUntilTimeout,
);
break;
}
}

// Commit the placeholder.
commitRoot(root);
commitRoot(root, workInProgressRootRecoverableErrors);
break;
}
case RootCompleted: {
// The work completed. Ready to commit.
commitRoot(root);
commitRoot(root, workInProgressRootRecoverableErrors);
break;
}
default: {
Expand Down Expand Up @@ -1140,7 +1150,7 @@ function performSyncWorkOnRoot(root) {
const finishedWork: Fiber = (root.current.alternate: any);
root.finishedWork = finishedWork;
root.finishedLanes = lanes;
commitRoot(root);
commitRoot(root, workInProgressRootRecoverableErrors);

// Before exiting, make sure there's a callback scheduled for the next
// pending level.
Expand Down Expand Up @@ -1337,6 +1347,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) {
workInProgressRootRenderPhaseUpdatedLanes = NoLanes;
workInProgressRootPingedLanes = NoLanes;
workInProgressRootConcurrentErrors = null;
workInProgressRootRecoverableErrors = null;

enqueueInterleavedUpdates();

Expand Down Expand Up @@ -1803,15 +1814,15 @@ function completeUnitOfWork(unitOfWork: Fiber): void {
}
}

function commitRoot(root) {
function commitRoot(root: FiberRoot, recoverableErrors: null | Array<mixed>) {
// TODO: This no longer makes any sense. We already wrap the mutation and
// layout phases. Should be able to remove.
const previousUpdateLanePriority = getCurrentUpdatePriority();
const prevTransition = ReactCurrentBatchConfig.transition;
try {
ReactCurrentBatchConfig.transition = 0;
setCurrentUpdatePriority(DiscreteEventPriority);
commitRootImpl(root, previousUpdateLanePriority);
commitRootImpl(root, recoverableErrors, previousUpdateLanePriority);
} finally {
ReactCurrentBatchConfig.transition = prevTransition;
setCurrentUpdatePriority(previousUpdateLanePriority);
Expand All @@ -1820,7 +1831,11 @@ function commitRoot(root) {
return null;
}

function commitRootImpl(root, renderPriorityLevel) {
function commitRootImpl(
root: FiberRoot,
recoverableErrors: null | Array<mixed>,
renderPriorityLevel: EventPriority,
) {
do {
// `flushPassiveEffects` will call `flushSyncUpdateQueue` at the end, which
// means `flushPassiveEffects` will sometimes result in additional
Expand Down Expand Up @@ -2091,6 +2106,15 @@ function commitRootImpl(root, renderPriorityLevel) {
// additional work on this root is scheduled.
ensureRootIsScheduled(root, now());

if (recoverableErrors !== null) {
// There were errors during this render, but recovered from them without
// needing to surface it to the UI. We log them here.
for (let i = 0; i < recoverableErrors.length; i++) {
const recoverableError = recoverableErrors[i];
logRecoverableError(root.errorLoggingConfig, recoverableError);
}
}

if (hasUncaughtError) {
hasUncaughtError = false;
const error = firstUncaughtError;
Expand Down
60 changes: 42 additions & 18 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import type {SuspenseState} from './ReactFiberSuspenseComponent.old';
import type {StackCursor} from './ReactFiberStack.old';
import type {Flags} from './ReactFiberFlags';
import type {FunctionComponentUpdateQueue} from './ReactFiberHooks.old';
import type {EventPriority} from './ReactEventPriorities.old';

import {
warnAboutDeprecatedLifecycles,
Expand Down Expand Up @@ -297,7 +298,11 @@ let workInProgressRootInterleavedUpdatedLanes: Lanes = NoLanes;
let workInProgressRootRenderPhaseUpdatedLanes: Lanes = NoLanes;
// Lanes that were pinged (in an interleaved event) during this render.
let workInProgressRootPingedLanes: Lanes = NoLanes;
// Errors that are thrown during the render phase.
let workInProgressRootConcurrentErrors: Array<mixed> | null = null;
// These are errors that we recovered from without surfacing them to the UI.
// We will log them once the tree commits.
let workInProgressRootRecoverableErrors: Array<mixed> | null = null;

// The most recent time we committed a fallback. This lets us ensure a train
// model where we don't commit new loading states in too quick succession.
Expand Down Expand Up @@ -896,16 +901,21 @@ function recoverFromConcurrentError(root, errorRetryLanes) {
}
}

const errorsFromFirstAttempt = workInProgressRootConcurrentErrors;
const exitStatus = renderRootSync(root, errorRetryLanes);
const recoverableErrors = workInProgressRootConcurrentErrors;
if (exitStatus !== RootErrored) {
// Successfully finished rendering
if (recoverableErrors !== null) {
// Although we recovered the UI without surfacing an error, we should
// still log the errors so they can be fixed.
for (let j = 0; j < recoverableErrors.length; j++) {
const recoverableError = recoverableErrors[j];
logRecoverableError(root.errorLoggingConfig, recoverableError);
// Successfully finished rendering on retry
if (errorsFromFirstAttempt !== null) {
// The errors from the failed first attempt have been recovered. Add
// them to the collection of recoverable errors. We'll log them in the
// commit phase.
if (workInProgressRootConcurrentErrors === null) {
workInProgressRootRecoverableErrors = errorsFromFirstAttempt;
} else {
workInProgressRootConcurrentErrors = workInProgressRootConcurrentErrors.push.apply(
null,
errorsFromFirstAttempt,
);
}
}
} else {
Expand All @@ -929,7 +939,7 @@ function finishConcurrentRender(root, exitStatus, lanes) {
case RootErrored: {
// We should have already attempted to retry this tree. If we reached
// this point, it errored again. Commit it.
commitRoot(root);
commitRoot(root, workInProgressRootRecoverableErrors);
break;
}
case RootSuspended: {
Expand Down Expand Up @@ -969,14 +979,14 @@ function finishConcurrentRender(root, exitStatus, lanes) {
// lower priority work to do. Instead of committing the fallback
// immediately, wait for more data to arrive.
root.timeoutHandle = scheduleTimeout(
commitRoot.bind(null, root),
commitRoot.bind(null, root, workInProgressRootRecoverableErrors),
msUntilTimeout,
);
break;
}
}
// The work expired. Commit immediately.
commitRoot(root);
commitRoot(root, workInProgressRootRecoverableErrors);
break;
}
case RootSuspendedWithDelay: {
Expand Down Expand Up @@ -1007,20 +1017,20 @@ function finishConcurrentRender(root, exitStatus, lanes) {
// Instead of committing the fallback immediately, wait for more data
// to arrive.
root.timeoutHandle = scheduleTimeout(
commitRoot.bind(null, root),
commitRoot.bind(null, root, workInProgressRootRecoverableErrors),
msUntilTimeout,
);
break;
}
}

// Commit the placeholder.
commitRoot(root);
commitRoot(root, workInProgressRootRecoverableErrors);
break;
}
case RootCompleted: {
// The work completed. Ready to commit.
commitRoot(root);
commitRoot(root, workInProgressRootRecoverableErrors);
break;
}
default: {
Expand Down Expand Up @@ -1140,7 +1150,7 @@ function performSyncWorkOnRoot(root) {
const finishedWork: Fiber = (root.current.alternate: any);
root.finishedWork = finishedWork;
root.finishedLanes = lanes;
commitRoot(root);
commitRoot(root, workInProgressRootRecoverableErrors);

// Before exiting, make sure there's a callback scheduled for the next
// pending level.
Expand Down Expand Up @@ -1337,6 +1347,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) {
workInProgressRootRenderPhaseUpdatedLanes = NoLanes;
workInProgressRootPingedLanes = NoLanes;
workInProgressRootConcurrentErrors = null;
workInProgressRootRecoverableErrors = null;

enqueueInterleavedUpdates();

Expand Down Expand Up @@ -1803,15 +1814,15 @@ function completeUnitOfWork(unitOfWork: Fiber): void {
}
}

function commitRoot(root) {
function commitRoot(root: FiberRoot, recoverableErrors: null | Array<mixed>) {
// TODO: This no longer makes any sense. We already wrap the mutation and
// layout phases. Should be able to remove.
const previousUpdateLanePriority = getCurrentUpdatePriority();
const prevTransition = ReactCurrentBatchConfig.transition;
try {
ReactCurrentBatchConfig.transition = 0;
setCurrentUpdatePriority(DiscreteEventPriority);
commitRootImpl(root, previousUpdateLanePriority);
commitRootImpl(root, recoverableErrors, previousUpdateLanePriority);
} finally {
ReactCurrentBatchConfig.transition = prevTransition;
setCurrentUpdatePriority(previousUpdateLanePriority);
Expand All @@ -1820,7 +1831,11 @@ function commitRoot(root) {
return null;
}

function commitRootImpl(root, renderPriorityLevel) {
function commitRootImpl(
root: FiberRoot,
recoverableErrors: null | Array<mixed>,
Copy link
Collaborator Author

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.

Copy link
Collaborator

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?

Copy link
Collaborator Author

@acdlite acdlite Feb 4, 2022 •
edited
Loading

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.

Copy link
Collaborator

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.

renderPriorityLevel: EventPriority,
) {
do {
// `flushPassiveEffects` will call `flushSyncUpdateQueue` at the end, which
// means `flushPassiveEffects` will sometimes result in additional
Expand Down Expand Up @@ -2091,6 +2106,15 @@ function commitRootImpl(root, renderPriorityLevel) {
// additional work on this root is scheduled.
ensureRootIsScheduled(root, now());

if (recoverableErrors !== null) {
Copy link
Collaborator

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.

Copy link
Collaborator Author

@acdlite acdlite Feb 4, 2022 •
edited
Loading

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.

Copy link
Collaborator

@sebmarkbage sebmarkbage Feb 4, 2022 •
edited
Loading

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.

// There were errors during this render, but recovered from them without
// needing to surface it to the UI. We log them here.
for (let i = 0; i < recoverableErrors.length; i++) {
const recoverableError = recoverableErrors[i];
logRecoverableError(root.errorLoggingConfig, recoverableError);
}
}

if (hasUncaughtError) {
hasUncaughtError = false;
const error = firstUncaughtError;
Expand Down