From 09fe69c2e0c853eb92bf3a1262a6d92c5795a95b Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 28 Jun 2021 17:38:50 -0400 Subject: [PATCH] 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). --- .eslintignore | 1 + .prettierignore | 1 + .../src/ErrorTester.js | 27 ++++ .../src/ErrorTesterCompiled.js | 25 +++ .../src/__tests__/parseHookNames-test.js | 4 + .../react-devtools-extensions/src/astUtils.js | 38 ++--- .../src/parseHookNames.js | 142 +++++++++++------- scripts/prettier/index.js | 7 +- 8 files changed, 162 insertions(+), 83 deletions(-) create mode 100644 packages/react-devtools-extensions/src/ErrorTester.js create mode 100644 packages/react-devtools-extensions/src/ErrorTesterCompiled.js diff --git a/.eslintignore b/.eslintignore index ca57fb56bd6b6..97f07314340d6 100644 --- a/.eslintignore +++ b/.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 diff --git a/.prettierignore b/.prettierignore index 1a16848698185..d944a5e628fa7 100644 --- a/.prettierignore +++ b/.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 diff --git a/packages/react-devtools-extensions/src/ErrorTester.js b/packages/react-devtools-extensions/src/ErrorTester.js new file mode 100644 index 0000000000000..196f1071e0b4b --- /dev/null +++ b/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 testErorrStack, { + SOURCE_STACK_FRAME_LINE_NUMBER, +} from './ErrorTesterCompiled'; + +// Source maps are automatically applied to Error stack frames. +export let sourceMapsAreAppliedToErrors: boolean = false; + +try { + testErorrStack(); +} catch (error) { + const parsed = ErrorStackParser.parse(error); + const topStackFrame = parsed[0]; + const lineNumber = topStackFrame.lineNumber; + if (lineNumber === SOURCE_STACK_FRAME_LINE_NUMBER) { + sourceMapsAreAppliedToErrors = true; + } +} diff --git a/packages/react-devtools-extensions/src/ErrorTesterCompiled.js b/packages/react-devtools-extensions/src/ErrorTesterCompiled.js new file mode 100644 index 0000000000000..38c776ccebda3 --- /dev/null +++ b/packages/react-devtools-extensions/src/ErrorTesterCompiled.js @@ -0,0 +1,25 @@ +"use strict"; + +Object.defineProperty(exports, "__esModule", { + value: true +}); +exports.default = testErorrStack; +exports.SOURCE_STACK_FRAME_LINE_NUMBER = void 0; + +/** + * 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 + */ +function testErorrStack() { + // If source maps are applied, this Error will have a stack frame with line 12. + throw Error('Test Error stack'); +} + +const SOURCE_STACK_FRAME_LINE_NUMBER = 12; +exports.SOURCE_STACK_FRAME_LINE_NUMBER = SOURCE_STACK_FRAME_LINE_NUMBER; +//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbInRlc3RFcm9yclN0YWNrLmpzIl0sIm5hbWVzIjpbInRlc3RFcm9yclN0YWNrIiwiRXJyb3IiLCJTT1VSQ0VfU1RBQ0tfRlJBTUVfTElORV9OVU1CRVIiXSwibWFwcGluZ3MiOiI7Ozs7Ozs7O0FBQUE7Ozs7Ozs7O0FBU2UsU0FBU0EsY0FBVCxHQUEwQjtBQUN2QztBQUNBLFFBQU1DLEtBQUssQ0FBQyxrQkFBRCxDQUFYO0FBQ0Q7O0FBRU0sTUFBTUMsOEJBQThCLEdBQUcsRUFBdkMiLCJzb3VyY2VzQ29udGVudCI6WyIvKipcbiAqIENvcHlyaWdodCAoYykgRmFjZWJvb2ssIEluYy4gYW5kIGl0cyBhZmZpbGlhdGVzLlxuICpcbiAqIFRoaXMgc291cmNlIGNvZGUgaXMgbGljZW5zZWQgdW5kZXIgdGhlIE1JVCBsaWNlbnNlIGZvdW5kIGluIHRoZVxuICogTElDRU5TRSBmaWxlIGluIHRoZSByb290IGRpcmVjdG9yeSBvZiB0aGlzIHNvdXJjZSB0cmVlLlxuICpcbiAqIEBmbG93XG4gKi9cblxuZXhwb3J0IGRlZmF1bHQgZnVuY3Rpb24gdGVzdEVyb3JyU3RhY2soKSB7XG4gIC8vIElmIHNvdXJjZSBtYXBzIGFyZSBhcHBsaWVkLCB0aGlzIEVycm9yIHdpbGwgaGF2ZSBhIHN0YWNrIGZyYW1lIHdpdGggbGluZSAxMi5cbiAgdGhyb3cgRXJyb3IoJ1Rlc3QgRXJyb3Igc3RhY2snKTtcbn1cblxuZXhwb3J0IGNvbnN0IFNPVVJDRV9TVEFDS19GUkFNRV9MSU5FX05VTUJFUiA9IDEyOyJdfQ== +//# sourceURL=testErorrStack.js \ No newline at end of file diff --git a/packages/react-devtools-extensions/src/__tests__/parseHookNames-test.js b/packages/react-devtools-extensions/src/__tests__/parseHookNames-test.js index 74d2d50de6139..b41db8639e895 100644 --- a/packages/react-devtools-extensions/src/__tests__/parseHookNames-test.js +++ b/packages/react-devtools-extensions/src/__tests__/parseHookNames-test.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'); diff --git a/packages/react-devtools-extensions/src/astUtils.js b/packages/react-devtools-extensions/src/astUtils.js index 691787b986c8e..4c7d4a361d17c 100644 --- a/packages/react-devtools-extensions/src/astUtils.js +++ b/packages/react-devtools-extensions/src/astUtils.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, + sources: Array, + sourcesContent: Array, + 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' diff --git a/packages/react-devtools-extensions/src/parseHookNames.js b/packages/react-devtools-extensions/src/parseHookNames.js index 0694cdec56ffe..77846dd57477f 100644 --- a/packages/react-devtools-extensions/src/parseHookNames.js +++ b/packages/react-devtools-extensions/src/parseHookNames.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, ): 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); diff --git a/scripts/prettier/index.js b/scripts/prettier/index.js index 483d20e69d9c0..0be3fefdca68e 100644 --- a/scripts/prettier/index.js +++ b/scripts/prettier/index.js @@ -26,7 +26,12 @@ let didError = false; const files = glob .sync('**/*.js', { - ignore: ['**/node_modules/**', '**/cjs/**', '**/__compiled__/**'], + ignore: [ + '**/node_modules/**', + '**/cjs/**', + '**/__compiled__/**', + 'packages/react-devtools-extensions/src/ErrorTesterCompiled.js', + ], }) .filter(f => !onlyChanged || changedFiles.has(f));