From e7a4669e8addec764278457580a210c11395d0c5 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 9 Feb 2022 18:30:21 -0500 Subject: [PATCH] Resolve default onRecoverableError at root init Minor follow up to initial onRecoverableError PR. When onRecoverableError is not provided to `createRoot`, the renderer falls back to a default implementation. Originally I implemented this with a host config method, but what we can do instead is pass the default implementation the root constructor as if it were a user provided one. --- packages/react-art/src/ReactARTHostConfig.js | 4 ---- .../react-dom/src/client/ReactDOMHostConfig.js | 12 ------------ packages/react-dom/src/client/ReactDOMLegacy.js | 7 ++++++- packages/react-dom/src/client/ReactDOMRoot.js | 16 ++++++++++++++-- .../react-native-renderer/src/ReactFabric.js | 8 +++++++- .../src/ReactFabricHostConfig.js | 4 ---- .../src/ReactNativeHostConfig.js | 4 ---- .../src/ReactNativeRenderer.js | 8 +++++++- .../react-noop-renderer/src/createReactNoop.js | 12 +++++++++--- .../src/ReactFiberReconciler.new.js | 2 +- .../src/ReactFiberReconciler.old.js | 2 +- .../src/ReactFiberWorkLoop.new.js | 11 ++--------- .../src/ReactFiberWorkLoop.old.js | 11 ++--------- .../react-reconciler/src/ReactInternalTypes.js | 2 +- .../src/forks/ReactFiberHostConfig.custom.js | 1 - .../react-test-renderer/src/ReactTestRenderer.js | 8 +++++++- 16 files changed, 57 insertions(+), 55 deletions(-) diff --git a/packages/react-art/src/ReactARTHostConfig.js b/packages/react-art/src/ReactARTHostConfig.js index 92417f77ae163..47bced8a1274a 100644 --- a/packages/react-art/src/ReactARTHostConfig.js +++ b/packages/react-art/src/ReactARTHostConfig.js @@ -451,7 +451,3 @@ export function preparePortalMount(portalInstance: any): void { export function detachDeletedInstance(node: Instance): void { // noop } - -export function logRecoverableError(error) { - // noop -} diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index e2cbed61e5e42..08cd8cb81c787 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -374,18 +374,6 @@ export function getCurrentEventPriority(): * { return getEventPriority(currentEvent.type); } -/* 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; // 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 cb95101401ea4..b95be0bd3f22d 100644 --- a/packages/react-dom/src/client/ReactDOMLegacy.js +++ b/packages/react-dom/src/client/ReactDOMLegacy.js @@ -102,6 +102,11 @@ function getReactRootElementInContainer(container: any) { } } +function noopOnRecoverableError() { + // This isn't reachable because onRecoverableError isn't called in the + // legacy API. +} + function legacyCreateRootFromDOMContainer( container: Container, forceHydrate: boolean, @@ -122,7 +127,7 @@ function legacyCreateRootFromDOMContainer( false, // isStrictMode false, // concurrentUpdatesByDefaultOverride, '', // identifierPrefix - null, + noopOnRecoverableError, ); markContainerAsRoot(root.current, container); diff --git a/packages/react-dom/src/client/ReactDOMRoot.js b/packages/react-dom/src/client/ReactDOMRoot.js index fe6b6ee31f773..9867a1b45e5da 100644 --- a/packages/react-dom/src/client/ReactDOMRoot.js +++ b/packages/react-dom/src/client/ReactDOMRoot.js @@ -65,6 +65,18 @@ import { import {ConcurrentRoot} from 'react-reconciler/src/ReactRootTags'; import {allowConcurrentByDefault} from 'shared/ReactFeatureFlags'; +/* global reportError */ +const defaultOnRecoverableError = + 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); + }; + function ReactDOMRoot(internalRoot: FiberRoot) { this._internalRoot = internalRoot; } @@ -145,7 +157,7 @@ export function createRoot( let isStrictMode = false; let concurrentUpdatesByDefaultOverride = false; let identifierPrefix = ''; - let onRecoverableError = null; + let onRecoverableError = defaultOnRecoverableError; if (options !== null && options !== undefined) { if (__DEV__) { if ((options: any).hydrate) { @@ -220,7 +232,7 @@ export function hydrateRoot( let isStrictMode = false; let concurrentUpdatesByDefaultOverride = false; let identifierPrefix = ''; - let onRecoverableError = null; + let onRecoverableError = defaultOnRecoverableError; if (options !== null && options !== undefined) { if (options.unstable_strictMode === true) { isStrictMode = true; diff --git a/packages/react-native-renderer/src/ReactFabric.js b/packages/react-native-renderer/src/ReactFabric.js index 25cbd31b9fdf5..2f64c5a591994 100644 --- a/packages/react-native-renderer/src/ReactFabric.js +++ b/packages/react-native-renderer/src/ReactFabric.js @@ -195,6 +195,12 @@ function sendAccessibilityEvent(handle: any, eventType: string) { } } +function onRecoverableError(error) { + // TODO: Expose onRecoverableError option to userspace + // eslint-disable-next-line react-internal/no-production-logging, react-internal/warning-args + console.error(error); +} + function render( element: Element, containerTag: number, @@ -214,7 +220,7 @@ function render( false, null, '', - null, + onRecoverableError, ); roots.set(containerTag, root); } diff --git a/packages/react-native-renderer/src/ReactFabricHostConfig.js b/packages/react-native-renderer/src/ReactFabricHostConfig.js index bec60bff22c99..727b782efd768 100644 --- a/packages/react-native-renderer/src/ReactFabricHostConfig.js +++ b/packages/react-native-renderer/src/ReactFabricHostConfig.js @@ -525,7 +525,3 @@ export function preparePortalMount(portalInstance: Instance): void { export function detachDeletedInstance(node: Instance): void { // noop } - -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 e1b1d25f5f60e..10c5e37f41bcc 100644 --- a/packages/react-native-renderer/src/ReactNativeHostConfig.js +++ b/packages/react-native-renderer/src/ReactNativeHostConfig.js @@ -513,7 +513,3 @@ export function preparePortalMount(portalInstance: Instance): void { export function detachDeletedInstance(node: Instance): void { // noop } - -export function logRecoverableError(error: mixed): void { - // noop -} diff --git a/packages/react-native-renderer/src/ReactNativeRenderer.js b/packages/react-native-renderer/src/ReactNativeRenderer.js index c5d4318311c0b..2ea04acdcbe6c 100644 --- a/packages/react-native-renderer/src/ReactNativeRenderer.js +++ b/packages/react-native-renderer/src/ReactNativeRenderer.js @@ -192,6 +192,12 @@ function sendAccessibilityEvent(handle: any, eventType: string) { } } +function onRecoverableError(error) { + // TODO: Expose onRecoverableError option to userspace + // eslint-disable-next-line react-internal/no-production-logging, react-internal/warning-args + console.error(error); +} + function render( element: Element, containerTag: number, @@ -210,7 +216,7 @@ function render( false, null, '', - null, + onRecoverableError, ); roots.set(containerTag, root); } diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 7e1ee5c57581b..c314af59cb59a 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -938,6 +938,12 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { return NoopRenderer.flushSync(fn); } + function onRecoverableError(error) { + // TODO: Turn this on once tests are fixed + // eslint-disable-next-line react-internal/no-production-logging, react-internal/warning-args + // console.error(error); + } + let idCounter = 0; const ReactNoop = { @@ -966,7 +972,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { null, false, '', - null, + onRecoverableError, ); roots.set(rootID, root); } @@ -988,7 +994,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { null, false, '', - null, + onRecoverableError, ); return { _Scheduler: Scheduler, @@ -1018,7 +1024,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { null, false, '', - null, + onRecoverableError, ); return { _Scheduler: Scheduler, diff --git a/packages/react-reconciler/src/ReactFiberReconciler.new.js b/packages/react-reconciler/src/ReactFiberReconciler.new.js index 0c99b9ec3077d..d3130581d2ff8 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.new.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.new.js @@ -245,7 +245,7 @@ export function createContainer( isStrictMode: boolean, concurrentUpdatesByDefaultOverride: null | boolean, identifierPrefix: string, - onRecoverableError: null | ((error: mixed) => void), + onRecoverableError: (error: mixed) => void, ): OpaqueRoot { return createFiberRoot( containerInfo, diff --git a/packages/react-reconciler/src/ReactFiberReconciler.old.js b/packages/react-reconciler/src/ReactFiberReconciler.old.js index 202d7ce3819dc..19136883c778c 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.old.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.old.js @@ -245,7 +245,7 @@ export function createContainer( isStrictMode: boolean, concurrentUpdatesByDefaultOverride: null | boolean, identifierPrefix: string, - onRecoverableError: null | ((error: mixed) => void), + onRecoverableError: (error: mixed) => void, ): OpaqueRoot { return createFiberRoot( containerInfo, diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index f3efac3e74d68..33c3d2e24184e 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -77,7 +77,6 @@ import { supportsMicrotasks, errorHydratingContainer, scheduleMicrotask, - logRecoverableError, } from './ReactFiberHostConfig'; import { @@ -2113,16 +2112,10 @@ function commitRootImpl( 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. + const onRecoverableError = root.onRecoverableError; for (let i = 0; i < recoverableErrors.length; i++) { const recoverableError = recoverableErrors[i]; - 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); - } + onRecoverableError(recoverableError); } } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index d6e37f2785ff2..db3c194d2683e 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -77,7 +77,6 @@ import { supportsMicrotasks, errorHydratingContainer, scheduleMicrotask, - logRecoverableError, } from './ReactFiberHostConfig'; import { @@ -2113,16 +2112,10 @@ function commitRootImpl( 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. + const onRecoverableError = root.onRecoverableError; for (let i = 0; i < recoverableErrors.length; i++) { const recoverableError = recoverableErrors[i]; - 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); - } + onRecoverableError(recoverableError); } } diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js index bbfe70c26cfc4..352498be61f30 100644 --- a/packages/react-reconciler/src/ReactInternalTypes.js +++ b/packages/react-reconciler/src/ReactInternalTypes.js @@ -247,7 +247,7 @@ type BaseFiberRootProperties = {| // a reference to. identifierPrefix: string, - onRecoverableError: null | ((error: mixed) => void), + onRecoverableError: (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 ff5bbf8035f07..6535d8d3fdec3 100644 --- a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js +++ b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js @@ -68,7 +68,6 @@ export const prepareScopeUpdate = $$$hostConfig.preparePortalMount; export const getInstanceFromScope = $$$hostConfig.getInstanceFromScope; export const getCurrentEventPriority = $$$hostConfig.getCurrentEventPriority; export const detachDeletedInstance = $$$hostConfig.detachDeletedInstance; -export const logRecoverableError = $$$hostConfig.logRecoverableError; // ------------------- // Microtasks diff --git a/packages/react-test-renderer/src/ReactTestRenderer.js b/packages/react-test-renderer/src/ReactTestRenderer.js index a8121d1a14fcf..75d4de071bfd1 100644 --- a/packages/react-test-renderer/src/ReactTestRenderer.js +++ b/packages/react-test-renderer/src/ReactTestRenderer.js @@ -437,6 +437,12 @@ function propsMatch(props: Object, filter: Object): boolean { return true; } +function onRecoverableError(error) { + // TODO: Expose onRecoverableError option to userspace + // eslint-disable-next-line react-internal/no-production-logging, react-internal/warning-args + console.error(error); +} + function create(element: React$Element, options: TestRendererOptions) { let createNodeMock = defaultTestOptions.createNodeMock; let isConcurrent = false; @@ -472,7 +478,7 @@ function create(element: React$Element, options: TestRendererOptions) { isStrictMode, concurrentUpdatesByDefault, '', - null, + onRecoverableError, ); if (root == null) {