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.

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
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
4 changes: 4 additions & 0 deletions packages/react-art/src/ReactARTHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -451,3 +451,7 @@ export function preparePortalMount(portalInstance: any): void {
export function detachDeletedInstance(node: Instance): void {
// noop
}

export function logRecoverableError(config, error) {
// noop
}
90 changes: 85 additions & 5 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1897,9 +1897,15 @@ describe('ReactDOMFizzServer', () => {
// Hydrate the tree. Child will throw during hydration, but not when it
// falls back to client rendering.
isClient = true;
ReactDOM.hydrateRoot(container, <App />);
ReactDOM.hydrateRoot(container, <App />, {
onRecoverableError(error) {
Scheduler.unstable_yieldValue(error.message);
},
});

expect(Scheduler).toFlushAndYield(['Yay!']);
// An error logged but instead of surfacing it to the UI, we switched
// to client rendering.
expect(Scheduler).toFlushAndYield(['Yay!', 'Hydration error']);
expect(getVisibleChildren(container)).toEqual(
<div>
<span />
Expand Down Expand Up @@ -1975,8 +1981,16 @@ describe('ReactDOMFizzServer', () => {

// Hydrate the tree. Child will throw during render.
isClient = true;
ReactDOM.hydrateRoot(container, <App />);
ReactDOM.hydrateRoot(container, <App />, {
onRecoverableError(error) {
Scheduler.unstable_yieldValue(
'Log recoverable error: ' + error.message,
);
},
});

// Because we failed to recover from the error, onRecoverableError
// shouldn't be called.
expect(Scheduler).toFlushAndYield([]);
expect(getVisibleChildren(container)).toEqual('Oops!');
},
Expand Down Expand Up @@ -2049,9 +2063,15 @@ describe('ReactDOMFizzServer', () => {
// Hydrate the tree. Child will throw during hydration, but not when it
// falls back to client rendering.
isClient = true;
ReactDOM.hydrateRoot(container, <App />);
ReactDOM.hydrateRoot(container, <App />, {
onRecoverableError(error) {
Scheduler.unstable_yieldValue(error.message);
},
});

expect(Scheduler).toFlushAndYield([]);
// An error logged but instead of surfacing it to the UI, we switched
// to client rendering.
expect(Scheduler).toFlushAndYield(['Hydration error']);
expect(getVisibleChildren(container)).toEqual(
<div>
<span />
Expand Down Expand Up @@ -2081,4 +2101,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 @@ -208,7 +208,11 @@ describe('ReactDOMServerPartialHydration', () => {
// On the client we don't have all data yet but we want to start
// hydrating anyway.
suspend = true;
ReactDOM.hydrateRoot(container, <App />);
ReactDOM.hydrateRoot(container, <App />, {
onRecoverableError(error) {
Scheduler.unstable_yieldValue(error.message);
},
});
if (gate(flags => flags.enableClientRenderFallbackOnHydrationMismatch)) {
Scheduler.unstable_flushAll();
} else {
Expand Down Expand Up @@ -290,7 +294,11 @@ describe('ReactDOMServerPartialHydration', () => {
suspend = true;
client = true;

ReactDOM.hydrateRoot(container, <App />);
ReactDOM.hydrateRoot(container, <App />, {
onRecoverableError(error) {
Scheduler.unstable_yieldValue(error.message);
},
});
expect(Scheduler).toFlushAndYield([
'Suspend',
'Component',
Expand All @@ -316,12 +324,16 @@ describe('ReactDOMServerPartialHydration', () => {
'Component',
'Component',
'Component',

// second pass as client render
'Hello',
'Component',
'Component',
'Component',
'Component',

// Hydration mismatch is logged
'An error occurred during hydration. The server HTML was replaced with client content',
]);

// Client rendered - suspense comment nodes removed
Expand Down Expand Up @@ -573,9 +585,19 @@ describe('ReactDOMServerPartialHydration', () => {

expect(() => {
act(() => {
ReactDOM.hydrateRoot(container, <App hasB={false} />);
ReactDOM.hydrateRoot(container, <App hasB={false} />, {
onRecoverableError(error) {
Scheduler.unstable_yieldValue(error.message);
},
});
});
}).toErrorDev('Did not expect server HTML to contain a <span> in <div>');
if (gate(flags => flags.enableClientRenderFallbackOnHydrationMismatch)) {
expect(Scheduler).toHaveYielded([
'An error occurred during hydration. The server HTML was replaced ' +
'with client content',
]);
}

expect(container.innerHTML).toContain('<span>A</span>');
expect(container.innerHTML).not.toContain('<span>B</span>');
Expand Down Expand Up @@ -3142,13 +3164,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
26 changes: 26 additions & 0 deletions packages/react-dom/src/client/ReactDOMHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ import {HostComponent, HostText} from 'react-reconciler/src/ReactWorkTags';
import {listenToAllSupportedEvents} from '../events/DOMPluginEventSystem';

import {DefaultEventPriority} from 'react-reconciler/src/ReactEventPriorities';
import {scheduleCallback, IdlePriority} from 'react-reconciler/src/Scheduler';

export type Type = string;
export type Props = {
Expand Down Expand Up @@ -123,6 +124,10 @@ export type TimeoutHandle = TimeoutID;
export type NoTimeout = -1;
export type RendererInspectionConfig = $ReadOnly<{||}>;

// Right now this is a single callback, but could be multiple in the in the
// future.
export type ErrorLoggingConfig = null | ((error: mixed) => void);

type SelectionInformation = {|
focusedElem: null | HTMLElement,
selectionRange: mixed,
Expand Down Expand Up @@ -374,6 +379,25 @@ export function getCurrentEventPriority(): * {
return getEventPriority(currentEvent.type);
}

export function logRecoverableError(
Copy link
Collaborator

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 {
const onRecoverableError = config;
if (onRecoverableError !== null) {
Copy link
Collaborator

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.

Copy link
Collaborator Author

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:

// 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.

Copy link
Collaborator

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.

// Schedule a callback to invoke the user-provided logging function.
scheduleCallback(IdlePriority, () => {
onRecoverableError(error);
});
} else {
// Default behavior is to rethrow the error in a separate task. This will
// trigger a browser error event.
scheduleCallback(IdlePriority, () => {
throw error;
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

});
}
}

export const isPrimaryRenderer = true;
export const warnsIfNotActing = true;
// This initialization code may run even on server environments
Expand Down Expand Up @@ -1070,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
1 change: 1 addition & 0 deletions packages/react-dom/src/client/ReactDOMLegacy.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ function legacyCreateRootFromDOMContainer(
false, // isStrictMode
false, // concurrentUpdatesByDefaultOverride,
'', // identifierPrefix
null,
);
markContainerAsRoot(root.current, container);

Expand Down
12 changes: 12 additions & 0 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,6 +37,7 @@ export type HydrateRootOptions = {
unstable_strictMode?: boolean,
unstable_concurrentUpdatesByDefault?: boolean,
identifierPrefix?: string,
onRecoverableError?: (error: mixed) => void,
...
};

Expand Down Expand Up @@ -143,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 @@ -163,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 @@ -173,6 +179,7 @@ export function createRoot(
isStrictMode,
concurrentUpdatesByDefaultOverride,
identifierPrefix,
onRecoverableError,
);
markContainerAsRoot(root.current, container);

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

const root = createContainer(
Expand All @@ -236,6 +247,7 @@ export function hydrateRoot(
isStrictMode,
concurrentUpdatesByDefaultOverride,
identifierPrefix,
onRecoverableError,
);
markContainerAsRoot(root.current, container);
// This can't be a comment node since hydration doesn't work on comment nodes anyway.
Expand Down
1 change: 1 addition & 0 deletions packages/react-native-renderer/src/ReactFabric.js
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ function render(
false,
null,
'',
null,
);
roots.set(containerTag, root);
}
Expand Down
9 changes: 9 additions & 0 deletions packages/react-native-renderer/src/ReactFabricHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@ export type RendererInspectionConfig = $ReadOnly<{|
) => void,
|}>;

export type ErrorLoggingConfig = null;

// TODO: Remove this conditional once all changes have propagated.
if (registerEventHandler) {
/**
Expand Down Expand Up @@ -525,3 +527,10 @@ export function preparePortalMount(portalInstance: Instance): void {
export function detachDeletedInstance(node: Instance): void {
// noop
}

export function logRecoverableError(
config: ErrorLoggingConfig,
error: mixed,
): void {
// noop
Copy link
Collaborator

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?

}
9 changes: 9 additions & 0 deletions packages/react-native-renderer/src/ReactNativeHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ export type RendererInspectionConfig = $ReadOnly<{|
) => void,
|}>;

export type ErrorLoggingConfig = null;

const UPDATE_SIGNAL = {};
if (__DEV__) {
Object.freeze(UPDATE_SIGNAL);
Expand Down Expand Up @@ -513,3 +515,10 @@ export function preparePortalMount(portalInstance: Instance): void {
export function detachDeletedInstance(node: Instance): void {
// noop
}

export function logRecoverableError(
config: ErrorLoggingConfig,
error: mixed,
): void {
// noop
}
Loading