diff --git a/packages/react-devtools-core/src/standalone.js b/packages/react-devtools-core/src/standalone.js index 00cc19f6d8342..9c55fe7d2c669 100644 --- a/packages/react-devtools-core/src/standalone.js +++ b/packages/react-devtools-core/src/standalone.js @@ -144,7 +144,7 @@ function canViewElementSourceFunction( const {source} = inspectedElement; - return doesFilePathExist(source.fileName, projectRoots); + return doesFilePathExist(source.sourceURL, projectRoots); } function viewElementSourceFunction( @@ -153,7 +153,7 @@ function viewElementSourceFunction( ): void { const {source} = inspectedElement; if (source !== null) { - launchEditor(source.fileName, source.lineNumber, projectRoots); + launchEditor(source.sourceURL, source.line, projectRoots); } else { log.error('Cannot inspect element', id); } diff --git a/packages/react-devtools-inline/__tests__/__e2e__/components.test.js b/packages/react-devtools-inline/__tests__/__e2e__/components.test.js index 39467d3a9c932..03de73decb8c5 100644 --- a/packages/react-devtools-inline/__tests__/__e2e__/components.test.js +++ b/packages/react-devtools-inline/__tests__/__e2e__/components.test.js @@ -92,15 +92,14 @@ test.describe('Components', () => { ? valueElement.value : valueElement.innerText; - return [name, value, source ? source.innerText : null]; + return [name, value, source.innerText]; }, {name: isEditableName, value: isEditableValue} ); expect(propName).toBe('label'); expect(propValue).toBe('"one"'); - expect(sourceText).toBe(null); - // TODO: expect(sourceText).toMatch(/ListApp[a-zA-Z]*\.js/); + expect(sourceText).toMatch(/e2e-app[a-zA-Z]*\.js/); }); test('should allow props to be edited', async () => { diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js index a863c6144abcc..030731a46533f 100644 --- a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js +++ b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js @@ -424,7 +424,9 @@ describe('InspectedElement', () => { targetRenderCount = 0; let inspectedElement = await inspectElementAtIndex(1); - expect(targetRenderCount).toBe(1); + // One more because we call render function for generating component stack, + // which is required for defining source location + expect(targetRenderCount).toBe(2); expect(inspectedElement.props).toMatchInlineSnapshot(` { "a": 1, @@ -485,7 +487,9 @@ describe('InspectedElement', () => { targetRenderCount = 0; let inspectedElement = await inspectElementAtIndex(1); - expect(targetRenderCount).toBe(1); + // One more because we call render function for generating component stack, + // which is required for defining source location + expect(targetRenderCount).toBe(2); expect(inspectedElement.props).toMatchInlineSnapshot(` { "a": 1, @@ -555,7 +559,9 @@ describe('InspectedElement', () => { const inspectedElement = await inspectElementAtIndex(0); expect(inspectedElement).not.toBe(null); - expect(targetRenderCount).toBe(2); + // One more because we call render function for generating component stack, + // which is required for defining source location + expect(targetRenderCount).toBe(3); expect(console.error).toHaveBeenCalledTimes(1); expect(console.info).toHaveBeenCalledTimes(1); expect(console.log).toHaveBeenCalledTimes(1); diff --git a/packages/react-devtools-shared/src/__tests__/storeComponentFilters-test.js b/packages/react-devtools-shared/src/__tests__/storeComponentFilters-test.js index 571302961dc51..895c9d521a361 100644 --- a/packages/react-devtools-shared/src/__tests__/storeComponentFilters-test.js +++ b/packages/react-devtools-shared/src/__tests__/storeComponentFilters-test.js @@ -224,11 +224,16 @@ describe('Store component filters', () => { `); }); + // Disabled: filtering by path was removed, source is now determined lazily, including symbolication if applicable // @reactVersion >= 16.0 - it('should filter by path', async () => { - const Component = () =>
Hi
; + xit('should filter by path', async () => { + // This component should use props object in order to throw for component stack generation + // See ReactComponentStackFrame:155 or DevToolsComponentStackFrame:147 + const Component = props => { + return
{props.message}
; + }; - await actAsync(async () => render()); + await actAsync(async () => render()); expect(store).toMatchInlineSnapshot(` [root] ▾ @@ -242,13 +247,7 @@ describe('Store component filters', () => { ]), ); - // TODO: Filtering should work on component location. - // expect(store).toMatchInlineSnapshot(`[root]`); - expect(store).toMatchInlineSnapshot(` - [root] - ▾ -
- `); + expect(store).toMatchInlineSnapshot(`[root]`); await actAsync( async () => @@ -497,19 +496,17 @@ describe('Store component filters', () => { ]), ); - utils.act( - () => - utils.withErrorsOrWarningsIgnored(['test-only:'], () => { - render( - - - - - , - ); - }), - false, - ); + utils.withErrorsOrWarningsIgnored(['test-only:'], () => { + utils.act(() => { + render( + + + + + , + ); + }, false); + }); expect(store).toMatchInlineSnapshot(``); expect(store.errorCount).toBe(0); diff --git a/packages/react-devtools-shared/src/backend/DevToolsComponentStackFrame.js b/packages/react-devtools-shared/src/backend/DevToolsComponentStackFrame.js index 4e0e334ce86a6..0e412a43f74c3 100644 --- a/packages/react-devtools-shared/src/backend/DevToolsComponentStackFrame.js +++ b/packages/react-devtools-shared/src/backend/DevToolsComponentStackFrame.js @@ -81,8 +81,6 @@ export function describeNativeComponentFrame( } } - let control; - const previousPrepareStackTrace = Error.prepareStackTrace; // $FlowFixMe[incompatible-type] It does accept undefined. Error.prepareStackTrace = undefined; @@ -98,64 +96,140 @@ export function describeNativeComponentFrame( currentDispatcherRef.current = null; disableLogs(); - try { - // This should throw. - if (construct) { - // Something should be setting the props in the constructor. - const Fake = function () { - throw Error(); - }; - // $FlowFixMe[prop-missing] - Object.defineProperty(Fake.prototype, 'props', { - set: function () { - // We use a throwing setter instead of frozen or non-writable props - // because that won't throw in a non-strict mode function. - throw Error(); - }, - }); - if (typeof Reflect === 'object' && Reflect.construct) { - // We construct a different control for this case to include any extra - // frames added by the construct call. - try { - Reflect.construct(Fake, []); - } catch (x) { - control = x; + // NOTE: keep in sync with the implementation in ReactComponentStackFrame + + /** + * Finding a common stack frame between sample and control errors can be + * tricky given the different types and levels of stack trace truncation from + * different JS VMs. So instead we'll attempt to control what that common + * frame should be through this object method: + * Having both the sample and control errors be in the function under the + * `DescribeNativeComponentFrameRoot` property, + setting the `name` and + * `displayName` properties of the function ensures that a stack + * frame exists that has the method name `DescribeNativeComponentFrameRoot` in + * it for both control and sample stacks. + */ + const RunInRootFrame = { + DetermineComponentFrameRoot(): [?string, ?string] { + let control; + try { + // This should throw. + if (construct) { + // Something should be setting the props in the constructor. + const Fake = function () { + throw Error(); + }; + // $FlowFixMe[prop-missing] + Object.defineProperty(Fake.prototype, 'props', { + set: function () { + // We use a throwing setter instead of frozen or non-writable props + // because that won't throw in a non-strict mode function. + throw Error(); + }, + }); + if (typeof Reflect === 'object' && Reflect.construct) { + // We construct a different control for this case to include any extra + // frames added by the construct call. + try { + Reflect.construct(Fake, []); + } catch (x) { + control = x; + } + Reflect.construct(fn, [], Fake); + } else { + try { + Fake.call(); + } catch (x) { + control = x; + } + // $FlowFixMe[prop-missing] found when upgrading Flow + fn.call(Fake.prototype); + } + } else { + try { + throw Error(); + } catch (x) { + control = x; + } + // TODO(luna): This will currently only throw if the function component + // tries to access React/ReactDOM/props. We should probably make this throw + // in simple components too + const maybePromise = fn(); + + // If the function component returns a promise, it's likely an async + // component, which we don't yet support. Attach a noop catch handler to + // silence the error. + // TODO: Implement component stacks for async client components? + if (maybePromise && typeof maybePromise.catch === 'function') { + maybePromise.catch(() => {}); + } } - Reflect.construct(fn, [], Fake); - } else { - try { - Fake.call(); - } catch (x) { - control = x; + } catch (sample) { + // This is inlined manually because closure doesn't do it for us. + if (sample && control && typeof sample.stack === 'string') { + return [sample.stack, control.stack]; } - // $FlowFixMe[prop-missing] found when upgrading Flow - fn.call(Fake.prototype); - } - } else { - try { - throw Error(); - } catch (x) { - control = x; } - fn(); - } - } catch (sample) { - // This is inlined manually because closure doesn't do it for us. - if (sample && control && typeof sample.stack === 'string') { + return [null, null]; + }, + }; + // $FlowFixMe[prop-missing] + RunInRootFrame.DetermineComponentFrameRoot.displayName = + 'DetermineComponentFrameRoot'; + const namePropDescriptor = Object.getOwnPropertyDescriptor( + RunInRootFrame.DetermineComponentFrameRoot, + 'name', + ); + // Before ES6, the `name` property was not configurable. + if (namePropDescriptor && namePropDescriptor.configurable) { + // V8 utilizes a function's `name` property when generating a stack trace. + Object.defineProperty( + RunInRootFrame.DetermineComponentFrameRoot, + // Configurable properties can be updated even if its writable descriptor + // is set to `false`. + // $FlowFixMe[cannot-write] + 'name', + {value: 'DetermineComponentFrameRoot'}, + ); + } + + try { + const [sampleStack, controlStack] = + RunInRootFrame.DetermineComponentFrameRoot(); + if (sampleStack && controlStack) { // This extracts the first frame from the sample that isn't also in the control. // Skipping one frame that we assume is the frame that calls the two. - const sampleLines = sample.stack.split('\n'); - const controlLines = control.stack.split('\n'); - let s = sampleLines.length - 1; - let c = controlLines.length - 1; - while (s >= 1 && c >= 0 && sampleLines[s] !== controlLines[c]) { - // We expect at least one stack frame to be shared. - // Typically this will be the root most one. However, stack frames may be - // cut off due to maximum stack limits. In this case, one maybe cut off - // earlier than the other. We assume that the sample is longer or the same - // and there for cut off earlier. So we should find the root most frame in - // the sample somewhere in the control. - c--; + const sampleLines = sampleStack.split('\n'); + const controlLines = controlStack.split('\n'); + let s = 0; + let c = 0; + while ( + s < sampleLines.length && + !sampleLines[s].includes('DetermineComponentFrameRoot') + ) { + s++; + } + while ( + c < controlLines.length && + !controlLines[c].includes('DetermineComponentFrameRoot') + ) { + c++; + } + // We couldn't find our intentionally injected common root frame, attempt + // to find another common root frame by search from the bottom of the + // control stack... + if (s === sampleLines.length || c === controlLines.length) { + s = sampleLines.length - 1; + c = controlLines.length - 1; + while (s >= 1 && c >= 0 && sampleLines[s] !== controlLines[c]) { + // We expect at least one stack frame to be shared. + // Typically this will be the root most one. However, stack frames may be + // cut off due to maximum stack limits. In this case, one maybe cut off + // earlier than the other. We assume that the sample is longer or the same + // and there for cut off earlier. So we should find the root most frame in + // the sample somewhere in the control. + c--; + } } for (; s >= 1 && c >= 0; s--, c--) { // Next we find the first one that isn't the same which should be the @@ -174,7 +248,15 @@ export function describeNativeComponentFrame( // The next one that isn't the same should be our match though. if (c < 0 || sampleLines[s] !== controlLines[c]) { // V8 adds a "new" prefix for native classes. Let's remove it to make it prettier. - const frame = '\n' + sampleLines[s].replace(' at new ', ' at '); + let frame = '\n' + sampleLines[s].replace(' at new ', ' at '); + + // If our component frame is labeled "" + // but we have a user-provided "displayName" + // splice it in to make the stack more readable. + if (fn.displayName && frame.includes('')) { + frame = frame.replace('', fn.displayName); + } + if (__DEV__) { if (typeof fn === 'function') { componentFrameCache.set(fn, frame); diff --git a/packages/react-devtools-shared/src/backend/legacy/renderer.js b/packages/react-devtools-shared/src/backend/legacy/renderer.js index 8f8af5594f8d4..e037d45075a80 100644 --- a/packages/react-devtools-shared/src/backend/legacy/renderer.js +++ b/packages/react-devtools-shared/src/backend/legacy/renderer.js @@ -828,6 +828,7 @@ export function attach( // Can view component source location. canViewSource: type === ElementTypeClass || type === ElementTypeFunction, + source: null, // Only legacy context exists in legacy versions. hasLegacyContext: true, diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index 71693593569bb..b1e5dac527e61 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -42,6 +42,7 @@ import {sessionStorageGetItem} from 'react-devtools-shared/src/storage'; import { gt, gte, + parseSourceFromComponentStack, serializeToString, } from 'react-devtools-shared/src/backend/utils'; import { @@ -124,6 +125,8 @@ import type { ElementType, Plugins, } from 'react-devtools-shared/src/frontend/types'; +import type {Source} from 'react-devtools-shared/src/shared/types'; +import {getStackByFiberInDevAndProd} from './DevToolsFiberComponentStack'; type getDisplayNameForFiberType = (fiber: Fiber) => string | null; type getTypeSymbolType = (type: any) => symbol | number; @@ -585,6 +588,8 @@ const fiberToIDMap: Map = new Map(); // operations that should be the same whether the current and work-in-progress Fiber is used. const idToArbitraryFiberMap: Map = new Map(); +const fiberToComponentStackMap: WeakMap = new WeakMap(); + export function attach( hook: DevToolsHook, rendererID: number, @@ -1029,15 +1034,21 @@ export function attach( } } - // TODO: Figure out a way to filter by path in the new model which has no debug info. - // if (hideElementsWithPaths.size > 0) { - // const {fileName} = ...; - // for (const pathRegExp of hideElementsWithPaths) { - // if (pathRegExp.test(fileName)) { - // return true; - // } - // } - // } + /* DISABLED: https://github.com/facebook/react/pull/28417 + if (hideElementsWithPaths.size > 0) { + const source = getSourceForFiber(fiber); + + if (source != null) { + const {fileName} = source; + // eslint-disable-next-line no-for-of-loops/no-for-of-loops + for (const pathRegExp of hideElementsWithPaths) { + if (pathRegExp.test(fileName)) { + return true; + } + } + } + } + */ return false; } @@ -1246,10 +1257,12 @@ export function attach( } fiberToIDMap.delete(fiber); + fiberToComponentStackMap.delete(fiber); const {alternate} = fiber; if (alternate !== null) { fiberToIDMap.delete(alternate); + fiberToComponentStackMap.delete(alternate); } if (forceErrorForFiberIDs.has(fiberID)) { @@ -3361,6 +3374,11 @@ export function attach( } } + let source = null; + if (canViewSource) { + source = getSourceForFiber(fiber); + } + return { id, @@ -3393,6 +3411,7 @@ export function attach( // Can view component source location. canViewSource, + source, // Does the component have legacy context attached to it. hasLegacyContext, @@ -4520,6 +4539,34 @@ export function attach( return idToArbitraryFiberMap.has(id); } + function getComponentStackForFiber(fiber: Fiber): string | null { + let componentStack = fiberToComponentStackMap.get(fiber); + if (componentStack == null) { + const dispatcherRef = renderer.currentDispatcherRef; + if (dispatcherRef == null) { + return null; + } + + componentStack = getStackByFiberInDevAndProd( + ReactTypeOfWork, + fiber, + dispatcherRef, + ); + fiberToComponentStackMap.set(fiber, componentStack); + } + + return componentStack; + } + + function getSourceForFiber(fiber: Fiber): Source | null { + const componentStack = getComponentStackForFiber(fiber); + if (componentStack == null) { + return null; + } + + return parseSourceFromComponentStack(componentStack); + } + return { cleanup, clearErrorsAndWarnings, @@ -4530,6 +4577,8 @@ export function attach( findNativeNodesForFiberID, flushInitialOperations, getBestMatchForTrackedPath, + getComponentStackForFiber, + getSourceForFiber, getDisplayNameForFiberID, getFiberForNative, getFiberIDForNative, diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index 6b0cd5ee6c2b0..df45122f6314f 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -29,6 +29,7 @@ import type {InitBackend} from 'react-devtools-shared/src/backend'; import type {TimelineDataExport} from 'react-devtools-timeline/src/types'; import type {BrowserTheme} from 'react-devtools-shared/src/frontend/types'; import type {BackendBridge} from 'react-devtools-shared/src/bridge'; +import type {Source} from 'react-devtools-shared/src/shared/types'; import type Agent from './agent'; type BundleType = @@ -278,6 +279,7 @@ export type InspectedElement = { // List of owners owners: Array | null, + source: Source | null, type: ElementType, diff --git a/packages/react-devtools-shared/src/backend/utils.js b/packages/react-devtools-shared/src/backend/utils.js index a1975e43c6323..5655fa0bed0f2 100644 --- a/packages/react-devtools-shared/src/backend/utils.js +++ b/packages/react-devtools-shared/src/backend/utils.js @@ -12,6 +12,7 @@ import {compareVersions} from 'compare-versions'; import {dehydrate} from '../hydration'; import isArray from 'shared/isArray'; +import type {Source} from 'react-devtools-shared/src/shared/types'; import type {DehydratedData} from 'react-devtools-shared/src/frontend/types'; // TODO: update this to the first React version that has a corresponding DevTools backend @@ -289,3 +290,34 @@ export const isReactNativeEnvironment = (): boolean => { // We should probably define the client for DevTools on the backend side and share it with the frontend return window.document == null; }; + +export function parseSourceFromComponentStack( + componentStack: string, +): Source | null { + const frames = componentStack.split('\n'); + // eslint-disable-next-line no-for-of-loops/no-for-of-loops + for (const frame of frames) { + const openingBracketIndex = frame.lastIndexOf('('); + if (openingBracketIndex === -1) continue; + const closingBracketIndex = frame.lastIndexOf(')'); + if ( + closingBracketIndex === -1 || + openingBracketIndex >= closingBracketIndex + ) + continue; + + const url = frame.slice(openingBracketIndex + 1, closingBracketIndex); + const match = url.match(/^(.+):(\d+):(\d+)$/); + if (match == null) continue; + + const [, sourceURL, line, column] = match; + + return { + sourceURL, + line: parseInt(line, 10), + column: parseInt(column, 10), + }; + } + + return null; +} diff --git a/packages/react-devtools-shared/src/backendAPI.js b/packages/react-devtools-shared/src/backendAPI.js index 21ae444a1ef8c..6fcc35b574277 100644 --- a/packages/react-devtools-shared/src/backendAPI.js +++ b/packages/react-devtools-shared/src/backendAPI.js @@ -228,6 +228,7 @@ export function convertInspectedElementBackendToFrontend( id, type, owners, + source, context, hooks, plugins, @@ -260,7 +261,7 @@ export function convertInspectedElementBackendToFrontend( rendererPackageName, rendererVersion, rootType, - source: null, // TODO: Load source location lazily. + source, type, owners: owners === null diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js index 635af271ade33..3c7e1834c30b7 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js @@ -220,10 +220,10 @@ export default function InspectedElementWrapper(_: Props): React.Node { const url = new URL(editorURL); url.href = url.href - .replace('{path}', source.fileName) - .replace('{line}', String(source.lineNumber)) - .replace('%7Bpath%7D', source.fileName) - .replace('%7Bline%7D', String(source.lineNumber)); + .replace('{path}', source.sourceURL) + .replace('{line}', String(source.line)) + .replace('%7Bpath%7D', source.sourceURL) + .replace('%7Bline%7D', String(source.line)); window.open(url); }, [inspectedElement, editorURL]); diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementView.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementView.js index 2f1503c65e487..ff7a046f878b2 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementView.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementView.js @@ -172,7 +172,7 @@ export default function InspectedElementView({ )} {source !== null && ( - + )}
@@ -239,15 +239,17 @@ export default function InspectedElementView({ } // This function is based on describeComponentFrame() in packages/shared/ReactComponentStackFrame -function formatSourceForDisplay(fileName: string, lineNumber: string) { +function formatSourceForDisplay(sourceURL: string, line: number) { + // Note: this RegExp doesn't work well with URLs from Metro, + // which provides bundle URL with query parameters prefixed with /& const BEFORE_SLASH_RE = /^(.*)[\\\/]/; - let nameOnly = fileName.replace(BEFORE_SLASH_RE, ''); + let nameOnly = sourceURL.replace(BEFORE_SLASH_RE, ''); // In DEV, include code for a common special case: // prefer "folder/index.js" instead of just "index.js". if (/^index\./.test(nameOnly)) { - const match = fileName.match(BEFORE_SLASH_RE); + const match = sourceURL.match(BEFORE_SLASH_RE); if (match) { const pathBeforeSlash = match[1]; if (pathBeforeSlash) { @@ -257,16 +259,16 @@ function formatSourceForDisplay(fileName: string, lineNumber: string) { } } - return `${nameOnly}:${lineNumber}`; + return `${nameOnly}:${sourceURL}`; } type SourceProps = { - fileName: string, - lineNumber: string, + sourceURL: string, + line: number, }; -function Source({fileName, lineNumber}: SourceProps) { - const handleCopy = () => copy(`${fileName}:${lineNumber}`); +function Source({sourceURL, line}: SourceProps) { + const handleCopy = () => copy(`${sourceURL}:${line}`); return (
@@ -276,7 +278,7 @@ function Source({fileName, lineNumber}: SourceProps) {
- {formatSourceForDisplay(fileName, lineNumber)} + {formatSourceForDisplay(sourceURL, line)}
); diff --git a/packages/react-devtools-shared/src/frontend/types.js b/packages/react-devtools-shared/src/frontend/types.js index 9623efd3dd230..10c2e62c20cbc 100644 --- a/packages/react-devtools-shared/src/frontend/types.js +++ b/packages/react-devtools-shared/src/frontend/types.js @@ -18,6 +18,7 @@ import type { Dehydrated, Unserializable, } from 'react-devtools-shared/src/hydration'; +import type {Source} from 'react-devtools-shared/src/shared/types'; export type BrowserTheme = 'dark' | 'light'; @@ -219,7 +220,7 @@ export type InspectedElement = { owners: Array | null, // Location of component in source code. - source: null, // TODO: Reinstate a way to load this lazily. + source: Source | null, type: ElementType, diff --git a/packages/react-devtools-shared/src/shared/types.js b/packages/react-devtools-shared/src/shared/types.js new file mode 100644 index 0000000000000..130cea5dd602f --- /dev/null +++ b/packages/react-devtools-shared/src/shared/types.js @@ -0,0 +1,14 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +export type Source = { + sourceURL: string, + line: number, + column: number, +};