Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat[devtools]: symbolicate source for inspected element #28471

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,13 @@ module.exports = {
__IS_CHROME__: 'readonly',
__IS_FIREFOX__: 'readonly',
__IS_EDGE__: 'readonly',
__IS_INTERNAL_VERSION__: 'readonly',
},
},
{
files: ['packages/react-devtools-shared/**/*.js'],
globals: {
__IS_INTERNAL_VERSION__: 'readonly',
},
},
],
Expand Down
2 changes: 1 addition & 1 deletion packages/react-debug-tools/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,6 @@
"react": "^17.0.0"
},
"dependencies": {
"error-stack-parser": "^2.0.2"
"error-stack-parser": "^2.1.4"
}
}
51 changes: 35 additions & 16 deletions packages/react-devtools-core/src/standalone.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import {
import {localStorageSetItem} from 'react-devtools-shared/src/storage';

import type {FrontendBridge} from 'react-devtools-shared/src/bridge';
import type {InspectedElement} from 'react-devtools-shared/src/frontend/types';
import type {Source} from 'react-devtools-shared/src/shared/types';

installHook(window);

Expand Down Expand Up @@ -127,36 +127,55 @@ function reload() {
store: ((store: any): Store),
warnIfLegacyBackendDetected: true,
viewElementSourceFunction,
fetchFileWithCaching,
}),
);
}, 100);
}

const resourceCache: Map<string, string> = new Map();

// As a potential improvement, this should be done from the backend of RDT.
// Browser extension is doing this via exchanging messages
// between devtools_page and dedicated content script for it, see `fetchFileWithCaching.js`.
async function fetchFileWithCaching(url: string) {
if (resourceCache.has(url)) {
return Promise.resolve(resourceCache.get(url));
}

return fetch(url)
.then(data => data.text())
.then(content => {
resourceCache.set(url, content);

return content;
});
}

function canViewElementSourceFunction(
inspectedElement: InspectedElement,
_source: Source,
symbolicatedSource: Source | null,
): boolean {
if (
inspectedElement.canViewSource === false ||
inspectedElement.source === null
) {
if (symbolicatedSource == null) {
return false;
}

const {source} = inspectedElement;

return doesFilePathExist(source.sourceURL, projectRoots);
return doesFilePathExist(symbolicatedSource.sourceURL, projectRoots);
}

function viewElementSourceFunction(
id: number,
inspectedElement: InspectedElement,
_source: Source,
symbolicatedSource: Source | null,
): void {
const {source} = inspectedElement;
if (source !== null) {
launchEditor(source.sourceURL, source.line, projectRoots);
} else {
log.error('Cannot inspect element', id);
if (symbolicatedSource == null) {
return;
}

launchEditor(
symbolicatedSource.sourceURL,
symbolicatedSource.line,
projectRoots,
);
}

function onDisconnected() {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/* global chrome */

import {normalizeUrl} from 'react-devtools-shared/src/utils';
import {__DEBUG__} from 'react-devtools-shared/src/constants';

let debugIDCounter = 0;
Expand Down Expand Up @@ -107,17 +108,35 @@ const fetchFromPage = async (url, resolve, reject) => {
});
};

// Fetching files from the extension won't make use of the network cache
// for resources that have already been loaded by the page.
// This helper function allows the extension to request files to be fetched
// by the content script (running in the page) to increase the likelihood of a cache hit.
const fetchFileWithCaching = url => {
// 1. Check if resource is available via chrome.devtools.inspectedWindow.getResources
// 2. Check if resource was loaded previously and available in network cache via chrome.devtools.network.getHAR
// 3. Fallback to fetching directly from the page context (from backend)
async function fetchFileWithCaching(url: string): Promise<string> {
if (__IS_CHROME__ || __IS_EDGE__) {
const resources = await new Promise(resolve =>
chrome.devtools.inspectedWindow.getResources(r => resolve(r)),
);

const normalizedReferenceURL = normalizeUrl(url);
const resource = resources.find(r => r.url === normalizedReferenceURL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we subscribe to onResourceAdded and maintain a Map of resources instead of doing an O(n) find for every URL?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't because this event is emitted once resource is added: if main frame resources are loaded and user then opens browser DevTools, the listener won't be called with main frame resources.

If this gets slow for larger apps, I can work on some caching and normalization (to get resource in O(1)), probably


if (resource != null) {
const content = await new Promise(resolve =>
resource.getContent(fetchedContent => resolve(fetchedContent)),
);

if (content) {
return content;
}
}
}

return new Promise((resolve, reject) => {
// Try fetching from the Network cache first.
// If DevTools was opened after the page started loading, we may have missed some requests.
// So fall back to a fetch() from the page and hope we get a cached response that way.
fetchFromNetworkCache(url, resolve, reject);
});
};
}

export default fetchFileWithCaching;
40 changes: 8 additions & 32 deletions packages/react-devtools-extensions/src/main/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -128,34 +128,13 @@ function createBridgeAndStore() {
}
};

const viewElementSourceFunction = id => {
const rendererID = store.getRendererIDForElement(id);
if (rendererID != null) {
// Ask the renderer interface to determine the component function,
// and store it as a global variable on the window
bridge.send('viewElementSource', {id, rendererID});
const viewElementSourceFunction = (source, symbolicatedSource) => {
const {sourceURL, line, column} = symbolicatedSource
? symbolicatedSource
: source;

setTimeout(() => {
// Ask Chrome to display the location of the component function,
// or a render method if it is a Class (ideally Class instance, not type)
// assuming the renderer found one.
chrome.devtools.inspectedWindow.eval(`
if (window.$type != null) {
if (
window.$type &&
window.$type.prototype &&
window.$type.prototype.isReactComponent
) {
// inspect Component.render, not constructor
inspect(window.$type.prototype.render);
} else {
// inspect Functional Component
inspect(window.$type);
}
}
`);
}, 100);
}
// We use 1-based line and column, Chrome expects them 0-based.
chrome.devtools.panels.openResource(sourceURL, line - 1, column - 1);
};

// TODO (Webpack 5) Hopefully we can remove this prop after the Webpack 5 migration.
Expand Down Expand Up @@ -183,17 +162,14 @@ function createBridgeAndStore() {
store,
warnIfUnsupportedVersionDetected: true,
viewAttributeSourceFunction,
// Firefox doesn't support chrome.devtools.panels.openResource yet
canViewElementSourceFunction: () => __IS_CHROME__ || __IS_EDGE__,
viewElementSourceFunction,
viewUrlSourceFunction,
}),
);
};
}

