From 8ba9d5eafb3336a3b6b68d7555e7901845e28d08 Mon Sep 17 00:00:00 2001
From: Josh Story <story@hey.com>
Date: Wed, 18 May 2022 08:55:16 -0700
Subject: [PATCH] Prod can now send error messages along with a hash, however
 this is currently only used when the server aborts

On the client, the error messages received is used or a generic fallback error message is used. In all environments the hash is attached to the error if found. In Dev, the component stack will be included as well. This gives us more options to inspect the error in onRecoverableError on the client while unifying the code paths in dev/prod
---
 .../src/__tests__/ReactDOMFizzServer-test.js  | 143 ++++++++++++------
 .../src/server/ReactDOMServerFormatConfig.js  |  74 ++++-----
 .../src/ReactFiberBeginWork.new.js            |  49 ++----
 .../src/ReactFiberBeginWork.old.js            |  49 ++----
 .../src/forks/ReactFiberHostConfig.custom.js  |   2 -
 packages/react-server/src/ReactFizzServer.js  |  18 ++-
 6 files changed, 167 insertions(+), 168 deletions(-)

diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
index f574d34a1625d..3101239dedde5 100644
--- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
+++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
@@ -90,24 +90,37 @@ describe('ReactDOMFizzServer', () => {
   });
 
   function expectErrors(errorsArr, toBeDevArr, toBeProdArr) {
+    const mappedErrows = errorsArr.map(error => {
+      if (error.componentStack) {
+        return [
+          error.message,
+          error.hash,
+          normalizeCodeLocInfo(error.componentStack),
+        ];
+      } else if (error.hash) {
+        return [error.message, error.hash];
+      }
+      return error.message;
+    });
     if (__DEV__) {
-      expect(errorsArr.map(error => normalizeCodeLocInfo(error))).toEqual(
-        toBeDevArr.map(error => {
-          if (typeof error === 'string' || error instanceof String) {
-            return error;
-          }
-          let str = JSON.stringify(error).replace(/\\n/g, '\n');
-          // this gets stripped away by normalizeCodeLocInfo...
-          // Kind of hacky but lets strip it away here too just so they match...
-          // easier than fixing the regex to account for this edge case
-          if (str.endsWith('at **)"}')) {
-            str = str.replace(/at \*\*\)\"}$/, 'at **)');
-          }
-          return str;
-        }),
+      expect(mappedErrows).toEqual(
+        toBeDevArr,
+        // .map(([errorMessage, errorHash, errorComponentStack]) => {
+        //   if (typeof error === 'string' || error instanceof String) {
+        //     return error;
+        //   }
+        //   let str = JSON.stringify(error).replace(/\\n/g, '\n');
+        //   // this gets stripped away by normalizeCodeLocInfo...
+        //   // Kind of hacky but lets strip it away here too just so they match...
+        //   // easier than fixing the regex to account for this edge case
+        //   if (str.endsWith('at **)"}')) {
+        //     str = str.replace(/at \*\*\)\"}$/, 'at **)');
+        //   }
+        //   return str;
+        // }),
       );
     } else {
-      expect(errorsArr).toEqual(toBeProdArr);
+      expect(mappedErrows).toEqual(toBeProdArr);
     }
   }
 
@@ -453,7 +466,7 @@ describe('ReactDOMFizzServer', () => {
       // Attempt to hydrate the content.
       ReactDOMClient.hydrateRoot(container, <App isClient={true} />, {
         onRecoverableError(error) {
-          errors.push(error.message);
+          errors.push(error);
         },
       });
     };
@@ -501,12 +514,18 @@ describe('ReactDOMFizzServer', () => {
     expectErrors(
       errors,
       [
-        theError.message +
-          '\nServer Error Hash: ' +
-          expectedHash +
+        [
+          theError.message,
+          expectedHash,
           componentStack(['Lazy', 'Suspense', 'div', 'App']),
+        ],
+      ],
+      [
+        [
+          'The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering.',
+          expectedHash,
+        ],
       ],
-      [expectedHash],
     );
 
     // The client rendered HTML is now in place.
@@ -590,7 +609,7 @@ describe('ReactDOMFizzServer', () => {
     // Attempt to hydrate the content.
     ReactDOMClient.hydrateRoot(container, <App isClient={true} />, {
       onRecoverableError(error) {
-        errors.push(error.message);
+        errors.push(error);
       },
     });
     Scheduler.unstable_flushAll();
@@ -615,12 +634,18 @@ describe('ReactDOMFizzServer', () => {
     expectErrors(
       errors,
       [
-        theError.message +
-          '\nServer Error Hash: ' +
-          expectedHash +
+        [
+          theError.message,
+          expectedHash,
           componentStack(['~lazy-element~', 'Suspense', 'div', 'App']),
+        ],
+      ],
+      [
+        [
+          'The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering.',
+          expectedHash,
+        ],
       ],
-      [expectedHash],
     );
 
     // The client rendered HTML is now in place.
@@ -681,7 +706,7 @@ describe('ReactDOMFizzServer', () => {
     // Attempt to hydrate the content.
     ReactDOMClient.hydrateRoot(container, <App isClient={true} />, {
       onRecoverableError(error) {
-        errors.push(error.message);
+        errors.push(error);
       },
     });
     Scheduler.unstable_flushAll();
@@ -691,12 +716,18 @@ describe('ReactDOMFizzServer', () => {
     expectErrors(
       errors,
       [
-        theError.message +
-          '\nServer Error Hash: ' +
-          expectedHash +
+        [
+          theError.message,
+          expectedHash,
           componentStack(['Erroring', 'Suspense', 'div', 'App']),
+        ],
+      ],
+      [
+        [
+          'The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering.',
+          expectedHash,
+        ],
       ],
-      [expectedHash],
     );
   });
 
@@ -744,7 +775,7 @@ describe('ReactDOMFizzServer', () => {
     // Attempt to hydrate the content.
     ReactDOMClient.hydrateRoot(container, <App isClient={true} />, {
       onRecoverableError(error) {
-        errors.push(error.message);
+        errors.push(error);
       },
     });
     Scheduler.unstable_flushAll();
@@ -764,12 +795,18 @@ describe('ReactDOMFizzServer', () => {
     expectErrors(
       errors,
       [
-        theError.message +
-          '\nServer Error Hash: ' +
-          expectedHash +
+        [
+          theError.message,
+          expectedHash,
           componentStack(['Lazy', 'Suspense', 'div', 'App']),
+        ],
+      ],
+      [
+        [
+          'The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering.',
+          expectedHash,
+        ],
       ],
-      [expectedHash],
     );
 
     // The client rendered HTML is now in place.
@@ -1057,7 +1094,7 @@ describe('ReactDOMFizzServer', () => {
     // Attempt to hydrate the content.
     ReactDOMClient.hydrateRoot(container, <App />, {
       onRecoverableError(error) {
-        errors.push(error.message);
+        errors.push(error);
       },
     });
     Scheduler.unstable_flushAll();
@@ -1761,7 +1798,7 @@ describe('ReactDOMFizzServer', () => {
     // Attempt to hydrate the content.
     ReactDOMClient.hydrateRoot(container, <App isClient={true} />, {
       onRecoverableError(error) {
-        errors.push(error.message);
+        errors.push(error);
       },
     });
     Scheduler.unstable_flushAll();
@@ -1795,9 +1832,9 @@ describe('ReactDOMFizzServer', () => {
     expectErrors(
       errors,
       [
-        theError.message +
-          '\nServer Error Hash: ' +
-          expectedHash +
+        [
+          theError.message,
+          expectedHash,
           componentStack([
             'AsyncText',
             'h1',
@@ -1806,8 +1843,14 @@ describe('ReactDOMFizzServer', () => {
             'Suspense',
             'App',
           ]),
+        ],
+      ],
+      [
+        [
+          'The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering.',
+          expectedHash,
+        ],
       ],
-      [expectedHash],
     );
 
     // The client rendered HTML is now in place.
@@ -3114,8 +3157,6 @@ describe('ReactDOMFizzServer', () => {
     });
     //@gate experimental
     it('escapes such that attributes cannot be masked', async () => {
-      window.__outlet = {};
-
       const dangerousErrorString = '" data-msg="bad message" data-foo="';
       const theError = new Error(dangerousErrorString);
 
@@ -3154,7 +3195,7 @@ describe('ReactDOMFizzServer', () => {
       const errors = [];
       ReactDOMClient.hydrateRoot(container, <App isClient={true} />, {
         onRecoverableError(error) {
-          errors.push(error.message);
+          errors.push(error);
         },
       });
       expect(Scheduler).toFlushAndYield([]);
@@ -3163,12 +3204,18 @@ describe('ReactDOMFizzServer', () => {
       expectErrors(
         errors,
         [
-          theError.message +
-            '\nServer Error Hash: ' +
-            expectedHash +
+          [
+            theError.message,
+            expectedHash,
             componentStack(['Erroring', 'Suspense', 'div', 'App']),
+          ],
+        ],
+        [
+          [
+            'The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering.',
+            expectedHash,
+          ],
         ],
-        [expectedHash],
       );
     });
   });
diff --git a/packages/react-dom/src/server/ReactDOMServerFormatConfig.js b/packages/react-dom/src/server/ReactDOMServerFormatConfig.js
index baea9b2367b61..7e489d3e12492 100644
--- a/packages/react-dom/src/server/ReactDOMServerFormatConfig.js
+++ b/packages/react-dom/src/server/ReactDOMServerFormatConfig.js
@@ -1506,10 +1506,10 @@ const endSuspenseBoundary = stringToPrecomputedChunk('<!--/$-->');
 const clientRenderedSuspenseBoundaryError1 = stringToPrecomputedChunk(
   '<template data-hash="',
 );
-const clientRenderedSuspenseBoundaryErrorDevA = stringToPrecomputedChunk(
+const clientRenderedSuspenseBoundaryError1A = stringToPrecomputedChunk(
   '" data-msg="',
 );
-const clientRenderedSuspenseBoundaryErrorDevB = stringToPrecomputedChunk(
+const clientRenderedSuspenseBoundaryError1B = stringToPrecomputedChunk(
   '" data-stack="',
 );
 const clientRenderedSuspenseBoundaryError2 = stringToPrecomputedChunk(
@@ -1565,17 +1565,19 @@ export function writeStartClientRenderedSuspenseBoundary(
   if (errorHash) {
     writeChunk(destination, clientRenderedSuspenseBoundaryError1);
     writeChunk(destination, stringToChunk(escapeTextForBrowser(errorHash)));
+    // In prod errorMessage will usually be nullish but there is one case where
+    // it is used (currently when the server aborts the task) so we leave it ungated.
+    if (errorMesssage) {
+      writeChunk(destination, clientRenderedSuspenseBoundaryError1A);
+      writeChunk(
+        destination,
+        stringToChunk(escapeTextForBrowser(errorMesssage)),
+      );
+    }
     if (__DEV__) {
-      // in dev send message and component stack if they exist
-      if (errorMesssage) {
-        writeChunk(destination, clientRenderedSuspenseBoundaryErrorDevA);
-        writeChunk(
-          destination,
-          stringToChunk(escapeTextForBrowser(errorMesssage)),
-        );
-      }
+      // Component stacks are currently only captured in dev
       if (errorComponentStack) {
-        writeChunk(destination, clientRenderedSuspenseBoundaryErrorDevB);
+        writeChunk(destination, clientRenderedSuspenseBoundaryError1B);
         writeChunk(
           destination,
           stringToChunk(escapeTextForBrowser(errorComponentStack)),
@@ -1924,8 +1926,9 @@ const clientRenderScript1Full = stringToPrecomputedChunk(
   clientRenderFunction + ';$RX("',
 );
 const clientRenderScript1Partial = stringToPrecomputedChunk('$RX("');
-const clientRenderScript2 = stringToPrecomputedChunk('")</script>');
-const clientRenderErrorScriptArgInterstitial = stringToPrecomputedChunk('","');
+const clientRenderScript1A = stringToPrecomputedChunk('"');
+const clientRenderScript2 = stringToPrecomputedChunk(')</script>');
+const clientRenderErrorScriptArgInterstitial = stringToPrecomputedChunk(',');
 
 export function writeClientRenderBoundaryInstruction(
   destination: Destination,
@@ -1952,21 +1955,20 @@ export function writeClientRenderBoundaryInstruction(
   }
 
   writeChunk(destination, boundaryID);
+  writeChunk(destination, clientRenderScript1A);
   if (errorHash || errorMessage || errorComponentStack) {
     writeChunk(destination, clientRenderErrorScriptArgInterstitial);
-    if (errorHash)
-      writeChunk(
-        destination,
-        stringToChunk(escapeJSStringsForInstructionScripts(errorHash)),
-      );
+    writeChunk(
+      destination,
+      stringToChunk(escapeJSStringsForInstructionScripts(errorHash || '')),
+    );
   }
   if (errorMessage || errorComponentStack) {
     writeChunk(destination, clientRenderErrorScriptArgInterstitial);
-    if (errorMessage)
-      writeChunk(
-        destination,
-        stringToChunk(escapeJSStringsForInstructionScripts(errorMessage)),
-      );
+    writeChunk(
+      destination,
+      stringToChunk(escapeJSStringsForInstructionScripts(errorMessage || '')),
+    );
   }
   if (errorComponentStack) {
     writeChunk(destination, clientRenderErrorScriptArgInterstitial);
@@ -1978,28 +1980,14 @@ export function writeClientRenderBoundaryInstruction(
   return writeChunkAndReturn(destination, clientRenderScript2);
 }
 
-const regexForJSStringsInScripts = /[<\'\`\u2028\u2029]/g;
+const regexForJSStringsInScripts = /[<\u2028\u2029]/g;
 function escapeJSStringsForInstructionScripts(input: string): string {
-  if (typeof input !== 'string') {
-    // eslint-disable-next-line react-internal/prod-error-codes
-    throw new Error(
-      `regexForJSStringsInScripts must be passed a string but was passed something of type "${
-        input === null ? 'null' : typeof input
-      }" instead`,
-    );
-  }
-  let escaped = JSON.stringify(input);
-  escaped = escaped.replace(regexForJSStringsInScripts, match => {
+  const escaped = JSON.stringify(input);
+  return escaped.replace(regexForJSStringsInScripts, match => {
     switch (match) {
       // santizing breaking out of strings and script tags
       case '<':
         return '\\u003c';
-      case "'":
-        return "\\'";
-      // case '"':        // JSON.stringify already escaped these
-      //   return '\\"';
-      case '`':
-        return '\\`';
       case '\u2028':
         return '\\u2028';
       case '\u2029':
@@ -2012,10 +2000,4 @@ function escapeJSStringsForInstructionScripts(input: string): string {
       }
     }
   });
-  if (escaped[0] === '"' && escaped[escaped.length - 1] === '"') {
-    // This should always be true since JSON.stringify of a string produces a value wrapped in double quotes.
-    return escaped.slice(1, -1);
-  } else {
-    return escaped;
-  }
 }
diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js
index 1430782a6f933..0f0a4ba3d143a 100644
--- a/packages/react-reconciler/src/ReactFiberBeginWork.new.js
+++ b/packages/react-reconciler/src/ReactFiberBeginWork.new.js
@@ -158,7 +158,6 @@ import {
   isSuspenseInstancePending,
   isSuspenseInstanceFallback,
   getSuspenseInstanceFallbackErrorDetails,
-  getSuspenseInstanceFallbackErrorHash,
   registerSuspenseInstanceRetry,
   supportsHydration,
   isPrimaryRenderer,
@@ -2663,42 +2662,26 @@ function updateDehydratedSuspenseComponent(
     // This boundary is in a permanent fallback state. In this case, we'll never
     // get an update and we'll never be able to hydrate the final content. Let's just try the
     // client side render instead.
-    let serverError;
-    if (__DEV__) {
-      const {
-        errorMessage,
-        errorComponentStack,
-        errorHash,
-      } = getSuspenseInstanceFallbackErrorDetails(suspenseInstance);
-      if (errorMessage) {
-        serverError = errorMessage;
-      }
-      if (errorHash) {
-        serverError =
-          (serverError ? serverError + '\nServer Error Hash: ' : '') +
-          errorHash;
-      }
-      if (serverError && errorComponentStack) {
-        serverError += errorComponentStack || '';
-      }
-    } else {
-      serverError = getSuspenseInstanceFallbackErrorHash(suspenseInstance);
-    }
+    const {
+      errorMessage,
+      errorComponentStack,
+      errorHash,
+    } = getSuspenseInstanceFallbackErrorDetails(suspenseInstance);
+    const error = errorMessage
+      ? // eslint-disable-next-line react-internal/prod-error-codes
+        new Error(errorMessage)
+      : new Error(
+          'The server could not finish this Suspense boundary, likely ' +
+            'due to an error during server rendering. Switched to ' +
+            'client rendering.',
+        );
+    if (errorHash) error.hash = errorHash;
+    if (errorComponentStack) error.componentStack = errorComponentStack;
     return retrySuspenseComponentWithoutHydrating(
       current,
       workInProgress,
       renderLanes,
-      // TODO: The server should serialize the error message so we can log it
-      // here on the client. Or, in production, a hash/id that corresponds to
-      // the error.
-      serverError
-        ? // eslint-disable-next-line react-internal/prod-error-codes
-          new Error(serverError)
-        : new Error(
-            'The server could not finish this Suspense boundary, likely ' +
-              'due to an error during server rendering. Switched to ' +
-              'client rendering.',
-          ),
+      error,
     );
   }
 
diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.old.js b/packages/react-reconciler/src/ReactFiberBeginWork.old.js
index d3b18434d4a35..77d4b6fd97f46 100644
--- a/packages/react-reconciler/src/ReactFiberBeginWork.old.js
+++ b/packages/react-reconciler/src/ReactFiberBeginWork.old.js
@@ -158,7 +158,6 @@ import {
   isSuspenseInstancePending,
   isSuspenseInstanceFallback,
   getSuspenseInstanceFallbackErrorDetails,
-  getSuspenseInstanceFallbackErrorHash,
   registerSuspenseInstanceRetry,
   supportsHydration,
   isPrimaryRenderer,
@@ -2663,42 +2662,26 @@ function updateDehydratedSuspenseComponent(
     // This boundary is in a permanent fallback state. In this case, we'll never
     // get an update and we'll never be able to hydrate the final content. Let's just try the
     // client side render instead.
-    let serverError;
-    if (__DEV__) {
-      const {
-        errorMessage,
-        errorComponentStack,
-        errorHash,
-      } = getSuspenseInstanceFallbackErrorDetails(suspenseInstance);
-      if (errorMessage) {
-        serverError = errorMessage;
-      }
-      if (errorHash) {
-        serverError =
-          (serverError ? serverError + '\nServer Error Hash: ' : '') +
-          errorHash;
-      }
-      if (serverError && errorComponentStack) {
-        serverError += errorComponentStack || '';
-      }
-    } else {
-      serverError = getSuspenseInstanceFallbackErrorHash(suspenseInstance);
-    }
+    const {
+      errorMessage,
+      errorComponentStack,
+      errorHash,
+    } = getSuspenseInstanceFallbackErrorDetails(suspenseInstance);
+    const error = errorMessage
+      ? // eslint-disable-next-line react-internal/prod-error-codes
+        new Error(errorMessage)
+      : new Error(
+          'The server could not finish this Suspense boundary, likely ' +
+            'due to an error during server rendering. Switched to ' +
+            'client rendering.',
+        );
+    if (errorHash) error.hash = errorHash;
+    if (errorComponentStack) error.componentStack = errorComponentStack;
     return retrySuspenseComponentWithoutHydrating(
       current,
       workInProgress,
       renderLanes,
-      // TODO: The server should serialize the error message so we can log it
-      // here on the client. Or, in production, a hash/id that corresponds to
-      // the error.
-      serverError
-        ? // eslint-disable-next-line react-internal/prod-error-codes
-          new Error(serverError)
-        : new Error(
-            'The server could not finish this Suspense boundary, likely ' +
-              'due to an error during server rendering. Switched to ' +
-              'client rendering.',
-          ),
+      error,
     );
   }
 
diff --git a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js
index ca90551ea656f..8ef999f432fb1 100644
--- a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js
+++ b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js
@@ -138,8 +138,6 @@ export const isSuspenseInstanceFallback =
   $$$hostConfig.isSuspenseInstanceFallback;
 export const getSuspenseInstanceFallbackErrorDetails =
   $$$hostConfig.getSuspenseInstanceFallbackErrorDetails;
-export const getSuspenseInstanceFallbackErrorHash =
-  $$$hostConfig.getSuspenseInstanceFallbackErrorHash;
 export const registerSuspenseInstanceRetry =
   $$$hostConfig.registerSuspenseInstanceRetry;
 export const getNextHydratableSibling = $$$hostConfig.getNextHydratableSibling;
diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js
index 4cb163d8fe383..7bea87aafb4ad 100644
--- a/packages/react-server/src/ReactFizzServer.js
+++ b/packages/react-server/src/ReactFizzServer.js
@@ -447,7 +447,14 @@ function captureBoundaryErrorDetailsDev(
 function logRecoverableError(request: Request, error: any): ?string {
   // 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.
-  return request.onError(error);
+  const errorHash = request.onError(error);
+  if (errorHash != null && typeof errorHash !== 'string') {
+    // eslint-disable-next-line react-internal/prod-error-codes
+    throw new Error(
+      `onError returned something with a type other than "string". onError should return a string and may return null or undefined but must not return anything else. It received something of type "${typeof errorHash}" instead`,
+    );
+  }
+  return errorHash;
 }
 
 function fatalError(request: Request, error: mixed): void {
@@ -1427,14 +1434,14 @@ function erroredTask(
   error: mixed,
 ) {
   // Report the error to a global handler.
-  const errorMsg = logRecoverableError(request, error);
+  const errorHash = logRecoverableError(request, error);
   if (boundary === null) {
     fatalError(request, error);
   } else {
     boundary.pendingTasks--;
     if (!boundary.forceClientRender) {
       boundary.forceClientRender = true;
-      boundary.errorHash = errorMsg;
+      boundary.errorHash = errorHash;
       if (__DEV__) {
         captureBoundaryErrorDetailsDev(boundary, error);
       }
@@ -1492,9 +1499,8 @@ function abortTask(task: Task): void {
 
     if (!boundary.forceClientRender) {
       boundary.forceClientRender = true;
-      // Technically not a hash since there is no error to pass to onError. However it will be handled
-      // the way we want in both dev and prod modes on the client if we report it here as a hash
-      boundary.errorHash = 'This Suspense boundary was aborted by the server';
+      boundary.errorMessage =
+        'This Suspense boundary was aborted by the server';
       if (boundary.parentFlushed) {
         request.clientRenderedBoundaries.push(boundary);
       }