Skip to content

Commit

Permalink
refactor[devtools]: lazily define source for fiber based on component…
Browse files Browse the repository at this point in the history 
… stacks (facebook#28351)

`_debugSource` was removed in
facebook#28265.

This PR migrates DevTools to define `source` for Fiber based on
component stacks. This will be done lazily for inspected elements, once
user clicks on the element in the tree.

`DevToolsComponentStackFrame.js` was just copy-pasted from the
implementation in `ReactComponentStackFrame`.

Symbolication part is done in
facebook#28471 and stacked on this commit.
  • Loading branch information
@hoxyq @AndyPengc12
hoxyq authored and AndyPengc12 committed Apr 15, 2024
1 parent 4b2e2d0 commit 75140aa
Show file tree
Hide file tree
Showing 15 changed files with 446 additions and 112 deletions.
4 changes: 2 additions & 2 deletions packages/react-devtools-core/src/standalone.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ function canViewElementSourceFunction(

const {source} = inspectedElement;

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

function viewElementSourceFunction(
Expand All @@ -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);
}
Expand Down
5 changes: 2 additions & 3 deletions packages/react-devtools-inline/__tests__/__e2e__/components.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
12 changes: 9 additions & 3 deletions packages/react-devtools-shared/src/__tests__/inspectedElement-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down
43 changes: 20 additions & 23 deletions packages/react-devtools-shared/src/__tests__/storeComponentFilters-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = () => <div>Hi</div>;
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 <div>{props.message}</div>;
};

await actAsync(async () => render(<Component />));
await actAsync(async () => render(<Component message="Hi" />));
expect(store).toMatchInlineSnapshot(`
[root]
▾ <Component>
Expand All @@ -242,13 +247,7 @@ describe('Store component filters', () => {
]),
);

// TODO: Filtering should work on component location.
// expect(store).toMatchInlineSnapshot(`[root]`);
expect(store).toMatchInlineSnapshot(`
[root]
▾ <Component>
<div>
`);
expect(store).toMatchInlineSnapshot(`[root]`);

await actAsync(
async () =>
Expand Down Expand Up @@ -497,19 +496,17 @@ describe('Store component filters', () => {
]),
);

utils.act(
() =>
utils.withErrorsOrWarningsIgnored(['test-only:'], () => {
render(
<React.Fragment>
<ComponentWithError />
<ComponentWithWarning />
<ComponentWithWarningAndError />
</React.Fragment>,
);
}),
false,
);
utils.withErrorsOrWarningsIgnored(['test-only:'], () => {
utils.act(() => {
render(
<React.Fragment>
<ComponentWithError />
<ComponentWithWarning />
<ComponentWithWarningAndError />
</React.Fragment>,
);
}, false);
});

expect(store).toMatchInlineSnapshot(``);
expect(store.errorCount).toBe(0);
Expand Down
90 changes: 90 additions & 0 deletions packages/react-devtools-shared/src/__tests__/utils-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
formatWithStyles,
gt,
gte,
parseSourceFromComponentStack,
} from 'react-devtools-shared/src/backend/utils';
import {
REACT_SUSPENSE_LIST_TYPE as SuspenseList,
Expand Down Expand Up @@ -297,4 +298,93 @@ describe('utils', () => {
expect(isPlainObject(Object.create(null))).toBe(true);
});
});

describe('parseSourceFromComponentStack', () => {
it('should return null if passed empty string', () => {
expect(parseSourceFromComponentStack('')).toEqual(null);
});

it('should construct the source from the first frame if available', () => {
expect(
parseSourceFromComponentStack(
'at l (https://react.dev/_next/static/chunks/main-78a3b4c2aa4e4850.js:1:10389)\n' +
'at f (https://react.dev/_next/static/chunks/pages/%5B%5B...markdownPath%5D%5D-af2ed613aedf1d57.js:1:8519)\n' +
'at r (https://react.dev/_next/static/chunks/pages/_app-dd0b77ea7bd5b246.js:1:498)\n',
),
).toEqual({
sourceURL:
'https://react.dev/_next/static/chunks/main-78a3b4c2aa4e4850.js',
line: 1,
column: 10389,
});
});

it('should construct the source from highest available frame', () => {
expect(
parseSourceFromComponentStack(
' at Q\n' +
' at a\n' +
' at m (https://react.dev/_next/static/chunks/848-122f91e9565d9ffa.js:5:9236)\n' +
' at div\n' +
' at div\n' +
' at div\n' +
' at nav\n' +
' at div\n' +
' at te (https://react.dev/_next/static/chunks/363-3c5f1b553b6be118.js:1:158857)\n' +
' at tt (https://react.dev/_next/static/chunks/363-3c5f1b553b6be118.js:1:165520)\n' +
' at f (https://react.dev/_next/static/chunks/pages/%5B%5B...markdownPath%5D%5D-af2ed613aedf1d57.js:1:8519)',
),
).toEqual({
sourceURL:
'https://react.dev/_next/static/chunks/848-122f91e9565d9ffa.js',
line: 5,
column: 9236,
});
});

it('should construct the source from frame, which has only url specified', () => {
expect(
parseSourceFromComponentStack(
' at Q\n' +
' at a\n' +
' at https://react.dev/_next/static/chunks/848-122f91e9565d9ffa.js:5:9236\n',
),
).toEqual({
sourceURL:
'https://react.dev/_next/static/chunks/848-122f91e9565d9ffa.js',
line: 5,
column: 9236,
});
});

it('should parse sourceURL correctly if it includes parentheses', () => {
expect(
parseSourceFromComponentStack(
'at HotReload (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/client/components/react-dev-overlay/hot-reloader-client.js:307:11)\n' +
' at Router (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/client/components/app-router.js:181:11)\n' +
' at ErrorBoundaryHandler (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/client/components/error-boundary.js:114:9)',
),
).toEqual({
sourceURL:
'webpack-internal:///(app-pages-browser)/./node_modules/next/dist/client/components/react-dev-overlay/hot-reloader-client.js',
line: 307,
column: 11,
});
});

it('should support Firefox stack', () => {
expect(
parseSourceFromComponentStack(
'tt@https://react.dev/_next/static/chunks/363-3c5f1b553b6be118.js:1:165558\n' +
'f@https://react.dev/_next/static/chunks/pages/%5B%5B...markdownPath%5D%5D-af2ed613aedf1d57.js:1:8535\n' +
'r@https://react.dev/_next/static/chunks/pages/_app-dd0b77ea7bd5b246.js:1:513',
),
).toEqual({
sourceURL:
'https://react.dev/_next/static/chunks/363-3c5f1b553b6be118.js',
line: 1,
column: 165558,
});
});
});
});
Loading

0 comments on commit 75140aa

Please sign in to comment.