Repository: facebook/react
Pull request: [Flight] Override prepareStackTrace when reading stacks

This lets us ensure that we use the original V8 format and it lets us skip source mapping. Source mapping every call can be expensive since we do it eagerly for server components even if an error doesn't happen.

In the case of an error being thrown we don't actually always do this in practice because if a try/catch before us touches it or if something in onError touches it (which the default console.error does), it has already been initialized. So we have to be resilient to thrown errors having other formats.

These are not as perf sensitive since something actually threw but if you want better perf in these cases, you can simply do something like `onError(error) { console.error(error.message) }` instead.

The server has to be aware whether it's looking up original or compiled output. I currently use the file:// check to determine if it's referring to a source mapped file or compiled file in the fixture. A bundled app can more easily check if it's a bundle or not.

Changed files:
filename: .eslintrc.js
patch: @@ -486,6 +486,7 @@ module.exports = {
     $ReadOnlyArray: 'readonly',
     $ArrayBufferView: 'readonly',
     $Shape: 'readonly',
+    CallSite: 'readonly',
     ConsoleTask: 'readonly', // TOOD: Figure out what the official name of this will be.
     ReturnType: 'readonly',
     AnimationFrameID: 'readonly',

filename: fixtures/flight/server/region.js
patch: @@ -187,7 +187,11 @@ if (process.env.NODE_ENV === 'development') {
       res.set('Content-type', 'application/json');
       let requestedFilePath = req.query.name;
 
+      let isCompiledOutput = false;
       if (requestedFilePath.startsWith('file://')) {
+        // We assume that if it was prefixed with file:// it's referring to the compiled output
+        // and if it's a direct file path we assume it's source mapped back to original format.
+        isCompiledOutput = true;
         requestedFilePath = requestedFilePath.slice(7);
       }
 
@@ -204,11 +208,11 @@ if (process.env.NODE_ENV === 'development') {
       let map;
       // There are two ways to return a source map depending on what we observe in error.stack.
       // A real app will have a similar choice to make for which strategy to pick.
-      if (!sourceMap || Error.prepareStackTrace === undefined) {
-        // When --enable-source-maps is enabled, the error.stack that we use to track
-        // stacks will have had the source map already applied so it's pointing to the
-        // original source. We return a blank source map that just maps everything to
-        // the original source in this case.
+      if (!sourceMap || !isCompiledOutput) {
+        // If a file doesn't have a source map, such as this file, then we generate a blank
+        // source map that just contains the original content and segments pointing to the
+        // original lines.
+        // Similarly
         const sourceContent = await readFile(requestedFilePath, 'utf8');
         const lines = sourceContent.split('\n').length;
         map = {
@@ -222,13 +226,11 @@ if (process.env.NODE_ENV === 'development') {
           sourceRoot: '',
         };
       } else {
-        // If something has overridden prepareStackTrace it is likely not getting the
-        // natively applied source mapping to error.stack and so the line will point to
-        // the compiled output similar to how a browser works.
-        // E.g. ironically this can happen with the source-map-support library that is
-        // auto-invoked by @babel/register if external source maps are generated.
-        // In this case we just use the source map that the native source mapping would
-        // have used.
+        // We always set prepareStackTrace before reading the stack so that we get the stack
+        // without source maps applied. Therefore we have to use the original source map.
+        // If something read .stack before we did, we might observe the line/column after
+        // source mapping back to the original file. We use the isCompiledOutput check above
+        // in that case.
         map = sourceMap.payload;
       }
       res.write(JSON.stringify(map));

filename: packages/react-server/src/ReactFlightServer.js
patch: @@ -137,10 +137,41 @@ function isNotExternal(stackFrame: string): boolean {
   return !externalRegExp.test(stackFrame);
 }
 
+function prepareStackTrace(
+  error: Error,
+  structuredStackTrace: CallSite[],
+): string {
+  const name = error.name || 'Error';
+  const message = error.message || '';
+  let stack = name + ': ' + message;
+  for (let i = 0; i < structuredStackTrace.length; i++) {
+    stack += '\n    at ' + structuredStackTrace[i].toString();
+  }
+  return stack;
+}
+
+function getStack(error: Error): string {
+  // We override Error.prepareStackTrace with our own version that normalizes
+  // the stack to V8 formatting even if the server uses other formatting.
+  // It also ensures that source maps are NOT applied to this since that can
+  // be slow we're better off doing that lazily from the client instead of
+  // eagerly on the server. If the stack has already been read, then we might
+  // not get a normalized stack and it might still have been source mapped.
+  // So the client still needs to be resilient to this.
+  const previousPrepare = Error.prepareStackTrace;
+  Error.prepareStackTrace = prepareStackTrace;
+  try {
+    // eslint-disable-next-line react-internal/safe-string-coercion
+    return String(error.stack);
+  } finally {
+    Error.prepareStackTrace = previousPrepare;
+  }
+}
+
 function initCallComponentFrame(): string {
   // Extract the stack frame of the callComponentInDEV function.
   const error = callComponentInDEV(Error, 'react-stack-top-frame', {});
-  const stack = error.stack;
+  const stack = getStack(error);
   const startIdx = stack.startsWith('Error: react-stack-top-frame\n') ? 29 : 0;
   const endIdx = stack.indexOf('\n', startIdx);
   if (endIdx === -1) {
@@ -155,7 +186,7 @@ function initCallIteratorFrame(): string {
     (callIteratorInDEV: any)({next: null});
     return '';
   } catch (error) {
-    const stack = error.stack;
+    const stack = getStack(error);
     const startIdx = stack.startsWith('TypeError: ')
       ? stack.indexOf('\n') + 1
       : 0;
@@ -174,7 +205,7 @@ function initCallLazyInitFrame(): string {
     _init: Error,
     _payload: 'react-stack-top-frame',
   });
-  const stack = error.stack;
+  const stack = getStack(error);
   const startIdx = stack.startsWith('Error: react-stack-top-frame\n') ? 29 : 0;
   const endIdx = stack.indexOf('\n', startIdx);
   if (endIdx === -1) {
@@ -188,7 +219,7 @@ function filterDebugStack(error: Error): string {
   // to save bandwidth even in DEV. We'll also replay these stacks on the client so by
   // stripping them early we avoid that overhead. Otherwise we'd normally just rely on
   // the DevTools or framework's ignore lists to filter them out.
-  let stack = error.stack;
+  let stack = getStack(error);
   if (stack.startsWith('Error: react-stack-top-frame\n')) {
     // V8's default formatting prefixes with the error message which we
     // don't want/need.
@@ -2601,8 +2632,7 @@ function emitPostponeChunk(
     try {
       // eslint-disable-next-line react-internal/safe-string-coercion
       reason = String(postponeInstance.message);
-      // eslint-disable-next-line react-internal/safe-string-coercion
-      stack = String(postponeInstance.stack);
+      stack = getStack(postponeInstance);
     } catch (x) {}
     row = serializeRowHeader('P', id) + stringify({reason, stack}) + '\n';
   } else {
@@ -2627,8 +2657,7 @@ function emitErrorChunk(
       if (error instanceof Error) {
         // eslint-disable-next-line react-internal/safe-string-coercion
         message = String(error.message);
-        // eslint-disable-next-line react-internal/safe-string-coercion
-        stack = String(error.stack);
+        stack = getStack(error);
       } else if (typeof error === 'object' && error !== null) {
         message = describeObjectForErrorMessage(error);
       } else {