Account for Error stack frames having source maps already applied

Refactored code to account for the fact that, in some environments, Error stack frames will already have source maps applied (so the code does not need to re-map line and column numbers).

Author: Brian Vaughn

.eslintignore

@@ -18,6 +18,7 @@ packages/react-devtools-extensions/chrome/build
 packages/react-devtools-extensions/firefox/build
 packages/react-devtools-extensions/shared/build
 packages/react-devtools-extensions/src/__tests__/__source__/__compiled__/
+packages/react-devtools-extensions/src/ErrorTesterCompiled.js
 packages/react-devtools-inline/dist
 packages/react-devtools-shell/dist
 packages/react-devtools-scheduling-profiler/dist

.prettierignore

@@ -3,6 +3,7 @@ packages/react-devtools-extensions/chrome/build
 packages/react-devtools-extensions/firefox/build
 packages/react-devtools-extensions/shared/build
 packages/react-devtools-extensions/src/__tests__/__source__/__compiled__/
+packages/react-devtools-extensions/src/ErrorTesterCompiled.js
 packages/react-devtools-inline/dist
 packages/react-devtools-shell/dist
 packages/react-devtools-scheduling-profiler/dist

packages/react-devtools-extensions/src/ErrorTester.js

@@ -0,0 +1,27 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @flow
+ */
+
+import ErrorStackParser from 'error-stack-parser';
+import testErrorStack, {
+  SOURCE_STACK_FRAME_LINE_NUMBER,
+} from './ErrorTesterCompiled';
+
+// Source maps are automatically applied to Error stack frames.
+export let sourceMapsAreAppliedToErrors: boolean = false;
+
+try {
+  testErrorStack();
+} catch (error) {
+  const parsed = ErrorStackParser.parse(error);
+  const topStackFrame = parsed[0];
+  const lineNumber = topStackFrame.lineNumber;
+  if (lineNumber === SOURCE_STACK_FRAME_LINE_NUMBER) {
+    sourceMapsAreAppliedToErrors = true;
+  }
+}

packages/react-devtools-extensions/src/ErrorTesterCompiled.js

