Skip to content

Commit

Permalink
Log all recoverable errors
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
acdlite committed Feb 3, 2022
1 parent fc5929f commit c6f65a2
Show file tree
Hide file tree
Showing 15 changed files with 192 additions and 32 deletions.
2 changes: 1 addition & 1 deletion packages/react-art/src/ReactARTHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,6 @@ export function detachDeletedInstance(node: Instance): void {
// noop
}

export function logHydrationError(config, error) {
export function logRecoverableError(config, error) {
// noop
}
66 changes: 63 additions & 3 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1898,7 +1898,7 @@ describe('ReactDOMFizzServer', () => {
// falls back to client rendering.
isClient = true;
ReactDOM.hydrateRoot(container, <App />, {
onHydrationError(error) {
onRecoverableError(error) {
Scheduler.unstable_yieldValue(error.message);
},
});
Expand Down Expand Up @@ -1982,7 +1982,7 @@ describe('ReactDOMFizzServer', () => {
// Hydrate the tree. Child will throw during render.
isClient = true;
ReactDOM.hydrateRoot(container, <App />, {
onHydrationError(error) {
onRecoverableError(error) {
// TODO: We logged a hydration error, but the same error ends up
// being thrown during the fallback to client rendering, too. Maybe
// we should only log if the client render succeeds.
Expand Down Expand Up @@ -2063,7 +2063,7 @@ describe('ReactDOMFizzServer', () => {
// falls back to client rendering.
isClient = true;
ReactDOM.hydrateRoot(container, <App />, {
onHydrationError(error) {
onRecoverableError(error) {
Scheduler.unstable_yieldValue(error.message);
},
});
Expand Down Expand Up @@ -2100,4 +2100,64 @@ describe('ReactDOMFizzServer', () => {
expect(span3Ref.current).toBe(span3);
},
);

it('logs regular (non-hydration) errors when the UI recovers', async () => {
let shouldThrow = true;

function A() {
if (shouldThrow) {
Scheduler.unstable_yieldValue('Oops!');
throw new Error('Oops!');
}
Scheduler.unstable_yieldValue('A');
return 'A';
}

function B() {
Scheduler.unstable_yieldValue('B');
return 'B';
}

function App() {
return (
<>
<A />
<B />
</>
);
}

const root = ReactDOM.createRoot(container, {
onRecoverableError(error) {
Scheduler.unstable_yieldValue(
'Logged a recoverable error: ' + error.message,
);
},
});
React.startTransition(() => {
root.render(<App />);
});

// Partially render A, but yield before the render has finished
expect(Scheduler).toFlushAndYieldThrough(['Oops!', 'Oops!']);

// React will try rendering again synchronously. During the retry, A will
// not throw. This simulates a concurrent data race that is fixed by
// blocking the main thread.
shouldThrow = false;
expect(Scheduler).toFlushAndYield([
// Finish initial render attempt
'B',

// Render again, synchronously
'A',
'B',

// Log the error
'Logged a recoverable error: Oops!',
]);

// UI looks normal
expect(container.textContent).toEqual('AB');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ describe('ReactDOMServerPartialHydration', () => {
// hydrating anyway.
suspend = true;
ReactDOM.hydrateRoot(container, <App />, {
onHydrationError(error) {
onRecoverableError(error) {
Scheduler.unstable_yieldValue(error.message);
},
});
Expand Down Expand Up @@ -299,7 +299,7 @@ describe('ReactDOMServerPartialHydration', () => {
client = true;

ReactDOM.hydrateRoot(container, <App />, {
onHydrationError(error) {
onRecoverableError(error) {
Scheduler.unstable_yieldValue(error.message);
},
});
Expand Down Expand Up @@ -3173,13 +3173,27 @@ describe('ReactDOMServerPartialHydration', () => {

expect(() => {
act(() => {
ReactDOM.hydrateRoot(container, <App />);
ReactDOM.hydrateRoot(container, <App />, {
onRecoverableError(error) {
Scheduler.unstable_yieldValue(
'Log recoverable error: ' + error.message,
);
},
});
});
}).toErrorDev(
'Warning: An error occurred during hydration. ' +
'The server HTML was replaced with client content in <div>.',
{withoutStack: true},
);
expect(Scheduler).toHaveYielded([
'Log recoverable error: An error occurred during hydration. The server ' +
'HTML was replaced with client content',
// TODO: There were multiple mismatches in a single container. Should
// we attempt to de-dupe them?
'Log recoverable error: An error occurred during hydration. The server ' +
'HTML was replaced with client content',
]);

// We show fallback state when mismatch happens at root
expect(container.innerHTML).toEqual(
Expand Down
10 changes: 6 additions & 4 deletions packages/react-dom/src/client/ReactDOMHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -379,15 +379,15 @@ export function getCurrentEventPriority(): * {
return getEventPriority(currentEvent.type);
}

export function logHydrationError(
export function logRecoverableError(
config: ErrorLoggingConfig,
error: mixed,
): void {
const onHydrationError = config;
if (onHydrationError !== null) {
const onRecoverableError = config;
if (onRecoverableError !== null) {
// Schedule a callback to invoke the user-provided logging function.
scheduleCallback(IdlePriority, () => {
onHydrationError(error);
onRecoverableError(error);
});
} else {
// Default behavior is to rethrow the error in a separate task. This will
Expand Down Expand Up @@ -1094,6 +1094,8 @@ export function didNotFindHydratableSuspenseInstance(

export function errorHydratingContainer(parentContainer: Container): void {
if (__DEV__) {
// TODO: This gets logged by onRecoverableError, too, so we should be
// able to remove it.
console.error(
'An error occurred during hydration. The server HTML was replaced with client content in <%s>.',
parentContainer.nodeName.toLowerCase(),
Expand Down
17 changes: 11 additions & 6 deletions packages/react-dom/src/client/ReactDOMRoot.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export type CreateRootOptions = {
unstable_strictMode?: boolean,
unstable_concurrentUpdatesByDefault?: boolean,
identifierPrefix?: string,
onRecoverableError?: (error: mixed) => void,
...
};

Expand All @@ -36,7 +37,7 @@ export type HydrateRootOptions = {
unstable_strictMode?: boolean,
unstable_concurrentUpdatesByDefault?: boolean,
identifierPrefix?: string,
onHydrationError?: (error: mixed) => void,
onRecoverableError?: (error: mixed) => void,
...
};

Expand Down Expand Up @@ -144,6 +145,7 @@ export function createRoot(
let isStrictMode = false;
let concurrentUpdatesByDefaultOverride = false;
let identifierPrefix = '';
let onRecoverableError = null;
if (options !== null && options !== undefined) {
if (__DEV__) {
if ((options: any).hydrate) {
Expand All @@ -164,6 +166,9 @@ export function createRoot(
if (options.identifierPrefix !== undefined) {
identifierPrefix = options.identifierPrefix;
}
if (options.onRecoverableError !== undefined) {
onRecoverableError = options.onRecoverableError;
}
}

const root = createContainer(
Expand All @@ -174,7 +179,7 @@ export function createRoot(
isStrictMode,
concurrentUpdatesByDefaultOverride,
identifierPrefix,
null,
onRecoverableError,
);
markContainerAsRoot(root.current, container);

Expand Down Expand Up @@ -215,7 +220,7 @@ export function hydrateRoot(
let isStrictMode = false;
let concurrentUpdatesByDefaultOverride = false;
let identifierPrefix = '';
let onHydrationError = null;
let onRecoverableError = null;
if (options !== null && options !== undefined) {
if (options.unstable_strictMode === true) {
isStrictMode = true;
Expand All @@ -229,8 +234,8 @@ export function hydrateRoot(
if (options.identifierPrefix !== undefined) {
identifierPrefix = options.identifierPrefix;
}
if (options.onHydrationError !== undefined) {
onHydrationError = options.onHydrationError;
if (options.onRecoverableError !== undefined) {
onRecoverableError = options.onRecoverableError;
}
}

Expand All @@ -242,7 +247,7 @@ export function hydrateRoot(
isStrictMode,
concurrentUpdatesByDefaultOverride,
identifierPrefix,
onHydrationError,
onRecoverableError,
);
markContainerAsRoot(root.current, container);
// This can't be a comment node since hydration doesn't work on comment nodes anyway.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,7 @@ export function detachDeletedInstance(node: Instance): void {
// noop
}

export function logHydrationError(
export function logRecoverableError(
config: ErrorLoggingConfig,
error: mixed,
): void {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,7 @@ export function detachDeletedInstance(node: Instance): void {
// noop
}

export function logHydrationError(
export function logRecoverableError(
config: ErrorLoggingConfig,
error: mixed,
): void {
Expand Down
2 changes: 1 addition & 1 deletion packages/react-noop-renderer/src/createReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {

detachDeletedInstance() {},

logHydrationError() {
logRecoverableError() {
// no-op
},
};
Expand Down
6 changes: 3 additions & 3 deletions packages/react-reconciler/src/ReactFiberThrow.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import {
import {
supportsPersistence,
getOffscreenContainerProps,
logHydrationError,
logRecoverableError,
} from './ReactFiberHostConfig';
import {shouldCaptureSuspense} from './ReactFiberSuspenseComponent.new';
import {NoMode, ConcurrentMode, DebugTracingMode} from './ReactTypeOfMode';
Expand Down Expand Up @@ -515,7 +515,7 @@ function throwException(
// 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.
logHydrationError(root.errorLoggingConfig, value);
logRecoverableError(root.errorLoggingConfig, value);
return;
}
} else {
Expand All @@ -526,7 +526,7 @@ function throwException(
// We didn't find a boundary that could handle this type of exception. Start
// over and traverse parent path again, this time treating the exception
// as an error.
renderDidError();
renderDidError(value);

value = createCapturedValue(value, sourceFiber);
let workInProgress = returnFiber;
Expand Down
6 changes: 3 additions & 3 deletions packages/react-reconciler/src/ReactFiberThrow.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import {
import {
supportsPersistence,
getOffscreenContainerProps,
logHydrationError,
logRecoverableError,
} from './ReactFiberHostConfig';
import {shouldCaptureSuspense} from './ReactFiberSuspenseComponent.old';
import {NoMode, ConcurrentMode, DebugTracingMode} from './ReactTypeOfMode';
Expand Down Expand Up @@ -515,7 +515,7 @@ function throwException(
// 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.
logHydrationError(root.errorLoggingConfig, value);
logRecoverableError(root.errorLoggingConfig, value);
return;
}
} else {
Expand All @@ -526,7 +526,7 @@ function throwException(
// We didn't find a boundary that could handle this type of exception. Start
// over and traverse parent path again, this time treating the exception
// as an error.
renderDidError();
renderDidError(value);

value = createCapturedValue(value, sourceFiber);
let workInProgress = returnFiber;
Expand Down
24 changes: 23 additions & 1 deletion packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ import {
supportsMicrotasks,
errorHydratingContainer,
scheduleMicrotask,
logRecoverableError,
} from './ReactFiberHostConfig';

import {
Expand Down Expand Up @@ -296,6 +297,7 @@ let workInProgressRootInterleavedUpdatedLanes: Lanes = NoLanes;
let workInProgressRootRenderPhaseUpdatedLanes: Lanes = NoLanes;
// Lanes that were pinged (in an interleaved event) during this render.
let workInProgressRootPingedLanes: Lanes = NoLanes;
let workInProgressRootConcurrentErrors: 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 @@ -895,6 +897,20 @@ function recoverFromConcurrentError(root, errorRetryLanes) {
}

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);
}
}
} else {
// The UI failed to recover.
}

executionContext = prevExecutionContext;

Expand Down Expand Up @@ -1320,6 +1336,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) {
workInProgressRootInterleavedUpdatedLanes = NoLanes;
workInProgressRootRenderPhaseUpdatedLanes = NoLanes;
workInProgressRootPingedLanes = NoLanes;
workInProgressRootConcurrentErrors = null;

enqueueInterleavedUpdates();

Expand Down Expand Up @@ -1474,10 +1491,15 @@ export function renderDidSuspendDelayIfPossible(): void {
}
}

export function renderDidError() {
export function renderDidError(error: mixed) {
if (workInProgressRootExitStatus !== RootSuspendedWithDelay) {
workInProgressRootExitStatus = RootErrored;
}
if (workInProgressRootConcurrentErrors === null) {
workInProgressRootConcurrentErrors = [error];
} else {
workInProgressRootConcurrentErrors.push(error);
}
}

// Called during render to determine if anything has suspended.
Expand Down
Loading

0 comments on commit c6f65a2

Please sign in to comment.