From cb1c764fb8c88e32194557b957f5f338d9b33d51 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 27 Jan 2022 19:05:21 -0500 Subject: [PATCH 1/8] [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.) --- packages/react-art/src/ReactARTHostConfig.js | 4 ++ .../src/__tests__/ReactDOMFizzServer-test.js | 31 +++++++++++--- ...DOMServerPartialHydration-test.internal.js | 41 +++++++++++++++++-- .../src/client/ReactDOMHostConfig.js | 24 +++++++++++ .../react-dom/src/client/ReactDOMLegacy.js | 1 + packages/react-dom/src/client/ReactDOMRoot.js | 7 ++++ .../react-native-renderer/src/ReactFabric.js | 1 + .../src/ReactFabricHostConfig.js | 9 ++++ .../src/ReactNativeHostConfig.js | 9 ++++ .../src/ReactNativeRenderer.js | 1 + .../src/createReactNoop.js | 4 ++ .../src/ReactFiberReconciler.new.js | 3 ++ .../src/ReactFiberReconciler.old.js | 3 ++ .../src/ReactFiberRoot.new.js | 16 +++++++- .../src/ReactFiberRoot.old.js | 16 +++++++- .../src/ReactFiberThrow.new.js | 9 ++++ .../src/ReactFiberThrow.old.js | 9 ++++ .../src/ReactInternalTypes.js | 7 +++- .../ReactFiberHostContext-test.internal.js | 2 + .../src/forks/ReactFiberHostConfig.custom.js | 2 + .../src/ReactTestHostConfig.js | 9 ++++ .../src/ReactTestRenderer.js | 1 + 22 files changed, 196 insertions(+), 13 deletions(-) diff --git a/packages/react-art/src/ReactARTHostConfig.js b/packages/react-art/src/ReactARTHostConfig.js index 47bced8a1274a..b1f80ccc2be85 100644 --- a/packages/react-art/src/ReactARTHostConfig.js +++ b/packages/react-art/src/ReactARTHostConfig.js @@ -451,3 +451,7 @@ export function preparePortalMount(portalInstance: any): void { export function detachDeletedInstance(node: Instance): void { // noop } + +export function logHydrationError(config, error) { + // noop +} diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index 2d6f880851a26..fdc11cfa6b3ad 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -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, ); + ReactDOM.hydrateRoot(container, , { + onHydrationError(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(
@@ -1975,9 +1981,16 @@ describe('ReactDOMFizzServer', () => { // Hydrate the tree. Child will throw during render. isClient = true; - ReactDOM.hydrateRoot(container, ); + ReactDOM.hydrateRoot(container, , { + onHydrationError(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. + Scheduler.unstable_yieldValue(error.message); + }, + }); - expect(Scheduler).toFlushAndYield([]); + expect(Scheduler).toFlushAndYield(['Oops!']); expect(getVisibleChildren(container)).toEqual('Oops!'); }, ); @@ -2049,9 +2062,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, ); + ReactDOM.hydrateRoot(container, , { + onHydrationError(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(
diff --git a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js index 8f2913c15d600..be4be0eaddcaa 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js @@ -208,9 +208,17 @@ describe('ReactDOMServerPartialHydration', () => { // On the client we don't have all data yet but we want to start // hydrating anyway. suspend = true; - ReactDOM.hydrateRoot(container, ); + ReactDOM.hydrateRoot(container, , { + onHydrationError(error) { + Scheduler.unstable_yieldValue(error.message); + }, + }); if (gate(flags => flags.enableClientRenderFallbackOnHydrationMismatch)) { - Scheduler.unstable_flushAll(); + // Hydration error is logged + expect(Scheduler).toFlushAndYield([ + 'An error occurred during hydration. The server HTML was replaced ' + + 'with client content', + ]); } else { expect(() => { Scheduler.unstable_flushAll(); @@ -290,13 +298,24 @@ describe('ReactDOMServerPartialHydration', () => { suspend = true; client = true; - ReactDOM.hydrateRoot(container, ); + ReactDOM.hydrateRoot(container, , { + onHydrationError(error) { + Scheduler.unstable_yieldValue(error.message); + }, + }); expect(Scheduler).toFlushAndYield([ 'Suspend', 'Component', 'Component', 'Component', 'Component', + + // Hydration mismatch errors are logged. + // TODO: This could get noisy. Is there some way to dedupe? + 'An error occurred during hydration. The server HTML was replaced with client content', + 'An error occurred during hydration. The server HTML was replaced with client content', + 'An error occurred during hydration. The server HTML was replaced with client content', + 'An error occurred during hydration. The server HTML was replaced with client content', ]); jest.runAllTimers(); @@ -316,12 +335,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 @@ -573,9 +596,19 @@ describe('ReactDOMServerPartialHydration', () => { expect(() => { act(() => { - ReactDOM.hydrateRoot(container, ); + ReactDOM.hydrateRoot(container, , { + onHydrationError(error) { + Scheduler.unstable_yieldValue(error.message); + }, + }); }); }).toErrorDev('Did not expect server HTML to contain a in
'); + 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('A'); expect(container.innerHTML).not.toContain('B'); diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index 341d1fa7a3764..cd2fad2fdb873 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -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 = { @@ -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, @@ -374,6 +379,25 @@ export function getCurrentEventPriority(): * { return getEventPriority(currentEvent.type); } +export function logHydrationError( + config: ErrorLoggingConfig, + error: mixed, +): void { + const onHydrationError = config; + if (onHydrationError !== null) { + // Schedule a callback to invoke the user-provided logging function. + scheduleCallback(IdlePriority, () => { + onHydrationError(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 const isPrimaryRenderer = true; export const warnsIfNotActing = true; // This initialization code may run even on server environments diff --git a/packages/react-dom/src/client/ReactDOMLegacy.js b/packages/react-dom/src/client/ReactDOMLegacy.js index 05ae0d5ce4f45..cb95101401ea4 100644 --- a/packages/react-dom/src/client/ReactDOMLegacy.js +++ b/packages/react-dom/src/client/ReactDOMLegacy.js @@ -122,6 +122,7 @@ function legacyCreateRootFromDOMContainer( false, // isStrictMode false, // concurrentUpdatesByDefaultOverride, '', // identifierPrefix + null, ); markContainerAsRoot(root.current, container); diff --git a/packages/react-dom/src/client/ReactDOMRoot.js b/packages/react-dom/src/client/ReactDOMRoot.js index 800eeba0d018d..caf1f78c4801c 100644 --- a/packages/react-dom/src/client/ReactDOMRoot.js +++ b/packages/react-dom/src/client/ReactDOMRoot.js @@ -36,6 +36,7 @@ export type HydrateRootOptions = { unstable_strictMode?: boolean, unstable_concurrentUpdatesByDefault?: boolean, identifierPrefix?: string, + onHydrationError?: (error: mixed) => void, ... }; @@ -173,6 +174,7 @@ export function createRoot( isStrictMode, concurrentUpdatesByDefaultOverride, identifierPrefix, + null, ); markContainerAsRoot(root.current, container); @@ -213,6 +215,7 @@ export function hydrateRoot( let isStrictMode = false; let concurrentUpdatesByDefaultOverride = false; let identifierPrefix = ''; + let onHydrationError = null; if (options !== null && options !== undefined) { if (options.unstable_strictMode === true) { isStrictMode = true; @@ -226,6 +229,9 @@ export function hydrateRoot( if (options.identifierPrefix !== undefined) { identifierPrefix = options.identifierPrefix; } + if (options.onHydrationError !== undefined) { + onHydrationError = options.onHydrationError; + } } const root = createContainer( @@ -236,6 +242,7 @@ export function hydrateRoot( isStrictMode, concurrentUpdatesByDefaultOverride, identifierPrefix, + onHydrationError, ); markContainerAsRoot(root.current, container); // This can't be a comment node since hydration doesn't work on comment nodes anyway. diff --git a/packages/react-native-renderer/src/ReactFabric.js b/packages/react-native-renderer/src/ReactFabric.js index bf7754d6099c2..25cbd31b9fdf5 100644 --- a/packages/react-native-renderer/src/ReactFabric.js +++ b/packages/react-native-renderer/src/ReactFabric.js @@ -214,6 +214,7 @@ function render( false, null, '', + null, ); roots.set(containerTag, root); } diff --git a/packages/react-native-renderer/src/ReactFabricHostConfig.js b/packages/react-native-renderer/src/ReactFabricHostConfig.js index 727b782efd768..3d2f890387678 100644 --- a/packages/react-native-renderer/src/ReactFabricHostConfig.js +++ b/packages/react-native-renderer/src/ReactFabricHostConfig.js @@ -95,6 +95,8 @@ export type RendererInspectionConfig = $ReadOnly<{| ) => void, |}>; +export type ErrorLoggingConfig = null; + // TODO: Remove this conditional once all changes have propagated. if (registerEventHandler) { /** @@ -525,3 +527,10 @@ export function preparePortalMount(portalInstance: Instance): void { export function detachDeletedInstance(node: Instance): void { // noop } + +export function logHydrationError( + config: ErrorLoggingConfig, + error: mixed, +): void { + // noop +} diff --git a/packages/react-native-renderer/src/ReactNativeHostConfig.js b/packages/react-native-renderer/src/ReactNativeHostConfig.js index 10c5e37f41bcc..bc7c859c4c858 100644 --- a/packages/react-native-renderer/src/ReactNativeHostConfig.js +++ b/packages/react-native-renderer/src/ReactNativeHostConfig.js @@ -55,6 +55,8 @@ export type RendererInspectionConfig = $ReadOnly<{| ) => void, |}>; +export type ErrorLoggingConfig = null; + const UPDATE_SIGNAL = {}; if (__DEV__) { Object.freeze(UPDATE_SIGNAL); @@ -513,3 +515,10 @@ export function preparePortalMount(portalInstance: Instance): void { export function detachDeletedInstance(node: Instance): void { // noop } + +export function logHydrationError( + config: ErrorLoggingConfig, + error: mixed, +): void { + // noop +} diff --git a/packages/react-native-renderer/src/ReactNativeRenderer.js b/packages/react-native-renderer/src/ReactNativeRenderer.js index fb539d8996811..c5d4318311c0b 100644 --- a/packages/react-native-renderer/src/ReactNativeRenderer.js +++ b/packages/react-native-renderer/src/ReactNativeRenderer.js @@ -210,6 +210,7 @@ function render( false, null, '', + null, ); roots.set(containerTag, root); } diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index ef76b6610617f..c93b5eb6e91dd 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -466,6 +466,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { }, detachDeletedInstance() {}, + + logHydrationError() { + // no-op + }, }; const hostConfig = useMutation diff --git a/packages/react-reconciler/src/ReactFiberReconciler.new.js b/packages/react-reconciler/src/ReactFiberReconciler.new.js index 1cf200e35d56d..93a3e13bef85a 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.new.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.new.js @@ -15,6 +15,7 @@ import type { TextInstance, Container, PublicInstance, + ErrorLoggingConfig, } from './ReactFiberHostConfig'; import type {RendererInspectionConfig} from './ReactFiberHostConfig'; import type {ReactNodeList} from 'shared/ReactTypes'; @@ -245,6 +246,7 @@ export function createContainer( isStrictMode: boolean, concurrentUpdatesByDefaultOverride: null | boolean, identifierPrefix: string, + errorLoggingConfig: ErrorLoggingConfig, ): OpaqueRoot { return createFiberRoot( containerInfo, @@ -254,6 +256,7 @@ export function createContainer( isStrictMode, concurrentUpdatesByDefaultOverride, identifierPrefix, + errorLoggingConfig, ); } diff --git a/packages/react-reconciler/src/ReactFiberReconciler.old.js b/packages/react-reconciler/src/ReactFiberReconciler.old.js index 4b02959ab0840..0fcba6293c7b7 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.old.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.old.js @@ -15,6 +15,7 @@ import type { TextInstance, Container, PublicInstance, + ErrorLoggingConfig, } from './ReactFiberHostConfig'; import type {RendererInspectionConfig} from './ReactFiberHostConfig'; import type {ReactNodeList} from 'shared/ReactTypes'; @@ -245,6 +246,7 @@ export function createContainer( isStrictMode: boolean, concurrentUpdatesByDefaultOverride: null | boolean, identifierPrefix: string, + errorLoggingConfig: ErrorLoggingConfig, ): OpaqueRoot { return createFiberRoot( containerInfo, @@ -254,6 +256,7 @@ export function createContainer( isStrictMode, concurrentUpdatesByDefaultOverride, identifierPrefix, + errorLoggingConfig, ); } diff --git a/packages/react-reconciler/src/ReactFiberRoot.new.js b/packages/react-reconciler/src/ReactFiberRoot.new.js index 9e9feb45d9b03..dd5e6a5fc7da0 100644 --- a/packages/react-reconciler/src/ReactFiberRoot.new.js +++ b/packages/react-reconciler/src/ReactFiberRoot.new.js @@ -9,6 +9,7 @@ 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'; @@ -30,7 +31,13 @@ import {initializeUpdateQueue} from './ReactUpdateQueue.new'; import {LegacyRoot, ConcurrentRoot} from './ReactRootTags'; import {createCache, retainCache} from './ReactFiberCacheComponent.new'; -function FiberRootNode(containerInfo, tag, hydrate, identifierPrefix) { +function FiberRootNode( + containerInfo, + tag, + hydrate, + identifierPrefix, + errorLoggingConfig, +) { this.tag = tag; this.containerInfo = containerInfo; this.pendingChildren = null; @@ -57,6 +64,7 @@ function FiberRootNode(containerInfo, tag, hydrate, identifierPrefix) { this.entanglements = createLaneMap(NoLanes); this.identifierPrefix = identifierPrefix; + this.errorLoggingConfig = errorLoggingConfig; if (enableCache) { this.pooledCache = null; @@ -103,13 +111,19 @@ export function createFiberRoot( hydrationCallbacks: null | SuspenseHydrationCallbacks, isStrictMode: boolean, concurrentUpdatesByDefaultOverride: null | boolean, + // 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. identifierPrefix: string, + errorLoggingConfig: ErrorLoggingConfig, ): FiberRoot { const root: FiberRoot = (new FiberRootNode( containerInfo, tag, hydrate, identifierPrefix, + errorLoggingConfig, ): any); if (enableSuspenseCallback) { root.hydrationCallbacks = hydrationCallbacks; diff --git a/packages/react-reconciler/src/ReactFiberRoot.old.js b/packages/react-reconciler/src/ReactFiberRoot.old.js index d8d061297854f..d6791e97c34fd 100644 --- a/packages/react-reconciler/src/ReactFiberRoot.old.js +++ b/packages/react-reconciler/src/ReactFiberRoot.old.js @@ -9,6 +9,7 @@ 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'; @@ -30,7 +31,13 @@ import {initializeUpdateQueue} from './ReactUpdateQueue.old'; import {LegacyRoot, ConcurrentRoot} from './ReactRootTags'; import {createCache, retainCache} from './ReactFiberCacheComponent.old'; -function FiberRootNode(containerInfo, tag, hydrate, identifierPrefix) { +function FiberRootNode( + containerInfo, + tag, + hydrate, + identifierPrefix, + errorLoggingConfig, +) { this.tag = tag; this.containerInfo = containerInfo; this.pendingChildren = null; @@ -57,6 +64,7 @@ function FiberRootNode(containerInfo, tag, hydrate, identifierPrefix) { this.entanglements = createLaneMap(NoLanes); this.identifierPrefix = identifierPrefix; + this.errorLoggingConfig = errorLoggingConfig; if (enableCache) { this.pooledCache = null; @@ -103,13 +111,19 @@ export function createFiberRoot( hydrationCallbacks: null | SuspenseHydrationCallbacks, isStrictMode: boolean, concurrentUpdatesByDefaultOverride: null | boolean, + // 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. identifierPrefix: string, + errorLoggingConfig: ErrorLoggingConfig, ): FiberRoot { const root: FiberRoot = (new FiberRootNode( containerInfo, tag, hydrate, identifierPrefix, + errorLoggingConfig, ): any); if (enableSuspenseCallback) { root.hydrationCallbacks = hydrationCallbacks; diff --git a/packages/react-reconciler/src/ReactFiberThrow.new.js b/packages/react-reconciler/src/ReactFiberThrow.new.js index cd9931687ba5a..60903d236e0d5 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.new.js +++ b/packages/react-reconciler/src/ReactFiberThrow.new.js @@ -37,6 +37,7 @@ import { import { supportsPersistence, getOffscreenContainerProps, + logHydrationError, } from './ReactFiberHostConfig'; import {shouldCaptureSuspense} from './ReactFiberSuspenseComponent.new'; import {NoMode, ConcurrentMode, DebugTracingMode} from './ReactTypeOfMode'; @@ -507,6 +508,14 @@ function throwException( root, rootRenderLanes, ); + + // Even though the user may not be affected by this error, we should + // still log it so it can be fixed. + // TODO: For now, we only log errors that occur during hydration, but we + // 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); return; } } else { diff --git a/packages/react-reconciler/src/ReactFiberThrow.old.js b/packages/react-reconciler/src/ReactFiberThrow.old.js index 8f6d18a48dea3..6b7f4bf6055b4 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.old.js +++ b/packages/react-reconciler/src/ReactFiberThrow.old.js @@ -37,6 +37,7 @@ import { import { supportsPersistence, getOffscreenContainerProps, + logHydrationError, } from './ReactFiberHostConfig'; import {shouldCaptureSuspense} from './ReactFiberSuspenseComponent.old'; import {NoMode, ConcurrentMode, DebugTracingMode} from './ReactTypeOfMode'; @@ -507,6 +508,14 @@ function throwException( root, rootRenderLanes, ); + + // Even though the user may not be affected by this error, we should + // still log it so it can be fixed. + // TODO: For now, we only log errors that occur during hydration, but we + // 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); return; } } else { diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js index 13965720b7cd3..1cc2128356090 100644 --- a/packages/react-reconciler/src/ReactInternalTypes.js +++ b/packages/react-reconciler/src/ReactInternalTypes.js @@ -16,7 +16,10 @@ import type { MutableSourceVersion, MutableSource, } from 'shared/ReactTypes'; -import type {SuspenseInstance} from './ReactFiberHostConfig'; +import type { + SuspenseInstance, + ErrorLoggingConfig, +} from './ReactFiberHostConfig'; import type {WorkTag} from './ReactWorkTags'; import type {TypeOfMode} from './ReactTypeOfMode'; import type {Flags} from './ReactFiberFlags'; @@ -246,6 +249,8 @@ type BaseFiberRootProperties = {| // the public createRoot object, which the fiber tree does not currently have // a reference to. identifierPrefix: string, + + errorLoggingConfig: ErrorLoggingConfig, |}; // The following attributes are only used by DevTools and are only present in DEV builds. diff --git a/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.internal.js b/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.internal.js index 4bf292df79f7a..d0c3d5b236ea4 100644 --- a/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.internal.js @@ -76,6 +76,7 @@ describe('ReactFiberHostContext', () => { null, false, '', + null, ); act(() => { Renderer.updateContainer( @@ -139,6 +140,7 @@ describe('ReactFiberHostContext', () => { null, false, '', + null, ); act(() => { Renderer.updateContainer( diff --git a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js index 6535d8d3fdec3..38cee5f94e11c 100644 --- a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js +++ b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js @@ -38,6 +38,7 @@ 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; @@ -68,6 +69,7 @@ export const prepareScopeUpdate = $$$hostConfig.preparePortalMount; export const getInstanceFromScope = $$$hostConfig.getInstanceFromScope; export const getCurrentEventPriority = $$$hostConfig.getCurrentEventPriority; export const detachDeletedInstance = $$$hostConfig.detachDeletedInstance; +export const logHydrationError = $$$hostConfig.logHydrationError; // ------------------- // Microtasks diff --git a/packages/react-test-renderer/src/ReactTestHostConfig.js b/packages/react-test-renderer/src/ReactTestHostConfig.js index 503b08efaf20b..5279fda0b43f6 100644 --- a/packages/react-test-renderer/src/ReactTestHostConfig.js +++ b/packages/react-test-renderer/src/ReactTestHostConfig.js @@ -42,6 +42,8 @@ export type EventResponder = any; export type RendererInspectionConfig = $ReadOnly<{||}>; +export type ErrorLoggingConfig = null; + export * from 'react-reconciler/src/ReactFiberHostConfigWithNoPersistence'; export * from 'react-reconciler/src/ReactFiberHostConfigWithNoHydration'; export * from 'react-reconciler/src/ReactFiberHostConfigWithNoTestSelectors'; @@ -314,3 +316,10 @@ export function getInstanceFromScope(scopeInstance: Object): null | Object { export function detachDeletedInstance(node: Instance): void { // noop } + +export function logHydrationError( + config: ErrorLoggingConfig, + error: mixed, +): void { + // noop +} diff --git a/packages/react-test-renderer/src/ReactTestRenderer.js b/packages/react-test-renderer/src/ReactTestRenderer.js index de6e4beffec5f..a8121d1a14fcf 100644 --- a/packages/react-test-renderer/src/ReactTestRenderer.js +++ b/packages/react-test-renderer/src/ReactTestRenderer.js @@ -472,6 +472,7 @@ function create(element: React$Element, options: TestRendererOptions) { isStrictMode, concurrentUpdatesByDefault, '', + null, ); if (root == null) { From 0f235487cccc61ff0313d63a842c3cea0dc14517 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 31 Jan 2022 12:50:19 -0500 Subject: [PATCH 2/8] 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. --- packages/react-art/src/ReactARTHostConfig.js | 2 +- .../src/__tests__/ReactDOMFizzServer-test.js | 66 ++++++++++++++++++- ...DOMServerPartialHydration-test.internal.js | 22 +++++-- .../src/client/ReactDOMHostConfig.js | 10 +-- packages/react-dom/src/client/ReactDOMRoot.js | 17 +++-- .../src/ReactFabricHostConfig.js | 2 +- .../src/ReactNativeHostConfig.js | 2 +- .../src/createReactNoop.js | 2 +- .../src/ReactFiberThrow.new.js | 6 +- .../src/ReactFiberThrow.old.js | 6 +- .../src/ReactFiberWorkLoop.new.js | 24 ++++++- .../src/ReactFiberWorkLoop.old.js | 24 ++++++- .../useMutableSourceHydration-test.js | 39 ++++++++++- .../src/forks/ReactFiberHostConfig.custom.js | 2 +- .../src/ReactTestHostConfig.js | 2 +- 15 files changed, 193 insertions(+), 33 deletions(-) diff --git a/packages/react-art/src/ReactARTHostConfig.js b/packages/react-art/src/ReactARTHostConfig.js index b1f80ccc2be85..4ec8e3a4e6f2e 100644 --- a/packages/react-art/src/ReactARTHostConfig.js +++ b/packages/react-art/src/ReactARTHostConfig.js @@ -452,6 +452,6 @@ export function detachDeletedInstance(node: Instance): void { // noop } -export function logHydrationError(config, error) { +export function logRecoverableError(config, error) { // noop } diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index fdc11cfa6b3ad..e16aa9cbc5649 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -1898,7 +1898,7 @@ describe('ReactDOMFizzServer', () => { // falls back to client rendering. isClient = true; ReactDOM.hydrateRoot(container, , { - onHydrationError(error) { + onRecoverableError(error) { Scheduler.unstable_yieldValue(error.message); }, }); @@ -1982,7 +1982,7 @@ describe('ReactDOMFizzServer', () => { // Hydrate the tree. Child will throw during render. isClient = true; ReactDOM.hydrateRoot(container, , { - 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. @@ -2063,7 +2063,7 @@ describe('ReactDOMFizzServer', () => { // falls back to client rendering. isClient = true; ReactDOM.hydrateRoot(container, , { - onHydrationError(error) { + onRecoverableError(error) { Scheduler.unstable_yieldValue(error.message); }, }); @@ -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 ( + <> + + + + ); + } + + const root = ReactDOM.createRoot(container, { + onRecoverableError(error) { + Scheduler.unstable_yieldValue( + 'Logged a recoverable error: ' + error.message, + ); + }, + }); + React.startTransition(() => { + root.render(); + }); + + // 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'); + }); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js index be4be0eaddcaa..6e81765347a93 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js @@ -209,7 +209,7 @@ describe('ReactDOMServerPartialHydration', () => { // hydrating anyway. suspend = true; ReactDOM.hydrateRoot(container, , { - onHydrationError(error) { + onRecoverableError(error) { Scheduler.unstable_yieldValue(error.message); }, }); @@ -299,7 +299,7 @@ describe('ReactDOMServerPartialHydration', () => { client = true; ReactDOM.hydrateRoot(container, , { - onHydrationError(error) { + onRecoverableError(error) { Scheduler.unstable_yieldValue(error.message); }, }); @@ -597,7 +597,7 @@ describe('ReactDOMServerPartialHydration', () => { expect(() => { act(() => { ReactDOM.hydrateRoot(container, , { - onHydrationError(error) { + onRecoverableError(error) { Scheduler.unstable_yieldValue(error.message); }, }); @@ -3175,13 +3175,27 @@ describe('ReactDOMServerPartialHydration', () => { expect(() => { act(() => { - ReactDOM.hydrateRoot(container, ); + ReactDOM.hydrateRoot(container, , { + 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
.', {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( diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index cd2fad2fdb873..fdd8b38e312b7 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -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 @@ -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(), diff --git a/packages/react-dom/src/client/ReactDOMRoot.js b/packages/react-dom/src/client/ReactDOMRoot.js index caf1f78c4801c..fe6b6ee31f773 100644 --- a/packages/react-dom/src/client/ReactDOMRoot.js +++ b/packages/react-dom/src/client/ReactDOMRoot.js @@ -24,6 +24,7 @@ export type CreateRootOptions = { unstable_strictMode?: boolean, unstable_concurrentUpdatesByDefault?: boolean, identifierPrefix?: string, + onRecoverableError?: (error: mixed) => void, ... }; @@ -36,7 +37,7 @@ export type HydrateRootOptions = { unstable_strictMode?: boolean, unstable_concurrentUpdatesByDefault?: boolean, identifierPrefix?: string, - onHydrationError?: (error: mixed) => void, + onRecoverableError?: (error: mixed) => void, ... }; @@ -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) { @@ -164,6 +166,9 @@ export function createRoot( if (options.identifierPrefix !== undefined) { identifierPrefix = options.identifierPrefix; } + if (options.onRecoverableError !== undefined) { + onRecoverableError = options.onRecoverableError; + } } const root = createContainer( @@ -174,7 +179,7 @@ export function createRoot( isStrictMode, concurrentUpdatesByDefaultOverride, identifierPrefix, - null, + onRecoverableError, ); markContainerAsRoot(root.current, container); @@ -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; @@ -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; } } @@ -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. diff --git a/packages/react-native-renderer/src/ReactFabricHostConfig.js b/packages/react-native-renderer/src/ReactFabricHostConfig.js index 3d2f890387678..e720c2e12534e 100644 --- a/packages/react-native-renderer/src/ReactFabricHostConfig.js +++ b/packages/react-native-renderer/src/ReactFabricHostConfig.js @@ -528,7 +528,7 @@ export function detachDeletedInstance(node: Instance): void { // noop } -export function logHydrationError( +export function logRecoverableError( config: ErrorLoggingConfig, error: mixed, ): void { diff --git a/packages/react-native-renderer/src/ReactNativeHostConfig.js b/packages/react-native-renderer/src/ReactNativeHostConfig.js index bc7c859c4c858..27df360718aee 100644 --- a/packages/react-native-renderer/src/ReactNativeHostConfig.js +++ b/packages/react-native-renderer/src/ReactNativeHostConfig.js @@ -516,7 +516,7 @@ export function detachDeletedInstance(node: Instance): void { // noop } -export function logHydrationError( +export function logRecoverableError( config: ErrorLoggingConfig, error: mixed, ): void { diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index c93b5eb6e91dd..411757c0436be 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -467,7 +467,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { detachDeletedInstance() {}, - logHydrationError() { + logRecoverableError() { // no-op }, }; diff --git a/packages/react-reconciler/src/ReactFiberThrow.new.js b/packages/react-reconciler/src/ReactFiberThrow.new.js index 60903d236e0d5..c2b2a9a2fa5e7 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.new.js +++ b/packages/react-reconciler/src/ReactFiberThrow.new.js @@ -37,7 +37,7 @@ import { import { supportsPersistence, getOffscreenContainerProps, - logHydrationError, + logRecoverableError, } from './ReactFiberHostConfig'; import {shouldCaptureSuspense} from './ReactFiberSuspenseComponent.new'; import {NoMode, ConcurrentMode, DebugTracingMode} from './ReactTypeOfMode'; @@ -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 { @@ -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; diff --git a/packages/react-reconciler/src/ReactFiberThrow.old.js b/packages/react-reconciler/src/ReactFiberThrow.old.js index 6b7f4bf6055b4..3ae5df1f93414 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.old.js +++ b/packages/react-reconciler/src/ReactFiberThrow.old.js @@ -37,7 +37,7 @@ import { import { supportsPersistence, getOffscreenContainerProps, - logHydrationError, + logRecoverableError, } from './ReactFiberHostConfig'; import {shouldCaptureSuspense} from './ReactFiberSuspenseComponent.old'; import {NoMode, ConcurrentMode, DebugTracingMode} from './ReactTypeOfMode'; @@ -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 { @@ -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; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index d5372999f3c46..cde794a35f872 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -76,6 +76,7 @@ import { supportsMicrotasks, errorHydratingContainer, scheduleMicrotask, + logRecoverableError, } from './ReactFiberHostConfig'; import { @@ -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 | 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. @@ -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; @@ -1320,6 +1336,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) { workInProgressRootInterleavedUpdatedLanes = NoLanes; workInProgressRootRenderPhaseUpdatedLanes = NoLanes; workInProgressRootPingedLanes = NoLanes; + workInProgressRootConcurrentErrors = null; enqueueInterleavedUpdates(); @@ -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. diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 92be9f0a323a9..3b26a00729f4a 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -76,6 +76,7 @@ import { supportsMicrotasks, errorHydratingContainer, scheduleMicrotask, + logRecoverableError, } from './ReactFiberHostConfig'; import { @@ -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 | 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. @@ -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; @@ -1320,6 +1336,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) { workInProgressRootInterleavedUpdatedLanes = NoLanes; workInProgressRootRenderPhaseUpdatedLanes = NoLanes; workInProgressRootPingedLanes = NoLanes; + workInProgressRootConcurrentErrors = null; enqueueInterleavedUpdates(); @@ -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. diff --git a/packages/react-reconciler/src/__tests__/useMutableSourceHydration-test.js b/packages/react-reconciler/src/__tests__/useMutableSourceHydration-test.js index 4c13ef7c35dfa..16ebb4657d28d 100644 --- a/packages/react-reconciler/src/__tests__/useMutableSourceHydration-test.js +++ b/packages/react-reconciler/src/__tests__/useMutableSourceHydration-test.js @@ -205,6 +205,9 @@ describe('useMutableSourceHydration', () => { act(() => { ReactDOM.hydrateRoot(container, , { mutableSources: [mutableSource], + onRecoverableError(error) { + Scheduler.unstable_yieldValue('Log error: ' + error.message); + }, }); source.value = 'two'; @@ -254,11 +257,17 @@ describe('useMutableSourceHydration', () => { React.startTransition(() => { ReactDOM.hydrateRoot(container, , { mutableSources: [mutableSource], + onRecoverableError(error) { + Scheduler.unstable_yieldValue('Log error: ' + error.message); + }, }); }); } else { ReactDOM.hydrateRoot(container, , { mutableSources: [mutableSource], + onRecoverableError(error) { + Scheduler.unstable_yieldValue('Log error: ' + error.message); + }, }); } expect(Scheduler).toFlushAndYieldThrough(['a:one']); @@ -269,7 +278,17 @@ describe('useMutableSourceHydration', () => { 'The server HTML was replaced with client content in
.', {withoutStack: true}, ); - expect(Scheduler).toHaveYielded(['a:two', 'b:two']); + expect(Scheduler).toHaveYielded([ + 'a:two', + 'b:two', + // TODO: Before onRecoverableError, this error was never surfaced to the + // user. The request to file an bug report no longer makes sense. + // However, the experimental useMutableSource API is slated for + // removal, anyway. + 'Log error: Cannot read from mutable source during the current ' + + 'render without tearing. This may be a bug in React. Please file ' + + 'an issue.', + ]); expect(source.listenerCount).toBe(2); }); @@ -328,11 +347,17 @@ describe('useMutableSourceHydration', () => { React.startTransition(() => { ReactDOM.hydrateRoot(container, fragment, { mutableSources: [mutableSource], + onRecoverableError(error) { + Scheduler.unstable_yieldValue('Log error: ' + error.message); + }, }); }); } else { ReactDOM.hydrateRoot(container, fragment, { mutableSources: [mutableSource], + onRecoverableError(error) { + Scheduler.unstable_yieldValue('Log error: ' + error.message); + }, }); } expect(Scheduler).toFlushAndYieldThrough(['0:a:one']); @@ -343,7 +368,17 @@ describe('useMutableSourceHydration', () => { 'The server HTML was replaced with client content in
.', {withoutStack: true}, ); - expect(Scheduler).toHaveYielded(['0:a:one', '1:b:two']); + expect(Scheduler).toHaveYielded([ + '0:a:one', + '1:b:two', + // TODO: Before onRecoverableError, this error was never surfaced to the + // user. The request to file an bug report no longer makes sense. + // However, the experimental useMutableSource API is slated for + // removal, anyway. + 'Log error: Cannot read from mutable source during the current ' + + 'render without tearing. This may be a bug in React. Please file ' + + 'an issue.', + ]); }); // @gate !enableSyncDefaultUpdates diff --git a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js index 38cee5f94e11c..8e67bf5517e45 100644 --- a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js +++ b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js @@ -69,7 +69,7 @@ export const prepareScopeUpdate = $$$hostConfig.preparePortalMount; export const getInstanceFromScope = $$$hostConfig.getInstanceFromScope; export const getCurrentEventPriority = $$$hostConfig.getCurrentEventPriority; export const detachDeletedInstance = $$$hostConfig.detachDeletedInstance; -export const logHydrationError = $$$hostConfig.logHydrationError; +export const logRecoverableError = $$$hostConfig.logRecoverableError; // ------------------- // Microtasks diff --git a/packages/react-test-renderer/src/ReactTestHostConfig.js b/packages/react-test-renderer/src/ReactTestHostConfig.js index 5279fda0b43f6..266b2c06e58a1 100644 --- a/packages/react-test-renderer/src/ReactTestHostConfig.js +++ b/packages/react-test-renderer/src/ReactTestHostConfig.js @@ -317,7 +317,7 @@ export function detachDeletedInstance(node: Instance): void { // noop } -export function logHydrationError( +export function logRecoverableError( config: ErrorLoggingConfig, error: mixed, ): void { From 667e43155883be6625b75dc887c801e578bc93a2 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 2 Feb 2022 20:13:59 -0500 Subject: [PATCH 3/8] 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. --- .../src/ReactFiberWorkLoop.new.js | 60 +++++++++++++------ .../src/ReactFiberWorkLoop.old.js | 60 +++++++++++++------ 2 files changed, 84 insertions(+), 36 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index cde794a35f872..edc6037a67057 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -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, @@ -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 | 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 | 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. @@ -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 { @@ -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: { @@ -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: { @@ -1007,7 +1017,7 @@ 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; @@ -1015,12 +1025,12 @@ function finishConcurrentRender(root, exitStatus, lanes) { } // Commit the placeholder. - commitRoot(root); + commitRoot(root, workInProgressRootRecoverableErrors); break; } case RootCompleted: { // The work completed. Ready to commit. - commitRoot(root); + commitRoot(root, workInProgressRootRecoverableErrors); break; } default: { @@ -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. @@ -1337,6 +1347,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) { workInProgressRootRenderPhaseUpdatedLanes = NoLanes; workInProgressRootPingedLanes = NoLanes; workInProgressRootConcurrentErrors = null; + workInProgressRootRecoverableErrors = null; enqueueInterleavedUpdates(); @@ -1803,7 +1814,7 @@ function completeUnitOfWork(unitOfWork: Fiber): void { } } -function commitRoot(root) { +function commitRoot(root: FiberRoot, recoverableErrors: null | Array) { // TODO: This no longer makes any sense. We already wrap the mutation and // layout phases. Should be able to remove. const previousUpdateLanePriority = getCurrentUpdatePriority(); @@ -1811,7 +1822,7 @@ function commitRoot(root) { try { ReactCurrentBatchConfig.transition = 0; setCurrentUpdatePriority(DiscreteEventPriority); - commitRootImpl(root, previousUpdateLanePriority); + commitRootImpl(root, recoverableErrors, previousUpdateLanePriority); } finally { ReactCurrentBatchConfig.transition = prevTransition; setCurrentUpdatePriority(previousUpdateLanePriority); @@ -1820,7 +1831,11 @@ function commitRoot(root) { return null; } -function commitRootImpl(root, renderPriorityLevel) { +function commitRootImpl( + root: FiberRoot, + recoverableErrors: null | Array, + renderPriorityLevel: EventPriority, +) { do { // `flushPassiveEffects` will call `flushSyncUpdateQueue` at the end, which // means `flushPassiveEffects` will sometimes result in additional @@ -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; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 3b26a00729f4a..4eb05f2ce40d0 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -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, @@ -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 | 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 | 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. @@ -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 { @@ -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: { @@ -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: { @@ -1007,7 +1017,7 @@ 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; @@ -1015,12 +1025,12 @@ function finishConcurrentRender(root, exitStatus, lanes) { } // Commit the placeholder. - commitRoot(root); + commitRoot(root, workInProgressRootRecoverableErrors); break; } case RootCompleted: { // The work completed. Ready to commit. - commitRoot(root); + commitRoot(root, workInProgressRootRecoverableErrors); break; } default: { @@ -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. @@ -1337,6 +1347,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) { workInProgressRootRenderPhaseUpdatedLanes = NoLanes; workInProgressRootPingedLanes = NoLanes; workInProgressRootConcurrentErrors = null; + workInProgressRootRecoverableErrors = null; enqueueInterleavedUpdates(); @@ -1803,7 +1814,7 @@ function completeUnitOfWork(unitOfWork: Fiber): void { } } -function commitRoot(root) { +function commitRoot(root: FiberRoot, recoverableErrors: null | Array) { // TODO: This no longer makes any sense. We already wrap the mutation and // layout phases. Should be able to remove. const previousUpdateLanePriority = getCurrentUpdatePriority(); @@ -1811,7 +1822,7 @@ function commitRoot(root) { try { ReactCurrentBatchConfig.transition = 0; setCurrentUpdatePriority(DiscreteEventPriority); - commitRootImpl(root, previousUpdateLanePriority); + commitRootImpl(root, recoverableErrors, previousUpdateLanePriority); } finally { ReactCurrentBatchConfig.transition = prevTransition; setCurrentUpdatePriority(previousUpdateLanePriority); @@ -1820,7 +1831,11 @@ function commitRoot(root) { return null; } -function commitRootImpl(root, renderPriorityLevel) { +function commitRootImpl( + root: FiberRoot, + recoverableErrors: null | Array, + renderPriorityLevel: EventPriority, +) { do { // `flushPassiveEffects` will call `flushSyncUpdateQueue` at the end, which // means `flushPassiveEffects` will sometimes result in additional @@ -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; From 1ee9f6059cfcf5f7827efbfbe2ef68e2ed939ef4 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 3 Feb 2022 16:58:57 -0500 Subject: [PATCH 4/8] 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.) --- .../src/__tests__/ReactDOMFizzServer-test.js | 11 +++++---- ...DOMServerPartialHydration-test.internal.js | 13 +--------- .../src/ReactFiberCompleteWork.new.js | 7 ++++++ .../src/ReactFiberCompleteWork.old.js | 7 ++++++ .../src/ReactFiberHydrationContext.new.js | 24 +++++++++++++++++++ .../src/ReactFiberHydrationContext.old.js | 24 +++++++++++++++++++ .../src/ReactFiberThrow.new.js | 12 ++++------ .../src/ReactFiberThrow.old.js | 12 ++++------ .../src/ReactFiberWorkLoop.new.js | 20 +++++++++------- .../src/ReactFiberWorkLoop.old.js | 20 +++++++++------- 10 files changed, 103 insertions(+), 47 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index e16aa9cbc5649..0b40c49b66e8e 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -1983,14 +1983,15 @@ describe('ReactDOMFizzServer', () => { isClient = true; ReactDOM.hydrateRoot(container, , { 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. - Scheduler.unstable_yieldValue(error.message); + Scheduler.unstable_yieldValue( + 'Log recoverable error: ' + error.message, + ); }, }); - expect(Scheduler).toFlushAndYield(['Oops!']); + // Because we failed to recover from the error, onRecoverableError + // shouldn't be called. + expect(Scheduler).toFlushAndYield([]); expect(getVisibleChildren(container)).toEqual('Oops!'); }, ); diff --git a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js index 6e81765347a93..1e819091bf4c7 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js @@ -214,11 +214,7 @@ describe('ReactDOMServerPartialHydration', () => { }, }); if (gate(flags => flags.enableClientRenderFallbackOnHydrationMismatch)) { - // Hydration error is logged - expect(Scheduler).toFlushAndYield([ - 'An error occurred during hydration. The server HTML was replaced ' + - 'with client content', - ]); + Scheduler.unstable_flushAll(); } else { expect(() => { Scheduler.unstable_flushAll(); @@ -309,13 +305,6 @@ describe('ReactDOMServerPartialHydration', () => { 'Component', 'Component', 'Component', - - // Hydration mismatch errors are logged. - // TODO: This could get noisy. Is there some way to dedupe? - 'An error occurred during hydration. The server HTML was replaced with client content', - 'An error occurred during hydration. The server HTML was replaced with client content', - 'An error occurred during hydration. The server HTML was replaced with client content', - 'An error occurred during hydration. The server HTML was replaced with client content', ]); jest.runAllTimers(); diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js index e8ec41d9d81f5..de5b2309146a7 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js @@ -131,6 +131,7 @@ import { resetHydrationState, getIsHydrating, hasUnhydratedTailNodes, + queueRecoverableHydrationErrors, } from './ReactFiberHydrationContext.new'; import { enableSuspenseCallback, @@ -1099,6 +1100,12 @@ function completeWork( return null; } } + + // Successfully completed this tree. If this was a forced client render, + // there may have been recoverable errors during first hydration + // attempt. If so, add them to a queue so we can log them in the + // commit phase. + queueRecoverableHydrationErrors(); } if ((workInProgress.flags & DidCapture) !== NoFlags) { diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js index 0a0273470a702..025f65fa4eda0 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js @@ -131,6 +131,7 @@ import { resetHydrationState, getIsHydrating, hasUnhydratedTailNodes, + queueRecoverableHydrationErrors, } from './ReactFiberHydrationContext.old'; import { enableSuspenseCallback, @@ -1099,6 +1100,12 @@ function completeWork( return null; } } + + // Successfully completed this tree. If this was a forced client render, + // there may have been recoverable errors during first hydration + // attempt. If so, add them to a queue so we can log them in the + // commit phase. + queueRecoverableHydrationErrors(); } if ((workInProgress.flags & DidCapture) !== NoFlags) { diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js index b6153eddccebb..1eae8d58ec363 100644 --- a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js +++ b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js @@ -77,6 +77,7 @@ import { getSuspendedTreeContext, restoreSuspendedTreeContext, } from './ReactFiberTreeContext.new'; +import {queueRecoverableErrors} from './ReactFiberWorkLoop.new'; // The deepest Fiber on the stack involved in a hydration context. // This may have been an insertion or a hydration. @@ -84,6 +85,9 @@ let hydrationParentFiber: null | Fiber = null; let nextHydratableInstance: null | HydratableInstance = null; let isHydrating: boolean = false; +// Hydration errors that were thrown inside this boundary +let hydrationErrors: Array | null = null; + function warnIfHydrating() { if (__DEV__) { if (isHydrating) { @@ -105,6 +109,7 @@ function enterHydrationState(fiber: Fiber): boolean { ); hydrationParentFiber = fiber; isHydrating = true; + hydrationErrors = null; return true; } @@ -121,6 +126,7 @@ function reenterHydrationStateFromDehydratedSuspenseInstance( ); hydrationParentFiber = fiber; isHydrating = true; + hydrationErrors = null; if (treeContext !== null) { restoreSuspendedTreeContext(fiber, treeContext); } @@ -601,10 +607,28 @@ function resetHydrationState(): void { isHydrating = false; } +export function queueRecoverableHydrationErrors(): void { + if (hydrationErrors !== null) { + // Successfully completed a forced client render. The errors that occurred + // during the hydration attempt are now recovered. We will log them in + // commit phase, once the entire tree has finished. + queueRecoverableErrors(hydrationErrors); + hydrationErrors = null; + } +} + function getIsHydrating(): boolean { return isHydrating; } +export function queueHydrationError(error: mixed): void { + if (hydrationErrors === null) { + hydrationErrors = [error]; + } else { + hydrationErrors.push(error); + } +} + export { warnIfHydrating, enterHydrationState, diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.old.js b/packages/react-reconciler/src/ReactFiberHydrationContext.old.js index 9e2518542454a..e5583916fbbd7 100644 --- a/packages/react-reconciler/src/ReactFiberHydrationContext.old.js +++ b/packages/react-reconciler/src/ReactFiberHydrationContext.old.js @@ -77,6 +77,7 @@ import { getSuspendedTreeContext, restoreSuspendedTreeContext, } from './ReactFiberTreeContext.old'; +import {queueRecoverableErrors} from './ReactFiberWorkLoop.old'; // The deepest Fiber on the stack involved in a hydration context. // This may have been an insertion or a hydration. @@ -84,6 +85,9 @@ let hydrationParentFiber: null | Fiber = null; let nextHydratableInstance: null | HydratableInstance = null; let isHydrating: boolean = false; +// Hydration errors that were thrown inside this boundary +let hydrationErrors: Array | null = null; + function warnIfHydrating() { if (__DEV__) { if (isHydrating) { @@ -105,6 +109,7 @@ function enterHydrationState(fiber: Fiber): boolean { ); hydrationParentFiber = fiber; isHydrating = true; + hydrationErrors = null; return true; } @@ -121,6 +126,7 @@ function reenterHydrationStateFromDehydratedSuspenseInstance( ); hydrationParentFiber = fiber; isHydrating = true; + hydrationErrors = null; if (treeContext !== null) { restoreSuspendedTreeContext(fiber, treeContext); } @@ -601,10 +607,28 @@ function resetHydrationState(): void { isHydrating = false; } +export function queueRecoverableHydrationErrors(): void { + if (hydrationErrors !== null) { + // Successfully completed a forced client render. The errors that occurred + // during the hydration attempt are now recovered. We will log them in + // commit phase, once the entire tree has finished. + queueRecoverableErrors(hydrationErrors); + hydrationErrors = null; + } +} + function getIsHydrating(): boolean { return isHydrating; } +export function queueHydrationError(error: mixed): void { + if (hydrationErrors === null) { + hydrationErrors = [error]; + } else { + hydrationErrors.push(error); + } +} + export { warnIfHydrating, enterHydrationState, diff --git a/packages/react-reconciler/src/ReactFiberThrow.new.js b/packages/react-reconciler/src/ReactFiberThrow.new.js index c2b2a9a2fa5e7..6a0c70f8e6d00 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.new.js +++ b/packages/react-reconciler/src/ReactFiberThrow.new.js @@ -37,7 +37,6 @@ import { import { supportsPersistence, getOffscreenContainerProps, - logRecoverableError, } from './ReactFiberHostConfig'; import {shouldCaptureSuspense} from './ReactFiberSuspenseComponent.new'; import {NoMode, ConcurrentMode, DebugTracingMode} from './ReactTypeOfMode'; @@ -80,7 +79,10 @@ import { mergeLanes, pickArbitraryLane, } from './ReactFiberLane.new'; -import {getIsHydrating} from './ReactFiberHydrationContext.new'; +import { + getIsHydrating, + queueHydrationError, +} from './ReactFiberHydrationContext.new'; const PossiblyWeakMap = typeof WeakMap === 'function' ? WeakMap : Map; @@ -511,11 +513,7 @@ function throwException( // Even though the user may not be affected by this error, we should // still log it so it can be fixed. - // TODO: For now, we only log errors that occur during hydration, but we - // 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); + queueHydrationError(value); return; } } else { diff --git a/packages/react-reconciler/src/ReactFiberThrow.old.js b/packages/react-reconciler/src/ReactFiberThrow.old.js index 3ae5df1f93414..21ab03f4ac925 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.old.js +++ b/packages/react-reconciler/src/ReactFiberThrow.old.js @@ -37,7 +37,6 @@ import { import { supportsPersistence, getOffscreenContainerProps, - logRecoverableError, } from './ReactFiberHostConfig'; import {shouldCaptureSuspense} from './ReactFiberSuspenseComponent.old'; import {NoMode, ConcurrentMode, DebugTracingMode} from './ReactTypeOfMode'; @@ -80,7 +79,10 @@ import { mergeLanes, pickArbitraryLane, } from './ReactFiberLane.old'; -import {getIsHydrating} from './ReactFiberHydrationContext.old'; +import { + getIsHydrating, + queueHydrationError, +} from './ReactFiberHydrationContext.old'; const PossiblyWeakMap = typeof WeakMap === 'function' ? WeakMap : Map; @@ -511,11 +513,7 @@ function throwException( // Even though the user may not be affected by this error, we should // still log it so it can be fixed. - // TODO: For now, we only log errors that occur during hydration, but we - // 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); + queueHydrationError(value); return; } } else { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index edc6037a67057..f444690504316 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -909,14 +909,7 @@ function recoverFromConcurrentError(root, errorRetryLanes) { // 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, - ); - } + queueRecoverableErrors(errorsFromFirstAttempt); } } else { // The UI failed to recover. @@ -927,6 +920,17 @@ function recoverFromConcurrentError(root, errorRetryLanes) { return exitStatus; } +export function queueRecoverableErrors(errors: Array) { + if (workInProgressRootConcurrentErrors === null) { + workInProgressRootRecoverableErrors = errors; + } else { + workInProgressRootConcurrentErrors = workInProgressRootConcurrentErrors.push.apply( + null, + errors, + ); + } +} + function finishConcurrentRender(root, exitStatus, lanes) { switch (exitStatus) { case RootIncomplete: diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 4eb05f2ce40d0..5355d2992293a 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -909,14 +909,7 @@ function recoverFromConcurrentError(root, errorRetryLanes) { // 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, - ); - } + queueRecoverableErrors(errorsFromFirstAttempt); } } else { // The UI failed to recover. @@ -927,6 +920,17 @@ function recoverFromConcurrentError(root, errorRetryLanes) { return exitStatus; } +export function queueRecoverableErrors(errors: Array) { + if (workInProgressRootConcurrentErrors === null) { + workInProgressRootRecoverableErrors = errors; + } else { + workInProgressRootConcurrentErrors = workInProgressRootConcurrentErrors.push.apply( + null, + errors, + ); + } +} + function finishConcurrentRender(root, exitStatus, lanes) { switch (exitStatus) { case RootIncomplete: From 2ab18ffa44400f84ec6fcb71d14f825910182be0 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 3 Feb 2022 18:09:20 -0500 Subject: [PATCH 5/8] 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. --- .../src/__tests__/ReactDOMFizzServer-test.js | 28 ++++++++++++++++--- ...DOMServerPartialHydration-test.internal.js | 8 +++++- .../src/client/ReactDOMHostConfig.js | 5 +--- 3 files changed, 32 insertions(+), 9 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index 0b40c49b66e8e..483c588c37b9f 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -1723,12 +1723,22 @@ describe('ReactDOMFizzServer', () => { }); expect(Scheduler).toHaveYielded(['server']); - ReactDOM.hydrateRoot(container, ); + ReactDOM.hydrateRoot(container, , { + 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}, @@ -1805,13 +1815,23 @@ describe('ReactDOMFizzServer', () => { }); expect(Scheduler).toHaveYielded(['server']); - ReactDOM.hydrateRoot(container, ); + ReactDOM.hydrateRoot(container, , { + 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}, diff --git a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js index 1e819091bf4c7..e071a36737104 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js @@ -3019,7 +3019,13 @@ describe('ReactDOMServerPartialHydration', () => { const span = container.getElementsByTagName('span')[0]; expect(span.innerHTML).toBe('Hidden child'); - ReactDOM.hydrateRoot(container, ); + ReactDOM.hydrateRoot(container, , { + onRecoverableError(error) { + Scheduler.unstable_yieldValue( + 'Log recoverable error: ' + error.message, + ); + }, + }); Scheduler.unstable_flushAll(); expect(ref.current).toBe(span); diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index fdd8b38e312b7..625f53e19849c 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -385,14 +385,11 @@ export function logRecoverableError( ): 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, () => { + queueMicrotask(() => { throw error; }); } From 73ff09120567399f1591e12d78706e9233be40cc Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 3 Feb 2022 19:01:53 -0500 Subject: [PATCH 6/8] 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. --- packages/react-art/src/ReactARTHostConfig.js | 2 +- .../src/client/ReactDOMHostConfig.js | 25 +++++-------------- .../src/ReactFabricHostConfig.js | 7 +----- .../src/ReactNativeHostConfig.js | 7 +----- .../src/createReactNoop.js | 13 +++++++++- .../src/ReactFiberReconciler.new.js | 5 ++-- .../src/ReactFiberReconciler.old.js | 5 ++-- .../src/ReactFiberRoot.new.js | 9 +++---- .../src/ReactFiberRoot.old.js | 9 +++---- .../src/ReactFiberWorkLoop.new.js | 9 ++++++- .../src/ReactFiberWorkLoop.old.js | 9 ++++++- .../src/ReactInternalTypes.js | 7 ++---- .../src/forks/ReactFiberHostConfig.custom.js | 1 - .../src/ReactTestHostConfig.js | 7 +----- 14 files changed, 52 insertions(+), 63 deletions(-) diff --git a/packages/react-art/src/ReactARTHostConfig.js b/packages/react-art/src/ReactARTHostConfig.js index 4ec8e3a4e6f2e..92417f77ae163 100644 --- a/packages/react-art/src/ReactARTHostConfig.js +++ b/packages/react-art/src/ReactARTHostConfig.js @@ -452,6 +452,6 @@ export function detachDeletedInstance(node: Instance): void { // noop } -export function logRecoverableError(config, error) { +export function logRecoverableError(error) { // noop } diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index 625f53e19849c..fdc66db2f44f9 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -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 = { @@ -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, @@ -379,20 +374,12 @@ export function getCurrentEventPriority(): * { return getEventPriority(currentEvent.type); } -export function logRecoverableError( - config: ErrorLoggingConfig, - error: mixed, -): void { - const onRecoverableError = config; - if (onRecoverableError !== null) { - onRecoverableError(error); - } else { - // Default behavior is to rethrow the error in a separate task. This will - // trigger a browser error event. - queueMicrotask(() => { - 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; + }); } export const isPrimaryRenderer = true; diff --git a/packages/react-native-renderer/src/ReactFabricHostConfig.js b/packages/react-native-renderer/src/ReactFabricHostConfig.js index e720c2e12534e..bec60bff22c99 100644 --- a/packages/react-native-renderer/src/ReactFabricHostConfig.js +++ b/packages/react-native-renderer/src/ReactFabricHostConfig.js @@ -95,8 +95,6 @@ export type RendererInspectionConfig = $ReadOnly<{| ) => void, |}>; -export type ErrorLoggingConfig = null; - // TODO: Remove this conditional once all changes have propagated. if (registerEventHandler) { /** @@ -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 } diff --git a/packages/react-native-renderer/src/ReactNativeHostConfig.js b/packages/react-native-renderer/src/ReactNativeHostConfig.js index 27df360718aee..e1b1d25f5f60e 100644 --- a/packages/react-native-renderer/src/ReactNativeHostConfig.js +++ b/packages/react-native-renderer/src/ReactNativeHostConfig.js @@ -55,8 +55,6 @@ export type RendererInspectionConfig = $ReadOnly<{| ) => void, |}>; -export type ErrorLoggingConfig = null; - const UPDATE_SIGNAL = {}; if (__DEV__) { Object.freeze(UPDATE_SIGNAL); @@ -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 } diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 411757c0436be..7e1ee5c57581b 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -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; @@ -979,6 +988,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { null, false, '', + null, ); return { _Scheduler: Scheduler, @@ -1008,6 +1018,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { null, false, '', + null, ); return { _Scheduler: Scheduler, diff --git a/packages/react-reconciler/src/ReactFiberReconciler.new.js b/packages/react-reconciler/src/ReactFiberReconciler.new.js index 93a3e13bef85a..0c99b9ec3077d 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.new.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.new.js @@ -15,7 +15,6 @@ import type { TextInstance, Container, PublicInstance, - ErrorLoggingConfig, } from './ReactFiberHostConfig'; import type {RendererInspectionConfig} from './ReactFiberHostConfig'; import type {ReactNodeList} from 'shared/ReactTypes'; @@ -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, @@ -256,7 +255,7 @@ export function createContainer( isStrictMode, concurrentUpdatesByDefaultOverride, identifierPrefix, - errorLoggingConfig, + onRecoverableError, ); } diff --git a/packages/react-reconciler/src/ReactFiberReconciler.old.js b/packages/react-reconciler/src/ReactFiberReconciler.old.js index 0fcba6293c7b7..202d7ce3819dc 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.old.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.old.js @@ -15,7 +15,6 @@ import type { TextInstance, Container, PublicInstance, - ErrorLoggingConfig, } from './ReactFiberHostConfig'; import type {RendererInspectionConfig} from './ReactFiberHostConfig'; import type {ReactNodeList} from 'shared/ReactTypes'; @@ -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, @@ -256,7 +255,7 @@ export function createContainer( isStrictMode, concurrentUpdatesByDefaultOverride, identifierPrefix, - errorLoggingConfig, + onRecoverableError, ); } diff --git a/packages/react-reconciler/src/ReactFiberRoot.new.js b/packages/react-reconciler/src/ReactFiberRoot.new.js index dd5e6a5fc7da0..85820dc7a3cdf 100644 --- a/packages/react-reconciler/src/ReactFiberRoot.new.js +++ b/packages/react-reconciler/src/ReactFiberRoot.new.js @@ -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'; @@ -36,7 +35,7 @@ function FiberRootNode( tag, hydrate, identifierPrefix, - errorLoggingConfig, + onRecoverableError, ) { this.tag = tag; this.containerInfo = containerInfo; @@ -64,7 +63,7 @@ function FiberRootNode( this.entanglements = createLaneMap(NoLanes); this.identifierPrefix = identifierPrefix; - this.errorLoggingConfig = errorLoggingConfig; + this.onRecoverableError = onRecoverableError; if (enableCache) { this.pooledCache = null; @@ -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; diff --git a/packages/react-reconciler/src/ReactFiberRoot.old.js b/packages/react-reconciler/src/ReactFiberRoot.old.js index d6791e97c34fd..e1eaee798bfb2 100644 --- a/packages/react-reconciler/src/ReactFiberRoot.old.js +++ b/packages/react-reconciler/src/ReactFiberRoot.old.js @@ -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'; @@ -36,7 +35,7 @@ function FiberRootNode( tag, hydrate, identifierPrefix, - errorLoggingConfig, + onRecoverableError, ) { this.tag = tag; this.containerInfo = containerInfo; @@ -64,7 +63,7 @@ function FiberRootNode( this.entanglements = createLaneMap(NoLanes); this.identifierPrefix = identifierPrefix; - this.errorLoggingConfig = errorLoggingConfig; + this.onRecoverableError = onRecoverableError; if (enableCache) { this.pooledCache = null; @@ -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; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index f444690504316..f3efac3e74d68 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -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); + } } } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 5355d2992293a..d6e37f2785ff2 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -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); + } } } diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js index 1cc2128356090..bbfe70c26cfc4 100644 --- a/packages/react-reconciler/src/ReactInternalTypes.js +++ b/packages/react-reconciler/src/ReactInternalTypes.js @@ -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'; @@ -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. diff --git a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js index 8e67bf5517e45..ff5bbf8035f07 100644 --- a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js +++ b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js @@ -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; diff --git a/packages/react-test-renderer/src/ReactTestHostConfig.js b/packages/react-test-renderer/src/ReactTestHostConfig.js index 266b2c06e58a1..840912db30886 100644 --- a/packages/react-test-renderer/src/ReactTestHostConfig.js +++ b/packages/react-test-renderer/src/ReactTestHostConfig.js @@ -42,8 +42,6 @@ export type EventResponder = any; export type RendererInspectionConfig = $ReadOnly<{||}>; -export type ErrorLoggingConfig = null; - export * from 'react-reconciler/src/ReactFiberHostConfigWithNoPersistence'; export * from 'react-reconciler/src/ReactFiberHostConfigWithNoHydration'; export * from 'react-reconciler/src/ReactFiberHostConfigWithNoTestSelectors'; @@ -317,9 +315,6 @@ export function detachDeletedInstance(node: Instance): void { // noop } -export function logRecoverableError( - config: ErrorLoggingConfig, - error: mixed, -): void { +export function logRecoverableError(error: mixed): void { // noop } From 61ef8f420aca10566e746d7f10416f86e88dd4ab Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 4 Feb 2022 10:09:04 -0500 Subject: [PATCH 7/8] 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. --- .../react-dom/src/client/ReactDOMHostConfig.js | 18 +++++++++++------- scripts/flow/environment.js | 1 + scripts/print-warnings/print-warnings.js | 10 ++++------ scripts/rollup/validate/eslintrc.cjs.js | 1 + scripts/rollup/validate/eslintrc.cjs2015.js | 1 + scripts/rollup/validate/eslintrc.esm.js | 1 + scripts/rollup/validate/eslintrc.fb.js | 1 + scripts/rollup/validate/eslintrc.rn.js | 1 + scripts/rollup/validate/eslintrc.umd.js | 1 + 9 files changed, 22 insertions(+), 13 deletions(-) diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index fdc66db2f44f9..e2cbed61e5e42 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -374,13 +374,17 @@ export function getCurrentEventPriority(): * { return getEventPriority(currentEvent.type); } -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; - }); -} +/* global reportError */ +export const logRecoverableError = + typeof reportError === 'function' + ? // In modern browsers, reportError will dispatch an error event, + // emulating an uncaught JavaScript error. + reportError + : (error: mixed) => { + // In older browsers and test environments, fallback to console.error. + // eslint-disable-next-line react-internal/no-production-logging, react-internal/warning-args + console.error(error); + }; export const isPrimaryRenderer = true; export const warnsIfNotActing = true; diff --git a/scripts/flow/environment.js b/scripts/flow/environment.js index f44d4ed7fcd1b..4294964a74b98 100644 --- a/scripts/flow/environment.js +++ b/scripts/flow/environment.js @@ -19,6 +19,7 @@ declare var __REACT_DEVTOOLS_GLOBAL_HOOK__: any; /*?{ };*/ declare var queueMicrotask: (fn: Function) => void; +declare var reportError: (error: mixed) => void; declare module 'create-react-class' { declare var exports: React$CreateClass; diff --git a/scripts/print-warnings/print-warnings.js b/scripts/print-warnings/print-warnings.js index 8e23dd880e92b..52fcb9630e1fa 100644 --- a/scripts/print-warnings/print-warnings.js +++ b/scripts/print-warnings/print-warnings.js @@ -67,12 +67,10 @@ function transform(file, enc, cb) { const warningMsgLiteral = evalStringConcat(node.arguments[0]); warnings.add(JSON.stringify(warningMsgLiteral)); } catch (error) { - console.error( - 'Failed to extract warning message from', - file.path - ); - console.error(astPath.node.loc); - throw error; + // Silently skip over this call. We have a lint rule to enforce + // that all calls are extractable, so if this one fails, assume + // it's intentional. + return; } } }, diff --git a/scripts/rollup/validate/eslintrc.cjs.js b/scripts/rollup/validate/eslintrc.cjs.js index 45b7a41e7e596..802141d6bc101 100644 --- a/scripts/rollup/validate/eslintrc.cjs.js +++ b/scripts/rollup/validate/eslintrc.cjs.js @@ -31,6 +31,7 @@ module.exports = { ArrayBuffer: 'readonly', TaskController: 'readonly', + reportError: 'readonly', // Flight Uint8Array: 'readonly', diff --git a/scripts/rollup/validate/eslintrc.cjs2015.js b/scripts/rollup/validate/eslintrc.cjs2015.js index 7135a5a706cff..b579566145778 100644 --- a/scripts/rollup/validate/eslintrc.cjs2015.js +++ b/scripts/rollup/validate/eslintrc.cjs2015.js @@ -30,6 +30,7 @@ module.exports = { ArrayBuffer: 'readonly', TaskController: 'readonly', + reportError: 'readonly', // Flight Uint8Array: 'readonly', diff --git a/scripts/rollup/validate/eslintrc.esm.js b/scripts/rollup/validate/eslintrc.esm.js index 34cde555478ab..23eb027071a23 100644 --- a/scripts/rollup/validate/eslintrc.esm.js +++ b/scripts/rollup/validate/eslintrc.esm.js @@ -30,6 +30,7 @@ module.exports = { ArrayBuffer: 'readonly', TaskController: 'readonly', + reportError: 'readonly', // Flight Uint8Array: 'readonly', diff --git a/scripts/rollup/validate/eslintrc.fb.js b/scripts/rollup/validate/eslintrc.fb.js index d267ca2bec308..2f8740049236c 100644 --- a/scripts/rollup/validate/eslintrc.fb.js +++ b/scripts/rollup/validate/eslintrc.fb.js @@ -31,6 +31,7 @@ module.exports = { ArrayBuffer: 'readonly', TaskController: 'readonly', + reportError: 'readonly', // Flight Uint8Array: 'readonly', diff --git a/scripts/rollup/validate/eslintrc.rn.js b/scripts/rollup/validate/eslintrc.rn.js index e466784e8fcb3..21d8397487ff2 100644 --- a/scripts/rollup/validate/eslintrc.rn.js +++ b/scripts/rollup/validate/eslintrc.rn.js @@ -31,6 +31,7 @@ module.exports = { ArrayBuffer: 'readonly', TaskController: 'readonly', + reportError: 'readonly', // jest jest: 'readonly', diff --git a/scripts/rollup/validate/eslintrc.umd.js b/scripts/rollup/validate/eslintrc.umd.js index 2d274375cca8c..223e96d28ef58 100644 --- a/scripts/rollup/validate/eslintrc.umd.js +++ b/scripts/rollup/validate/eslintrc.umd.js @@ -36,6 +36,7 @@ module.exports = { ArrayBuffer: 'readonly', TaskController: 'readonly', + reportError: 'readonly', // Flight Uint8Array: 'readonly', From 24abe608079947c6284d3da1d3dbae703bab86fa Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 4 Feb 2022 10:14:35 -0500 Subject: [PATCH 8/8] Naming nits queueRecoverableHydrationErrors -> upgradeHydrationErrorsToRecoverable --- .../src/ReactFiberCompleteWork.new.js | 4 ++-- .../src/ReactFiberCompleteWork.old.js | 4 ++-- .../src/ReactFiberHydrationContext.new.js | 2 +- .../src/ReactFiberHydrationContext.old.js | 2 +- packages/react-server/src/ReactFizzServer.js | 12 ++++++------ packages/react-server/src/ReactFlightServer.js | 10 +++++----- 6 files changed, 17 insertions(+), 17 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js index de5b2309146a7..39f724c76df92 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js @@ -131,7 +131,7 @@ import { resetHydrationState, getIsHydrating, hasUnhydratedTailNodes, - queueRecoverableHydrationErrors, + upgradeHydrationErrorsToRecoverable, } from './ReactFiberHydrationContext.new'; import { enableSuspenseCallback, @@ -1105,7 +1105,7 @@ function completeWork( // there may have been recoverable errors during first hydration // attempt. If so, add them to a queue so we can log them in the // commit phase. - queueRecoverableHydrationErrors(); + upgradeHydrationErrorsToRecoverable(); } if ((workInProgress.flags & DidCapture) !== NoFlags) { diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js index 025f65fa4eda0..c7baf36d16c47 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js @@ -131,7 +131,7 @@ import { resetHydrationState, getIsHydrating, hasUnhydratedTailNodes, - queueRecoverableHydrationErrors, + upgradeHydrationErrorsToRecoverable, } from './ReactFiberHydrationContext.old'; import { enableSuspenseCallback, @@ -1105,7 +1105,7 @@ function completeWork( // there may have been recoverable errors during first hydration // attempt. If so, add them to a queue so we can log them in the // commit phase. - queueRecoverableHydrationErrors(); + upgradeHydrationErrorsToRecoverable(); } if ((workInProgress.flags & DidCapture) !== NoFlags) { diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js index 1eae8d58ec363..4a460d584a53d 100644 --- a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js +++ b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js @@ -607,7 +607,7 @@ function resetHydrationState(): void { isHydrating = false; } -export function queueRecoverableHydrationErrors(): void { +export function upgradeHydrationErrorsToRecoverable(): void { if (hydrationErrors !== null) { // Successfully completed a forced client render. The errors that occurred // during the hydration attempt are now recovered. We will log them in diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.old.js b/packages/react-reconciler/src/ReactFiberHydrationContext.old.js index e5583916fbbd7..3ee040237829a 100644 --- a/packages/react-reconciler/src/ReactFiberHydrationContext.old.js +++ b/packages/react-reconciler/src/ReactFiberHydrationContext.old.js @@ -607,7 +607,7 @@ function resetHydrationState(): void { isHydrating = false; } -export function queueRecoverableHydrationErrors(): void { +export function upgradeHydrationErrorsToRecoverable(): void { if (hydrationErrors !== null) { // Successfully completed a forced client render. The errors that occurred // during the hydration attempt are now recovered. We will log them in diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index 5ce012a0f538a..2cf1e5c2fb31d 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -401,7 +401,7 @@ function popComponentStackInDEV(task: Task): void { } } -function reportError(request: Request, error: mixed): void { +function logRecoverableError(request: Request, error: mixed): void { // If this callback errors, we intentionally let that error bubble up to become a fatal error // so that someone fixes the error reporting instead of hiding it. const onError = request.onError; @@ -484,7 +484,7 @@ function renderSuspenseBoundary( } } catch (error) { contentRootSegment.status = ERRORED; - reportError(request, error); + logRecoverableError(request, error); newBoundary.forceClientRender = true; // We don't need to decrement any task numbers because we didn't spawn any new task. // We don't need to schedule any task because we know the parent has written yet. @@ -1337,7 +1337,7 @@ function erroredTask( error: mixed, ) { // Report the error to a global handler. - reportError(request, error); + logRecoverableError(request, error); if (boundary === null) { fatalError(request, error); } else { @@ -1557,7 +1557,7 @@ export function performWork(request: Request): void { flushCompletedQueues(request, request.destination); } } catch (error) { - reportError(request, error); + logRecoverableError(request, error); fatalError(request, error); } finally { setCurrentResponseState(prevResponseState); @@ -1945,7 +1945,7 @@ export function startFlowing(request: Request, destination: Destination): void { try { flushCompletedQueues(request, destination); } catch (error) { - reportError(request, error); + logRecoverableError(request, error); fatalError(request, error); } } @@ -1960,7 +1960,7 @@ export function abort(request: Request): void { flushCompletedQueues(request, request.destination); } } catch (error) { - reportError(request, error); + logRecoverableError(request, error); fatalError(request, error); } } diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index 418fbb8241a66..052fa730fd91e 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -421,7 +421,7 @@ export function resolveModelToJSON( x.then(ping, ping); return serializeByRefID(newSegment.id); } else { - reportError(request, x); + logRecoverableError(request, x); // Something errored. We'll still send everything we have up until this point. // We'll replace this element with a lazy reference that throws on the client // once it gets rendered. @@ -604,7 +604,7 @@ export function resolveModelToJSON( ); } -function reportError(request: Request, error: mixed): void { +function logRecoverableError(request: Request, error: mixed): void { const onError = request.onError; onError(error); } @@ -687,7 +687,7 @@ function retrySegment(request: Request, segment: Segment): void { x.then(ping, ping); return; } else { - reportError(request, x); + logRecoverableError(request, x); // This errored, we need to serialize this error to the emitErrorChunk(request, segment.id, x); } @@ -711,7 +711,7 @@ function performWork(request: Request): void { flushCompletedChunks(request, request.destination); } } catch (error) { - reportError(request, error); + logRecoverableError(request, error); fatalError(request, error); } finally { ReactCurrentDispatcher.current = prevDispatcher; @@ -794,7 +794,7 @@ export function startFlowing(request: Request, destination: Destination): void { try { flushCompletedChunks(request, destination); } catch (error) { - reportError(request, error); + logRecoverableError(request, error); fatalError(request, error); } }