Skip to content

Commit 61bd004

Browse files
hoxyqhoxyq
authored
refactor[devtools]: lazily define source for fiber based on component stacks (#28351)
`_debugSource` was removed in #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 #28471 and stacked on this commit.
1 parent aa4eae6 commit 61bd004

File tree

15 files changed

+446
-112
lines changed

15 files changed

+446
-112
lines changed

‎packages/react-devtools-core/src/standalone.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ function canViewElementSourceFunction(
144144

145145
const {source} = inspectedElement;
146146

147-
return doesFilePathExist(source.fileName, projectRoots);
147+
return doesFilePathExist(source.sourceURL, projectRoots);
148148
}
149149

150150
function viewElementSourceFunction(
@@ -153,7 +153,7 @@ function viewElementSourceFunction(
153153
): void {
154154
const {source} = inspectedElement;
155155
if (source !== null) {
156-
launchEditor(source.fileName, source.lineNumber, projectRoots);
156+
launchEditor(source.sourceURL, source.line, projectRoots);
157157
} else {
158158
log.error('Cannot inspect element', id);
159159
}

‎packages/react-devtools-inline/__tests__/__e2e__/components.test.js

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -92,15 +92,14 @@ test.describe('Components', () => {
9292
? valueElement.value
9393
: valueElement.innerText;
9494

95-
return [name, value, source ? source.innerText : null];
95+
return [name, value, source.innerText];
9696
},
9797
{name: isEditableName, value: isEditableValue}
9898
);
9999

100100
expect(propName).toBe('label');
101101
expect(propValue).toBe('"one"');
102-
expect(sourceText).toBe(null);
103-
// TODO: expect(sourceText).toMatch(/ListApp[a-zA-Z]*\.js/);
102+
expect(sourceText).toMatch(/e2e-app[a-zA-Z]*\.js/);
104103
});
105104

106105
test('should allow props to be edited', async () => {

‎packages/react-devtools-shared/src/__tests__/inspectedElement-test.js

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -424,7 +424,9 @@ describe('InspectedElement', () => {
424424
targetRenderCount = 0;
425425

426426
let inspectedElement = await inspectElementAtIndex(1);
427-
expect(targetRenderCount).toBe(1);
427+
// One more because we call render function for generating component stack,
428+
// which is required for defining source location
429+
expect(targetRenderCount).toBe(2);
428430
expect(inspectedElement.props).toMatchInlineSnapshot(`
429431
{
430432
"a": 1,
@@ -485,7 +487,9 @@ describe('InspectedElement', () => {
485487
targetRenderCount = 0;
486488

487489
let inspectedElement = await inspectElementAtIndex(1);
488-
expect(targetRenderCount).toBe(1);
490+
// One more because we call render function for generating component stack,
491+
// which is required for defining source location
492+
expect(targetRenderCount).toBe(2);
489493
expect(inspectedElement.props).toMatchInlineSnapshot(`
490494
{
491495
"a": 1,
@@ -555,7 +559,9 @@ describe('InspectedElement', () => {
555559
const inspectedElement = await inspectElementAtIndex(0);
556560

557561
expect(inspectedElement).not.toBe(null);
558-
expect(targetRenderCount).toBe(2);
562+
// One more because we call render function for generating component stack,
563+
// which is required for defining source location
564+
expect(targetRenderCount).toBe(3);
559565
expect(console.error).toHaveBeenCalledTimes(1);
560566
expect(console.info).toHaveBeenCalledTimes(1);
561567
expect(console.log).toHaveBeenCalledTimes(1);

‎packages/react-devtools-shared/src/__tests__/storeComponentFilters-test.js

Lines changed: 20 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -224,11 +224,16 @@ describe('Store component filters', () => {
224224
`);
225225
});
226226

227+
// Disabled: filtering by path was removed, source is now determined lazily, including symbolication if applicable
227228
// @reactVersion >= 16.0
228-
it('should filter by path', async () => {
229-
const Component = () => <div>Hi</div>;
229+
xit('should filter by path', async () => {
230+
// This component should use props object in order to throw for component stack generation
231+
// See ReactComponentStackFrame:155 or DevToolsComponentStackFrame:147
232+
const Component = props => {
233+
return <div>{props.message}</div>;
234+
};
230235

231-
await actAsync(async () => render(<Component />));
236+
await actAsync(async () => render(<Component message="Hi" />));
232237
expect(store).toMatchInlineSnapshot(`
233238
[root]
234239
▾ <Component>
@@ -242,13 +247,7 @@ describe('Store component filters', () => {
242247
]),
243248
);
244249

245-
// TODO: Filtering should work on component location.
246-
// expect(store).toMatchInlineSnapshot(`[root]`);
247-
expect(store).toMatchInlineSnapshot(`
248-
[root]
249-
▾ <Component>
250-
<div>
251-
`);
250+
expect(store).toMatchInlineSnapshot(`[root]`);
252251

253252
await actAsync(
254253
async () =>
@@ -497,19 +496,17 @@ describe('Store component filters', () => {
497496
]),
498497
);
499498

500-
utils.act(
501-
() =>
502-
utils.withErrorsOrWarningsIgnored(['test-only:'], () => {
503-
render(
504-
<React.Fragment>
505-
<ComponentWithError />
506-
<ComponentWithWarning />
507-
<ComponentWithWarningAndError />
508-
</React.Fragment>,
509-
);
510-
}),
511-
false,
512-
);
499+
utils.withErrorsOrWarningsIgnored(['test-only:'], () => {
500+
utils.act(() => {
501+
render(
502+
<React.Fragment>
503+
<ComponentWithError />
504+
<ComponentWithWarning />
505+
<ComponentWithWarningAndError />
506+
</React.Fragment>,
507+
);
508+
}, false);
509+
});
513510

514511
expect(store).toMatchInlineSnapshot(``);
515512
expect(store.errorCount).toBe(0);

‎packages/react-devtools-shared/src/__tests__/utils-test.js

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import {
1818
formatWithStyles,
1919
gt,
2020
gte,
21+
parseSourceFromComponentStack,
2122
} from 'react-devtools-shared/src/backend/utils';
2223
import {
2324
REACT_SUSPENSE_LIST_TYPE as SuspenseList,
@@ -297,4 +298,93 @@ describe('utils', () => {
297298
expect(isPlainObject(Object.create(null))).toBe(true);
298299
});
299300
});
301+
302+
describe('parseSourceFromComponentStack', () => {
303+
it('should return null if passed empty string', () => {
304+
expect(parseSourceFromComponentStack('')).toEqual(null);
305+
});
306+
307+
it('should construct the source from the first frame if available', () => {
308+
expect(
309+
parseSourceFromComponentStack(
310+
'at l (https://react.dev/_next/static/chunks/main-78a3b4c2aa4e4850.js:1:10389)\n' +
311+
'at f (https://react.dev/_next/static/chunks/pages/%5B%5B...markdownPath%5D%5D-af2ed613aedf1d57.js:1:8519)\n' +
312+
'at r (https://react.dev/_next/static/chunks/pages/_app-dd0b77ea7bd5b246.js:1:498)\n',
313+
),
314+
).toEqual({
315+
sourceURL:
316+
'https://react.dev/_next/static/chunks/main-78a3b4c2aa4e4850.js',
317+
line: 1,
318+
column: 10389,
319+
});
320+
});
321+
322+
it('should construct the source from highest available frame', () => {
323+
expect(
324+
parseSourceFromComponentStack(
325+
' at Q\n' +
326+
' at a\n' +
327+
' at m (https://react.dev/_next/static/chunks/848-122f91e9565d9ffa.js:5:9236)\n' +
328+
' at div\n' +
329+
' at div\n' +
330+
' at div\n' +
331+
' at nav\n' +
332+
' at div\n' +
333+
' at te (https://react.dev/_next/static/chunks/363-3c5f1b553b6be118.js:1:158857)\n' +
334+
' at tt (https://react.dev/_next/static/chunks/363-3c5f1b553b6be118.js:1:165520)\n' +
335+
' at f (https://react.dev/_next/static/chunks/pages/%5B%5B...markdownPath%5D%5D-af2ed613aedf1d57.js:1:8519)',
336+
),
337+
).toEqual({
338+
sourceURL:
339+
'https://react.dev/_next/static/chunks/848-122f91e9565d9ffa.js',
340+
line: 5,
341+
column: 9236,
342+
});
343+
});
344+
345+
it('should construct the source from frame, which has only url specified', () => {
346+
expect(
347+
parseSourceFromComponentStack(
348+
' at Q\n' +
349+
' at a\n' +
350+
' at https://react.dev/_next/static/chunks/848-122f91e9565d9ffa.js:5:9236\n',
351+
),
352+
).toEqual({
353+
sourceURL:
354+
'https://react.dev/_next/static/chunks/848-122f91e9565d9ffa.js',
355+
line: 5,
356+
column: 9236,
357+
});
358+
});
359+
360+
it('should parse sourceURL correctly if it includes parentheses', () => {
361+
expect(
362+
parseSourceFromComponentStack(
363+
'at HotReload (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/client/components/react-dev-overlay/hot-reloader-client.js:307:11)\n' +
364+
' at Router (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/client/components/app-router.js:181:11)\n' +
365+
' at ErrorBoundaryHandler (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/client/components/error-boundary.js:114:9)',
366+
),
367+
).toEqual({
368+
sourceURL:
369+
'webpack-internal:///(app-pages-browser)/./node_modules/next/dist/client/components/react-dev-overlay/hot-reloader-client.js',
370+
line: 307,
371+
column: 11,
372+
});
373+
});
374+
375+
it('should support Firefox stack', () => {
376+
expect(
377+
parseSourceFromComponentStack(
378+
'tt@https://react.dev/_next/static/chunks/363-3c5f1b553b6be118.js:1:165558\n' +
379+
'f@https://react.dev/_next/static/chunks/pages/%5B%5B...markdownPath%5D%5D-af2ed613aedf1d57.js:1:8535\n' +
380+
'r@https://react.dev/_next/static/chunks/pages/_app-dd0b77ea7bd5b246.js:1:513',
381+
),
382+
).toEqual({
383+
sourceURL:
384+
'https://react.dev/_next/static/chunks/363-3c5f1b553b6be118.js',
385+
line: 1,
386+
column: 165558,
387+
});
388+
});
389+
});
300390
});

0 commit comments

Comments
 (0)