@@ -15,6 +15,10 @@ describe('parseHookNames', () => {
   beforeEach(() => {
     jest.resetModules();
 
+    jest.mock('source-map-support', () => {
+      console.trace('source-map-support');
+    });
+
     const {
       overrideFeatureFlags,
     } = require('react-devtools-shared/src/__tests__/utils');

packages/react-devtools-extensions/src/__tests__/parseHookNames-test.js

@@ -8,7 +8,6 @@
  */
 
 import traverse, {NodePath, Node} from '@babel/traverse';
-import {parse} from '@babel/parser';
 import {File} from '@babel/types';
 
 import type {HooksNode} from 'react-debug-tools/src/ReactDebugHooks';
@@ -21,6 +20,14 @@ export type SourceFileASTWithHookDetails = {
   source: string,
 };
 
+export type SourceMap = {|
+  mappings: string,
+  names: Array<string>,
+  sources: Array<string>,
+  sourcesContent: Array<string>,
+  version: number,
+|};
+
 const AST_NODE_TYPES = Object.freeze({
   CALL_EXPRESSION: 'CallExpression',
   MEMBER_EXPRESSION: 'MemberExpression',
@@ -56,37 +63,18 @@ export function filterMemberWithHookVariableName(hook: NodePath): boolean {
   );
 }
 
-// Provides the AST of the hook's source file
-export function getASTFromSourceMap(
+// Map the generated source line and column position to the original source and line.
+export function mapCompiledLineNumberToOriginalLineNumber(
   consumer: SourceConsumer,
   lineNumber: number,
   columnNumber: number,
-): SourceFileASTWithHookDetails | null {
-  // A check added to prevent parsing large files
-  const FAIL_SAFE_CHECK = 100000;
-
-  // Map the generated source line and column position to the original source and line.
-  const {line, source} = consumer.originalPositionFor({
+): number | null {
+  const {line} = consumer.originalPositionFor({
     line: lineNumber,
     column: columnNumber,
   });
 
-  if (line == null) {
-    console.error(
-      `Could not find source location (line: ${lineNumber}, column: ${columnNumber})`,
-    );
-    return null;
-  } else if (line > FAIL_SAFE_CHECK) {
-    console.error(`Source File: ${source} is too big`);
-    return null;
-  }
-
-  const sourceFileContent = consumer.sourceContentFor(source, true);
-  const sourceFileAST = parse(sourceFileContent, {
-    sourceType: 'unambiguous',
-    plugins: ['jsx', 'typescript'],
-  });
-  return {line, source, sourceFileAST};
+  return line;
 }
 
 // Returns all AST Nodes associated with 'potentialReactHookASTNode'

packages/react-devtools-extensions/src/astUtils.js

@@ -14,13 +14,14 @@ import {enableHookNameParsing} from 'react-devtools-feature-flags';
 import {SourceMapConsumer} from 'source-map';
 import {
   checkNodeLocation,
-  getASTFromSourceMap,
   getFilteredHookASTNodes,
   getHookName,
   getPotentialHookDeclarationsFromAST,
   isConfirmedHookDeclaration,
   isNonDeclarativePrimitiveHook,
+  mapCompiledLineNumberToOriginalLineNumber,
 } from './astUtils';
+import {sourceMapsAreAppliedToErrors} from './ErrorTester';
 
 import type {
   HooksNode,
@@ -29,18 +30,35 @@ import type {
 } from 'react-debug-tools/src/ReactDebugHooks';
 import type {HookNames} from 'react-devtools-shared/src/hookNamesCache';
 import type {Thenable} from 'shared/ReactTypes';
-import type {SourceConsumer} from './astUtils';
+import type {SourceConsumer, SourceMap} from './astUtils';
 
 const SOURCE_MAP_REGEX = / ?sourceMappingURL=([^\s'"]+)/gm;
 const ABSOLUTE_URL_REGEX = /^https?:\/\//i;
 const MAX_SOURCE_LENGTH = 100_000_000;
 
 type HookSourceData = {|
+  // Generated by react-debug-tools.
   hookSource: HookSource,
+
+  // AST for original source code; typically comes from a consumed source map.
+  originalSourceAST: mixed,
+
+  // Source code (React components or custom hooks) containing primitive hook calls.
+  // If no source map has been provided, this code will be the same as runtimeSourceCode.
+  originalSourceCode: string | null,
+
+  // Compiled code (React components or custom hooks) containing primitive hook calls.
+  runtimeSourceCode: string | null,
+
+  // APIs from source-map for parsing source maps (if detected).
   sourceConsumer: SourceConsumer | null,
-  sourceContents: string | null,
+
+  // External URL of source map.
+  // Sources without source maps (or with inline source maps) won't have this.
   sourceMapURL: string | null,
-  sourceMapContents: string | null,
+
+  // Parsed source map object.
+  sourceMapContents: SourceMap | null,
 |};
 
 export default async function parseHookNames(
@@ -72,8 +90,10 @@ export default async function parseHookNames(
       if (!fileNameToHookSourceData.has(fileName)) {
         fileNameToHookSourceData.set(fileName, {
           hookSource,
+          originalSourceAST: null,
+          originalSourceCode: null,
+          runtimeSourceCode: null,
           sourceConsumer: null,
-          sourceContents: null,
           sourceMapURL: null,
           sourceMapContents: null,
         });
@@ -85,7 +105,7 @@ export default async function parseHookNames(
 
   return loadSourceFiles(fileNameToHookSourceData)
     .then(() => extractAndLoadSourceMaps(fileNameToHookSourceData))
-    .then(() => parseSourceMaps(fileNameToHookSourceData))
+    .then(() => parseSourceAST(fileNameToHookSourceData))
     .then(() => findHookNames(hooksList, fileNameToHookSourceData));
 }
 
@@ -108,11 +128,10 @@ function extractAndLoadSourceMaps(
 ): Promise<*> {
   const promises = [];
   fileNameToHookSourceData.forEach(hookSourceData => {
-    const sourceMappingURLs = ((hookSourceData.sourceContents: any): string).match(
-      SOURCE_MAP_REGEX,
-    );
+    const runtimeSourceCode = ((hookSourceData.runtimeSourceCode: any): string);
+    const sourceMappingURLs = runtimeSourceCode.match(SOURCE_MAP_REGEX);
     if (sourceMappingURLs == null) {
-      // Maybe file has not been transformed; let's try to parse it as-is.
+      // Maybe file has not been transformed; we'll try to parse it as-is in parseSourceAST().
     } else {
       for (let i = 0; i < sourceMappingURLs.length; i++) {
         const sourceMappingURL = sourceMappingURLs[i];
@@ -217,63 +236,50 @@ function findHookNames(
         return null; // Should not be reachable.
       }
 
-      let hooksFromAST;
-      let potentialReactHookASTNode;
-      let sourceCode;
-
       const sourceConsumer = hookSourceData.sourceConsumer;
-      if (sourceConsumer) {
-        const astData = getASTFromSourceMap(
+
+      let originalSourceLineNumber;
+      if (sourceMapsAreAppliedToErrors || !sourceConsumer) {
+        // Either the current environment automatically applies source maps to errors,
+        // or the current code had no source map to begin with.
+        // Either way, we don't need to convert the Error stack frame locations.
+        originalSourceLineNumber = lineNumber;
+      } else {
+        originalSourceLineNumber = mapCompiledLineNumberToOriginalLineNumber(
           sourceConsumer,
           lineNumber,
           columnNumber,
         );
+      }
 
-        if (astData === null) {
-          return null;
-        }
-
-        const {sourceFileAST, line, source} = astData;
-
-        sourceCode = source;
-        hooksFromAST = getPotentialHookDeclarationsFromAST(sourceFileAST);
-
-        // Iterate through potential hooks and try to find the current hook.
-        // potentialReactHookASTNode will contain declarations of the form const X = useState(0);
-        // where X could be an identifier or an array pattern (destructuring syntax)
-        potentialReactHookASTNode = hooksFromAST.find(node => {
-          const nodeLocationCheck = checkNodeLocation(node, line);
-          const hookDeclaractionCheck = isConfirmedHookDeclaration(node);
-          return nodeLocationCheck && hookDeclaractionCheck;
-        });
-      } else {
-        sourceCode = hookSourceData.sourceContents;
-
-        // There's no source map to parse here so we can use the source contents directly.
-        const ast = parse(sourceCode, {
-          sourceType: 'unambiguous',
-          plugins: ['jsx', 'typescript'],
-        });
-        hooksFromAST = getPotentialHookDeclarationsFromAST(ast);
-        const line = ((hookSource.lineNumber: any): number);
-        potentialReactHookASTNode = hooksFromAST.find(node => {
-          const nodeLocationCheck = checkNodeLocation(node, line);
-          const hookDeclaractionCheck = isConfirmedHookDeclaration(node);
-          return nodeLocationCheck && hookDeclaractionCheck;
-        });
+      if (originalSourceLineNumber === null) {
+        return null;
       }
 
-      if (!sourceCode || !potentialReactHookASTNode) {
+      const hooksFromAST = getPotentialHookDeclarationsFromAST(
+        hookSourceData.originalSourceAST,
+      );
+      const potentialReactHookASTNode = hooksFromAST.find(node => {
+        const nodeLocationCheck = checkNodeLocation(
+          node,
+          ((originalSourceLineNumber: any): number),
+        );
+        const hookDeclaractionCheck = isConfirmedHookDeclaration(node);
+        return nodeLocationCheck && hookDeclaractionCheck;
+      });
+
+      if (!potentialReactHookASTNode) {
         return null;
       }
 
       // nodesAssociatedWithReactHookASTNode could directly be used to obtain the hook variable name
       // depending on the type of potentialReactHookASTNode
       try {
+        const originalSourceCode = ((hookSourceData.originalSourceCode: any): string);
         const nodesAssociatedWithReactHookASTNode = getFilteredHookASTNodes(
           potentialReactHookASTNode,
           hooksFromAST,
-          sourceCode,
+          originalSourceCode,
         );
 
         return getHookName(
@@ -305,19 +311,19 @@ function loadSourceFiles(
   const promises = [];
   fileNameToHookSourceData.forEach((hookSourceData, fileName) => {
     promises.push(
-      fetchFile(fileName).then(sourceContents => {
-        if (sourceContents.length > MAX_SOURCE_LENGTH) {
+      fetchFile(fileName).then(runtimeSourceCode => {
+        if (runtimeSourceCode.length > MAX_SOURCE_LENGTH) {
           throw Error('Source code too large to parse');
         }
 
-        hookSourceData.sourceContents = sourceContents;
+        hookSourceData.runtimeSourceCode = runtimeSourceCode;
       }),
     );
   });
   return Promise.all(promises);
 }
 
-async function parseSourceMaps(
+async function parseSourceAST(
   fileNameToHookSourceData: Map<string, HookSourceData>,
 ): Promise<*> {
   // SourceMapConsumer.initialize() does nothing when running in Node (aka Jest)
@@ -332,19 +338,41 @@ async function parseSourceMaps(
 
   const promises = [];
   fileNameToHookSourceData.forEach(hookSourceData => {
-    if (hookSourceData.sourceMapContents !== null) {
+    const {runtimeSourceCode, sourceMapContents} = hookSourceData;
+    if (sourceMapContents !== null) {
       // Parse and extract the AST from the source map.
       promises.push(
         SourceMapConsumer.with(
-          hookSourceData.sourceMapContents,
+          sourceMapContents,
           null,
           (sourceConsumer: SourceConsumer) => {
             hookSourceData.sourceConsumer = sourceConsumer;
+
+            // Now that the source map has been loaded,
+            // extract the original source for later.
+            const source = sourceMapContents.sources[0];
+            const originalSourceCode = sourceConsumer.sourceContentFor(
+              source,
+              true,
+            );
+
+            // Save the original source and parsed AST for later.
+            // TODO (named hooks) Cache this across components, per source/file name.
+            hookSourceData.originalSourceCode = originalSourceCode;
+            hookSourceData.originalSourceAST = parse(originalSourceCode, {
+              sourceType: 'unambiguous',
+              plugins: ['jsx', 'typescript'],
+            });
           },
         ),
       );
     } else {
-      // There's no source map to parse here so we can skip this step.
+      // There's no source map to parse here so we can just parse the original source itself.
+      hookSourceData.originalSourceCode = runtimeSourceCode;
+      hookSourceData.originalSourceAST = parse(runtimeSourceCode, {
+        sourceType: 'unambiguous',
+        plugins: ['jsx', 'typescript'],
+      });
     }
   });
   return Promise.all(promises);