const viewUrlSourceFunction = (url, line, col) => {
chrome.devtools.panels.openResource(url, line, col);
};

function ensureInitialHTMLIsCleared(container) {
if (container._hasInitialHTMLBeenCleared) {
return;
Expand Down
2 changes: 2 additions & 0 deletions packages/react-devtools-extensions/webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ const LOGGING_URL = process.env.LOGGING_URL || null;
const IS_CHROME = process.env.IS_CHROME === 'true';
const IS_FIREFOX = process.env.IS_FIREFOX === 'true';
const IS_EDGE = process.env.IS_EDGE === 'true';
const IS_INTERNAL_VERSION = process.env.FEATURE_FLAG_TARGET === 'extension-fb';

const featureFlagTarget = process.env.FEATURE_FLAG_TARGET || 'extension-oss';

Expand Down Expand Up @@ -119,6 +120,7 @@ module.exports = {
__IS_CHROME__: IS_CHROME,
__IS_FIREFOX__: IS_FIREFOX,
__IS_EDGE__: IS_EDGE,
__IS_INTERNAL_VERSION__: IS_INTERNAL_VERSION,
'process.env.DEVTOOLS_PACKAGE': `"react-devtools-extensions"`,
'process.env.DEVTOOLS_VERSION': `"${DEVTOOLS_VERSION}"`,
'process.env.EDITOR_URL': EDITOR_URL != null ? `"${EDITOR_URL}"` : null,
Expand Down
1 change: 1 addition & 0 deletions packages/react-devtools-shared/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
"@reach/tooltip": "^0.16.0",
"clipboard-js": "^0.3.6",
"compare-versions": "^5.0.3",
"jsc-safe-url": "^0.2.4",
"json5": "^2.1.3",
"local-storage-fallback": "^4.1.1",
"lodash.throttle": "^4.1.1",
Expand Down
4 changes: 3 additions & 1 deletion packages/react-devtools-shared/src/backendAPI.js
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,9 @@ export function convertInspectedElementBackendToFrontend(
rendererPackageName,
rendererVersion,
rootType,
source,
// Previous backend implementations (<= 5.0.1) have a different interface for Source, with fileName.
// This gates the source features for only compatible backends: >= 5.0.2
source: source && source.sourceURL ? source : null,
type,
owners:
owners === null
Expand Down
Loading