From 9137d4c61a503b1ed5ee9547b5d62b57293b6e24 Mon Sep 17 00:00:00 2001 From: Jack Works <5390719+Jack-Works@users.noreply.github.com> Date: Tue, 21 May 2024 23:42:11 +0800 Subject: [PATCH 1/6] fix: path handling in react devtools --- .../src/symbolicateSource.js | 16 ++++------------ packages/react-devtools-shared/src/utils.js | 3 +-- 2 files changed, 5 insertions(+), 14 deletions(-) diff --git a/packages/react-devtools-shared/src/symbolicateSource.js b/packages/react-devtools-shared/src/symbolicateSource.js index d28ed42e59db0..71feb29236991 100644 --- a/packages/react-devtools-shared/src/symbolicateSource.js +++ b/packages/react-devtools-shared/src/symbolicateSource.js @@ -68,7 +68,7 @@ async function symbolicateSource( resourceLine.length, ); - const sourceMap = await fetchFileWithCaching(sourceMapURL).catch( + const sourceMap = await fetchFileWithCaching(new URL(sourceMapURL, sourceURL).toString()).catch( () => null, ); if (sourceMap != null) { @@ -91,22 +91,14 @@ async function symbolicateSource( return {sourceURL: normalizedURL, line, column}; } catch (e) { // This is not valid URL - if (possiblyURL.startsWith('/')) { + // /path or X:\\path + if (possiblyURL.startsWith('/') || possiblyURL.slice(1).startsWith(':\\\\')) { // This is an absolute path return {sourceURL: possiblyURL, line, column}; } // This is a relative path - const [sourceMapAbsolutePathWithoutQueryParameters] = - sourceMapURL.split(/[?#&]/); - - const absoluteSourcePath = - sourceMapAbsolutePathWithoutQueryParameters + - (sourceMapAbsolutePathWithoutQueryParameters.endsWith('/') - ? '' - : '/') + - possiblyURL; - + const absoluteSourcePath = new URL(possiblyURL, sourceMapURL).toString(); return {sourceURL: absoluteSourcePath, line, column}; } } catch (e) { diff --git a/packages/react-devtools-shared/src/utils.js b/packages/react-devtools-shared/src/utils.js index 5b0903883c301..1bb054cbe589b 100644 --- a/packages/react-devtools-shared/src/utils.js +++ b/packages/react-devtools-shared/src/utils.js @@ -1017,7 +1017,6 @@ export function backendToFrontendSerializedElementMapper( }; } -// This is a hacky one to just support this exact case. export function normalizeUrl(url: string): string { - return url.replace('/./', '/'); + return new URL(url).toString(); } From 7e38c389fdca1c548342a37fb4e5a1b8c8505156 Mon Sep 17 00:00:00 2001 From: Jack Works <5390719+Jack-Works@users.noreply.github.com> Date: Tue, 21 May 2024 23:47:15 +0800 Subject: [PATCH 2/6] fix: prettier --- .../src/symbolicateSource.js | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/packages/react-devtools-shared/src/symbolicateSource.js b/packages/react-devtools-shared/src/symbolicateSource.js index 71feb29236991..19021b1628fea 100644 --- a/packages/react-devtools-shared/src/symbolicateSource.js +++ b/packages/react-devtools-shared/src/symbolicateSource.js @@ -68,9 +68,9 @@ async function symbolicateSource( resourceLine.length, ); - const sourceMap = await fetchFileWithCaching(new URL(sourceMapURL, sourceURL).toString()).catch( - () => null, - ); + const sourceMap = await fetchFileWithCaching( + new URL(sourceMapURL, sourceURL).toString(), + ).catch(() => null); if (sourceMap != null) { try { const parsedSourceMap = JSON.parse(sourceMap); @@ -92,13 +92,19 @@ async function symbolicateSource( } catch (e) { // This is not valid URL // /path or X:\\path - if (possiblyURL.startsWith('/') || possiblyURL.slice(1).startsWith(':\\\\')) { + if ( + possiblyURL.startsWith('/') || + possiblyURL.slice(1).startsWith(':\\\\') + ) { // This is an absolute path return {sourceURL: possiblyURL, line, column}; } // This is a relative path - const absoluteSourcePath = new URL(possiblyURL, sourceMapURL).toString(); + const absoluteSourcePath = new URL( + possiblyURL, + sourceMapURL, + ).toString(); return {sourceURL: absoluteSourcePath, line, column}; } } catch (e) { From b211182e54eea53069e49e3295a611d8b32f88ee Mon Sep 17 00:00:00 2001 From: Jack Works <5390719+Jack-Works@users.noreply.github.com> Date: Thu, 23 May 2024 00:24:41 +0800 Subject: [PATCH 3/6] resolve review --- packages/react-devtools-shared/src/utils.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/react-devtools-shared/src/utils.js b/packages/react-devtools-shared/src/utils.js index 1bb054cbe589b..ffbc9e390d5a4 100644 --- a/packages/react-devtools-shared/src/utils.js +++ b/packages/react-devtools-shared/src/utils.js @@ -1017,6 +1017,7 @@ export function backendToFrontendSerializedElementMapper( }; } +// Chrome normalizes urls like webpack-internals:// but new URL don't, so cannot use new URL here. export function normalizeUrl(url: string): string { - return new URL(url).toString(); + return url.replace('/./', '/'); } From 6827c5dc076a173753148f5c9b0995a73af43cda Mon Sep 17 00:00:00 2001 From: Jack Works <5390719+Jack-Works@users.noreply.github.com> Date: Mon, 1 Jul 2024 23:43:01 +0800 Subject: [PATCH 4/6] test: add test --- .../src/__tests__/utils-test.js | 30 +++++++++++++++++++ .../src/hooks/SourceMapConsumer.js | 15 +++------- .../parseHookNames/parseSourceAndMetadata.js | 5 ++++ .../src/symbolicateSource.js | 21 ++++++++----- 4 files changed, 53 insertions(+), 18 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/utils-test.js b/packages/react-devtools-shared/src/__tests__/utils-test.js index 71cd2aaf38dc0..e1967cd21efb2 100644 --- a/packages/react-devtools-shared/src/__tests__/utils-test.js +++ b/packages/react-devtools-shared/src/__tests__/utils-test.js @@ -25,6 +25,7 @@ import { REACT_STRICT_MODE_TYPE as StrictMode, } from 'shared/ReactSymbols'; import {createElement} from 'react'; +import {symbolicateSource} from '../symbolicateSource'; describe('utils', () => { describe('getDisplayName', () => { @@ -387,4 +388,33 @@ describe('utils', () => { }); }); }); + + describe('symbolicateSource', () => { + const source = `"use strict"; +Object.defineProperty(exports, "__esModule", { value: true }); +exports.f = f; +function f() { } +//# sourceMappingURL=`; + const result = { + column: 16, + line: 1, + sourceURL: 'http://test/a.mts', + }; + const fs = { + 'http://test/a.mts': `export function f() {}`, + 'http://test/a.mjs.map': `{"version":3,"file":"a.mjs","sourceRoot":"","sources":["a.mts"],"names":[],"mappings":";;AAAA,cAAsB;AAAtB,SAAgB,CAAC,KAAI,CAAC"}`, + 'http://test/a.mjs': `${source}a.mjs.map`, + 'http://test/b.mjs': `${source}./a.mjs.map`, + 'http://test/c.mjs': `${source}http://test/a.mjs.map`, + 'http://test/d.mjs': `${source}/a.mjs.map`, + }; + const fetchFileWithCaching = async (url: string) => fs[url] || null; + it('should parse source map urls', async () => { + const run = url => symbolicateSource(fetchFileWithCaching, url, 4, 10); + await expect(run('http://test/a.mjs')).resolves.toStrictEqual(result); + await expect(run('http://test/b.mjs')).resolves.toStrictEqual(result); + await expect(run('http://test/c.mjs')).resolves.toStrictEqual(result); + await expect(run('http://test/d.mjs')).resolves.toStrictEqual(result); + }); + }); }); diff --git a/packages/react-devtools-shared/src/hooks/SourceMapConsumer.js b/packages/react-devtools-shared/src/hooks/SourceMapConsumer.js index 779cbb3b2f0d1..8e6503685bdb5 100644 --- a/packages/react-devtools-shared/src/hooks/SourceMapConsumer.js +++ b/packages/react-devtools-shared/src/hooks/SourceMapConsumer.js @@ -24,8 +24,8 @@ type SearchPosition = { type ResultPosition = { column: number, line: number, - sourceContent: string, - sourceURL: string, + sourceContent: string | null, + sourceURL: string | null, }; export type SourceMapConsumerType = { @@ -118,18 +118,11 @@ function BasicSourceMapConsumer(sourceMapJSON: BasicSourceMap) { const line = nearestEntry[2] + 1; const column = nearestEntry[3]; - if (sourceContent === null || sourceURL === null) { - // TODO maybe fall back to the runtime source instead of throwing? - throw Error( - `Could not find original source for line:${lineNumber} and column:${columnNumber}`, - ); - } - return { column, line, - sourceContent: ((sourceContent: any): string), - sourceURL: ((sourceURL: any): string), + sourceContent: ((sourceContent: any): string | null), + sourceURL: ((sourceURL: any): string | null), }; } diff --git a/packages/react-devtools-shared/src/hooks/parseHookNames/parseSourceAndMetadata.js b/packages/react-devtools-shared/src/hooks/parseHookNames/parseSourceAndMetadata.js index 40bfed48ba53f..15423c12416cc 100644 --- a/packages/react-devtools-shared/src/hooks/parseHookNames/parseSourceAndMetadata.js +++ b/packages/react-devtools-shared/src/hooks/parseHookNames/parseSourceAndMetadata.js @@ -276,6 +276,11 @@ function parseSourceAST( columnNumber, lineNumber, }); + if (sourceContent === null || sourceURL === null) { + throw Error( + `Could not find original source for line:${lineNumber} and column:${columnNumber}`, + ); + } originalSourceColumnNumber = column; originalSourceLineNumber = line; diff --git a/packages/react-devtools-shared/src/symbolicateSource.js b/packages/react-devtools-shared/src/symbolicateSource.js index 19021b1628fea..b5efb29d79c4d 100644 --- a/packages/react-devtools-shared/src/symbolicateSource.js +++ b/packages/react-devtools-shared/src/symbolicateSource.js @@ -39,7 +39,7 @@ export async function symbolicateSourceWithCache( } const SOURCE_MAP_ANNOTATION_PREFIX = 'sourceMappingURL='; -async function symbolicateSource( +export async function symbolicateSource( fetchFileWithCaching: FetchFileWithCaching, sourceURL: string, lineNumber: number, // 1-based @@ -63,14 +63,15 @@ async function symbolicateSource( const sourceMapAnnotationStartIndex = resourceLine.indexOf( SOURCE_MAP_ANNOTATION_PREFIX, ); - const sourceMapURL = resourceLine.slice( + const sourceMapAt = resourceLine.slice( sourceMapAnnotationStartIndex + SOURCE_MAP_ANNOTATION_PREFIX.length, resourceLine.length, ); - const sourceMap = await fetchFileWithCaching( - new URL(sourceMapURL, sourceURL).toString(), - ).catch(() => null); + const sourceMapURL = new URL(sourceMapAt, sourceURL).toString(); + const sourceMap = await fetchFileWithCaching(sourceMapURL).catch( + () => null, + ); if (sourceMap != null) { try { const parsedSourceMap = JSON.parse(sourceMap); @@ -84,16 +85,21 @@ async function symbolicateSource( columnNumber, // 1-based }); + if (possiblyURL === null) { + return null; + } try { - void new URL(possiblyURL); // This is a valid URL + // sourceMapURL=https://... + void new URL(possiblyURL); // test if it is a valid URL const normalizedURL = normalizeUrl(possiblyURL); return {sourceURL: normalizedURL, line, column}; } catch (e) { // This is not valid URL - // /path or X:\\path if ( + // sourceMapURL=/file possiblyURL.startsWith('/') || + // sourceMapURL=C:\\... possiblyURL.slice(1).startsWith(':\\\\') ) { // This is an absolute path @@ -101,6 +107,7 @@ async function symbolicateSource( } // This is a relative path + // sourceMapURL=x.js.map const absoluteSourcePath = new URL( possiblyURL, sourceMapURL, From 954530f443e42586d34f6517e94a9f2b4d2099b2 Mon Sep 17 00:00:00 2001 From: Jack Works <5390719+Jack-Works@users.noreply.github.com> Date: Mon, 1 Jul 2024 23:48:33 +0800 Subject: [PATCH 5/6] fix: prettier --- packages/react-devtools-shared/src/__tests__/utils-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-devtools-shared/src/__tests__/utils-test.js b/packages/react-devtools-shared/src/__tests__/utils-test.js index 71a22574e5989..f35cacc73308f 100644 --- a/packages/react-devtools-shared/src/__tests__/utils-test.js +++ b/packages/react-devtools-shared/src/__tests__/utils-test.js @@ -414,7 +414,7 @@ function f() { } await expect(run('http://test/d.mjs')).resolves.toStrictEqual(result); }); }); - + describe('formatConsoleArguments', () => { it('works with empty arguments list', () => { expect(formatConsoleArguments(...[])).toEqual([]); From 1345786f510e1f315ed696f4138a96c34230475f Mon Sep 17 00:00:00 2001 From: Jack Works <5390719+Jack-Works@users.noreply.github.com> Date: Wed, 3 Jul 2024 15:30:45 +0800 Subject: [PATCH 6/6] chore: resolve review --- packages/react-devtools-shared/src/symbolicateSource.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/react-devtools-shared/src/symbolicateSource.js b/packages/react-devtools-shared/src/symbolicateSource.js index b5efb29d79c4d..9430e88b3fc15 100644 --- a/packages/react-devtools-shared/src/symbolicateSource.js +++ b/packages/react-devtools-shared/src/symbolicateSource.js @@ -89,7 +89,7 @@ export async function symbolicateSource( return null; } try { - // sourceMapURL=https://... + // sourceMapURL = https://react.dev/script.js.map void new URL(possiblyURL); // test if it is a valid URL const normalizedURL = normalizeUrl(possiblyURL); @@ -97,9 +97,9 @@ export async function symbolicateSource( } catch (e) { // This is not valid URL if ( - // sourceMapURL=/file + // sourceMapURL = /file possiblyURL.startsWith('/') || - // sourceMapURL=C:\\... + // sourceMapURL = C:\\... possiblyURL.slice(1).startsWith(':\\\\') ) { // This is an absolute path @@ -107,7 +107,7 @@ export async function symbolicateSource( } // This is a relative path - // sourceMapURL=x.js.map + // possiblyURL = x.js.map, sourceMapURL = https://react.dev/script.js.map const absoluteSourcePath = new URL( possiblyURL, sourceMapURL,