Resolve default onRecoverableError at root init (facebook#23264)

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.
zhengjitf · Apr 15, 2022 · f804fbf · f804fbf
1 parent c91ba1a
commit f804fbf
Show file tree

Hide file tree

Showing 16 changed files with 57 additions and 55 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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<ElementType>,
   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
@@ -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
@@ -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
@@ -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<ElementType>,
   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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<any>, options: TestRendererOptions) {
   let createNodeMock = defaultTestOptions.createNodeMock;
   let isConcurrent = false;
@@ -472,7 +478,7 @@ function create(element: React$Element<any>, options: TestRendererOptions) {
     isStrictMode,
     concurrentUpdatesByDefault,
     '',
-    null,
+    onRecoverableError,
   );
 
   if (root == null) {