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
Merged

Add onRecoverableError option to hydrateRoot, createRoot #23207

Show file tree
Hide file tree
Changes from 2 commits
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
2 changes: 1 addition & 1 deletion packages/react-art/src/ReactARTHostConfig.js
View file
Open in desktop
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 logRecoverableError(config, error) {
export function logRecoverableError(error) {
// noop
}
28 changes: 24 additions & 4 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1723,12 +1723,22 @@ describe('ReactDOMFizzServer', () => {
});
expect(Scheduler).toHaveYielded(['server']);

ReactDOM.hydrateRoot(container, <App />);
ReactDOM.hydrateRoot(container, <App />, {
onRecoverableError(error) {
Scheduler.unstable_yieldValue(
'Log recoverable error: ' + error.message,
);
},
});

if (gate(flags => flags.enableClientRenderFallbackOnHydrationMismatch)) {
expect(() => {
// The first paint switches to client rendering due to mismatch
expect(Scheduler).toFlushUntilNextPaint(['client']);
expect(Scheduler).toFlushUntilNextPaint([
'client',
'Log recoverable error: An error occurred during hydration. ' +
'The server HTML was replaced with client content',
]);
}).toErrorDev(
'Warning: An error occurred during hydration. The server HTML was replaced with client content',
{withoutStack: true},
Expand Down Expand Up @@ -1805,13 +1815,23 @@ describe('ReactDOMFizzServer', () => {
});
expect(Scheduler).toHaveYielded(['server']);

ReactDOM.hydrateRoot(container, <App />);
ReactDOM.hydrateRoot(container, <App />, {
onRecoverableError(error) {
Scheduler.unstable_yieldValue(
'Log recoverable error: ' + error.message,
);
},
});

if (gate(flags => flags.enableClientRenderFallbackOnHydrationMismatch)) {
// The first paint uses the client due to mismatch forcing client render
expect(() => {
// The first paint switches to client rendering due to mismatch
expect(Scheduler).toFlushUntilNextPaint(['client']);
expect(Scheduler).toFlushUntilNextPaint([
'client',
'Log recoverable error: An error occurred during hydration. ' +
'The server HTML was replaced with client content',
]);
}).toErrorDev(
'Warning: An error occurred during hydration. The server HTML was replaced with client content',
{withoutStack: true},
Expand Down
8 changes: 7 additions & 1 deletion packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3019,7 +3019,13 @@ describe('ReactDOMServerPartialHydration', () => {
const span = container.getElementsByTagName('span')[0];
expect(span.innerHTML).toBe('Hidden child');

ReactDOM.hydrateRoot(container, <App />);
ReactDOM.hydrateRoot(container, <App />, {
onRecoverableError(error) {
Scheduler.unstable_yieldValue(
'Log recoverable error: ' + error.message,
);
},
});

Scheduler.unstable_flushAll();
expect(ref.current).toBe(span);
Expand Down
28 changes: 6 additions & 22 deletions packages/react-dom/src/client/ReactDOMHostConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ 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 @@ -124,10 +123,6 @@ 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 @@ -379,23 +374,12 @@ export function getCurrentEventPriority(): * {
return getEventPriority(currentEvent.type);
}

export function logRecoverableError(
config: ErrorLoggingConfig,
error: mixed,
): void {
const onRecoverableError = config;
if (onRecoverableError !== null) {
// 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;
});
}
export function logRecoverableError(error: mixed): void {
// Default behavior is to rethrow the error in a separate task. This will
// trigger a browser error event.
queueMicrotask(() => {
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.

Any worries about the Safari bug that can execute this on unclean stack if iframe is added?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sigh. Yeah.

Copy link
Collaborator Author

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.

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.

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(...));

Copy link
Collaborator Author

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?

Copy link
Collaborator

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.

Copy link
Collaborator

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.

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.

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.

});
}

export const isPrimaryRenderer = true;
Expand Down
7 changes: 1 addition & 6 deletions packages/react-native-renderer/src/ReactFabricHostConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,6 @@ export type RendererInspectionConfig = $ReadOnly<{|
) => void,
|}>;

export type ErrorLoggingConfig = null;

// TODO: Remove this conditional once all changes have propagated.
if (registerEventHandler) {
/**
Expand Down Expand Up @@ -528,9 +526,6 @@ export function detachDeletedInstance(node: Instance): void {
// noop
}

export function logRecoverableError(
config: ErrorLoggingConfig,
error: mixed,
): void {
export function logRecoverableError(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?

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

export type ErrorLoggingConfig = null;

const UPDATE_SIGNAL = {};
if (__DEV__) {
Object.freeze(UPDATE_SIGNAL);
Expand Down Expand Up @@ -516,9 +514,6 @@ export function detachDeletedInstance(node: Instance): void {
// noop
}

export function logRecoverableError(
config: ErrorLoggingConfig,
error: mixed,
): void {
export function logRecoverableError(error: mixed): void {
// noop
}
13 changes: 12 additions & 1 deletion packages/react-noop-renderer/src/createReactNoop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -958,7 +958,16 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
if (!root) {
const container = {rootID: rootID, pendingChildren: [], children: []};
rootContainers.set(rootID, container);
root = NoopRenderer.createContainer(container, tag, false, null, null);
root = NoopRenderer.createContainer(
container,
tag,
false,
null,
null,
false,
'',
null,
);
roots.set(rootID, root);
}
return root.current.stateNode.containerInfo;
Expand All @@ -979,6 +988,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
null,
false,
'',
null,
);
return {
_Scheduler: Scheduler,
Expand Down Expand Up @@ -1008,6 +1018,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
null,
false,
'',
null,
);
return {
_Scheduler: Scheduler,
Expand Down
5 changes: 2 additions & 3 deletions packages/react-reconciler/src/ReactFiberReconciler.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import type {
TextInstance,
Container,
PublicInstance,
ErrorLoggingConfig,
} from './ReactFiberHostConfig';
import type {RendererInspectionConfig} from './ReactFiberHostConfig';
import type {ReactNodeList} from 'shared/ReactTypes';
Expand Down Expand Up @@ -246,7 +245,7 @@ export function createContainer(
isStrictMode: boolean,
concurrentUpdatesByDefaultOverride: null | boolean,
identifierPrefix: string,
errorLoggingConfig: ErrorLoggingConfig,
onRecoverableError: null | ((error: mixed) => void),
): OpaqueRoot {
return createFiberRoot(
containerInfo,
Expand All @@ -256,7 +255,7 @@ export function createContainer(
isStrictMode,
concurrentUpdatesByDefaultOverride,
identifierPrefix,
errorLoggingConfig,
onRecoverableError,
);
}

Expand Down
5 changes: 2 additions & 3 deletions packages/react-reconciler/src/ReactFiberReconciler.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import type {
TextInstance,
Container,
PublicInstance,
ErrorLoggingConfig,
} from './ReactFiberHostConfig';
import type {RendererInspectionConfig} from './ReactFiberHostConfig';
import type {ReactNodeList} from 'shared/ReactTypes';
Expand Down Expand Up @@ -246,7 +245,7 @@ export function createContainer(
isStrictMode: boolean,
concurrentUpdatesByDefaultOverride: null | boolean,
identifierPrefix: string,
errorLoggingConfig: ErrorLoggingConfig,
onRecoverableError: null | ((error: mixed) => void),
): OpaqueRoot {
return createFiberRoot(
containerInfo,
Expand All @@ -256,7 +255,7 @@ export function createContainer(
isStrictMode,
concurrentUpdatesByDefaultOverride,
identifierPrefix,
errorLoggingConfig,
onRecoverableError,
);
}

Expand Down
9 changes: 4 additions & 5 deletions packages/react-reconciler/src/ReactFiberRoot.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

import type {FiberRoot, SuspenseHydrationCallbacks} from './ReactInternalTypes';
import type {RootTag} from './ReactRootTags';
import type {ErrorLoggingConfig} from './ReactFiberHostConfig';

import {noTimeout, supportsHydration} from './ReactFiberHostConfig';
import {createHostRootFiber} from './ReactFiber.new';
Expand All @@ -36,7 +35,7 @@ function FiberRootNode(
tag,
hydrate,
identifierPrefix,
errorLoggingConfig,
onRecoverableError,
) {
this.tag = tag;
this.containerInfo = containerInfo;
Expand Down Expand Up @@ -64,7 +63,7 @@ function FiberRootNode(
this.entanglements = createLaneMap(NoLanes);

this.identifierPrefix = identifierPrefix;
this.errorLoggingConfig = errorLoggingConfig;
this.onRecoverableError = onRecoverableError;

if (enableCache) {
this.pooledCache = null;
Expand Down Expand Up @@ -116,14 +115,14 @@ export function createFiberRoot(
// them through the root constructor. Perhaps we should put them all into a
// single type, like a DynamicHostConfig that is defined by the renderer.
identifierPrefix: string,
errorLoggingConfig: ErrorLoggingConfig,
onRecoverableError: null | ((error: mixed) => void),
): FiberRoot {
const root: FiberRoot = (new FiberRootNode(
containerInfo,
tag,
hydrate,
identifierPrefix,
errorLoggingConfig,
onRecoverableError,
): any);
if (enableSuspenseCallback) {
root.hydrationCallbacks = hydrationCallbacks;
Expand Down
9 changes: 4 additions & 5 deletions packages/react-reconciler/src/ReactFiberRoot.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

import type {FiberRoot, SuspenseHydrationCallbacks} from './ReactInternalTypes';
import type {RootTag} from './ReactRootTags';
import type {ErrorLoggingConfig} from './ReactFiberHostConfig';

import {noTimeout, supportsHydration} from './ReactFiberHostConfig';
import {createHostRootFiber} from './ReactFiber.old';
Expand All @@ -36,7 +35,7 @@ function FiberRootNode(
tag,
hydrate,
identifierPrefix,
errorLoggingConfig,
onRecoverableError,
) {
this.tag = tag;
this.containerInfo = containerInfo;
Expand Down Expand Up @@ -64,7 +63,7 @@ function FiberRootNode(
this.entanglements = createLaneMap(NoLanes);

this.identifierPrefix = identifierPrefix;
this.errorLoggingConfig = errorLoggingConfig;
this.onRecoverableError = onRecoverableError;

if (enableCache) {
this.pooledCache = null;
Expand Down Expand Up @@ -116,14 +115,14 @@ export function createFiberRoot(
// them through the root constructor. Perhaps we should put them all into a
// single type, like a DynamicHostConfig that is defined by the renderer.
identifierPrefix: string,
errorLoggingConfig: ErrorLoggingConfig,
onRecoverableError: null | ((error: mixed) => void),
): FiberRoot {
const root: FiberRoot = (new FiberRootNode(
containerInfo,
tag,
hydrate,
identifierPrefix,
errorLoggingConfig,
onRecoverableError,
): any);
if (enableSuspenseCallback) {
root.hydrationCallbacks = hydrationCallbacks;
Expand Down
9 changes: 8 additions & 1 deletion packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2115,7 +2115,14 @@ function commitRootImpl(
// 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);
const onRecoverableError = root.onRecoverableError;
if (onRecoverableError !== null) {
onRecoverableError(recoverableError);
} else {
// No user-provided onRecoverableError. Use the default behavior
// provided by the renderer's host config.
logRecoverableError(recoverableError);
}
}
}

Expand Down
9 changes: 8 additions & 1 deletion packages/react-reconciler/src/ReactFiberWorkLoop.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2115,7 +2115,14 @@ function commitRootImpl(
// 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);
const onRecoverableError = root.onRecoverableError;
if (onRecoverableError !== null) {
onRecoverableError(recoverableError);
} else {
// No user-provided onRecoverableError. Use the default behavior
// provided by the renderer's host config.
logRecoverableError(recoverableError);
}
}
}

Expand Down
7 changes: 2 additions & 5 deletions packages/react-reconciler/src/ReactInternalTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,7 @@ import type {
MutableSourceVersion,
MutableSource,
} from 'shared/ReactTypes';
import type {
SuspenseInstance,
ErrorLoggingConfig,
} from './ReactFiberHostConfig';
import type {SuspenseInstance} from './ReactFiberHostConfig';
import type {WorkTag} from './ReactWorkTags';
import type {TypeOfMode} from './ReactTypeOfMode';
import type {Flags} from './ReactFiberFlags';
Expand Down Expand Up @@ -250,7 +247,7 @@ type BaseFiberRootProperties = {|
// a reference to.
identifierPrefix: string,

errorLoggingConfig: ErrorLoggingConfig,
onRecoverableError: null | ((error: mixed) => void),
|};

// The following attributes are only used by DevTools and are only present in DEV builds.
Expand Down
1 change: 0 additions & 1 deletion packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ export opaque type ChildSet = mixed; // eslint-disable-line no-undef
export opaque type TimeoutHandle = mixed; // eslint-disable-line no-undef
export opaque type NoTimeout = mixed; // eslint-disable-line no-undef
export opaque type RendererInspectionConfig = mixed; // eslint-disable-line no-undef
export opaque type ErrorLoggingConfig = mixed; // eslint-disable-line no-undef
export type EventResponder = any;

export const getPublicInstance = $$$hostConfig.getPublicInstance;
Expand Down
Loading