Skip to content

Commit b3aa93f

Browse files
committed
refactor[devtools]: lazily define source for fiber based on component stack
1 parent 2e470a7 commit b3aa93f

File tree

12 files changed

+278
-94
lines changed

12 files changed

+278
-94
lines changed

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,8 +99,7 @@ test.describe('Components', () => {
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(/ListApp[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: 18 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -226,9 +226,13 @@ describe('Store component filters', () => {
226226

227227
// @reactVersion >= 16.0
228228
it('should filter by path', async () => {
229-
const Component = () => <div>Hi</div>;
229+
// This component should use props object in order to throw for component stack generation
230+
// See ReactComponentStackFrame:155 or DevToolsComponentStackFrame:147
231+
const Component = props => {
232+
return <div>{props.message}</div>;
233+
};
230234

231-
await actAsync(async () => render(<Component />));
235+
await actAsync(async () => render(<Component message="Hi" />));
232236
expect(store).toMatchInlineSnapshot(`
233237
[root]
234238
▾ <Component>
@@ -242,13 +246,7 @@ describe('Store component filters', () => {
242246
]),
243247
);
244248

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

253251
await actAsync(
254252
async () =>
@@ -497,19 +495,17 @@ describe('Store component filters', () => {
497495
]),
498496
);
499497

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-
);
498+
utils.withErrorsOrWarningsIgnored(['test-only:'], () => {
499+
utils.act(() => {
500+
render(
501+
<React.Fragment>
502+
<ComponentWithError />
503+
<ComponentWithWarning />
504+
<ComponentWithWarningAndError />
505+
</React.Fragment>,
506+
);
507+
}, false);
508+
});
513509

514510
expect(store).toMatchInlineSnapshot(``);
515511
expect(store.errorCount).toBe(0);

packages/react-devtools-shared/src/backend/DevToolsComponentStackFrame.js

Lines changed: 138 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -74,8 +74,6 @@ export function describeNativeComponentFrame(
7474
}
7575
}
7676

77-
let control;
78-
7977
const previousPrepareStackTrace = Error.prepareStackTrace;
8078
// $FlowFixMe[incompatible-type] It does accept undefined.
8179
Error.prepareStackTrace = undefined;
@@ -91,64 +89,140 @@ export function describeNativeComponentFrame(
9189
currentDispatcherRef.current = null;
9290
disableLogs();
9391

94-
try {
95-
// This should throw.
96-
if (construct) {
97-
// Something should be setting the props in the constructor.
98-
const Fake = function () {
99-
throw Error();
100-
};
101-
// $FlowFixMe[prop-missing]
102-
Object.defineProperty(Fake.prototype, 'props', {
103-
set: function () {
104-
// We use a throwing setter instead of frozen or non-writable props
105-
// because that won't throw in a non-strict mode function.
106-
throw Error();
107-
},
108-
});
109-
if (typeof Reflect === 'object' && Reflect.construct) {
110-
// We construct a different control for this case to include any extra
111-
// frames added by the construct call.
112-
try {
113-
Reflect.construct(Fake, []);
114-
} catch (x) {
115-
control = x;
92+
// NOTE: keep in sync with the implementation in ReactComponentStackFrame
93+
94+
/**
95+
* Finding a common stack frame between sample and control errors can be
96+
* tricky given the different types and levels of stack trace truncation from
97+
* different JS VMs. So instead we'll attempt to control what that common
98+
* frame should be through this object method:
99+
* Having both the sample and control errors be in the function under the
100+
* `DescribeNativeComponentFrameRoot` property, + setting the `name` and
101+
* `displayName` properties of the function ensures that a stack
102+
* frame exists that has the method name `DescribeNativeComponentFrameRoot` in
103+
* it for both control and sample stacks.
104+
*/
105+
const RunInRootFrame = {
106+
DetermineComponentFrameRoot(): [?string, ?string] {
107+
let control;
108+
try {
109+
// This should throw.
110+
if (construct) {
111+
// Something should be setting the props in the constructor.
112+
const Fake = function () {
113+
throw Error();
114+
};
115+
// $FlowFixMe[prop-missing]
116+
Object.defineProperty(Fake.prototype, 'props', {
117+
set: function () {
118+
// We use a throwing setter instead of frozen or non-writable props
119+
// because that won't throw in a non-strict mode function.
120+
throw Error();
121+
},
122+
});
123+
if (typeof Reflect === 'object' && Reflect.construct) {
124+
// We construct a different control for this case to include any extra
125+
// frames added by the construct call.
126+
try {
127+
Reflect.construct(Fake, []);
128+
} catch (x) {
129+
control = x;
130+
}
131+
Reflect.construct(fn, [], Fake);
132+
} else {
133+
try {
134+
Fake.call();
135+
} catch (x) {
136+
control = x;
137+
}
138+
// $FlowFixMe[prop-missing] found when upgrading Flow
139+
fn.call(Fake.prototype);
140+
}
141+
} else {
142+
try {
143+
throw Error();
144+
} catch (x) {
145+
control = x;
146+
}
147+
// TODO(luna): This will currently only throw if the function component
148+
// tries to access React/ReactDOM/props. We should probably make this throw
149+
// in simple components too
150+
const maybePromise = fn();
151+
152+
// If the function component returns a promise, it's likely an async
153+
// component, which we don't yet support. Attach a noop catch handler to
154+
// silence the error.
155+
// TODO: Implement component stacks for async client components?
156+
if (maybePromise && typeof maybePromise.catch === 'function') {
157+
maybePromise.catch(() => {});
158+
}
116159
}
117-
Reflect.construct(fn, [], Fake);
118-
} else {
119-
try {
120-
Fake.call();
121-
} catch (x) {
122-
control = x;
160+
} catch (sample) {
161+
// This is inlined manually because closure doesn't do it for us.
162+
if (sample && control && typeof sample.stack === 'string') {
163+
return [sample.stack, control.stack];
123164
}
124-
// $FlowFixMe[prop-missing] found when upgrading Flow
125-
fn.call(Fake.prototype);
126-
}
127-
} else {
128-
try {
129-
throw Error();
130-
} catch (x) {
131-
control = x;
132165
}
133-
fn();
134-
}
135-
} catch (sample) {
136-
// This is inlined manually because closure doesn't do it for us.
137-
if (sample && control && typeof sample.stack === 'string') {
166+
return [null, null];
167+
},
168+
};
169+
// $FlowFixMe[prop-missing]
170+
RunInRootFrame.DetermineComponentFrameRoot.displayName =
171+
'DetermineComponentFrameRoot';
172+
const namePropDescriptor = Object.getOwnPropertyDescriptor(
173+
RunInRootFrame.DetermineComponentFrameRoot,
174+
'name',
175+
);
176+
// Before ES6, the `name` property was not configurable.
177+
if (namePropDescriptor && namePropDescriptor.configurable) {
178+
// V8 utilizes a function's `name` property when generating a stack trace.
179+
Object.defineProperty(
180+
RunInRootFrame.DetermineComponentFrameRoot,
181+
// Configurable properties can be updated even if its writable descriptor
182+
// is set to `false`.
183+
// $FlowFixMe[cannot-write]
184+
'name',
185+
{value: 'DetermineComponentFrameRoot'},
186+
);
187+
}
188+
189+
try {
190+
const [sampleStack, controlStack] =
191+
RunInRootFrame.DetermineComponentFrameRoot();
192+
if (sampleStack && controlStack) {
138193
// This extracts the first frame from the sample that isn't also in the control.
139194
// Skipping one frame that we assume is the frame that calls the two.
140-
const sampleLines = sample.stack.split('\n');
141-
const controlLines = control.stack.split('\n');
142-
let s = sampleLines.length - 1;
143-
let c = controlLines.length - 1;
144-
while (s >= 1 && c >= 0 && sampleLines[s] !== controlLines[c]) {
145-
// We expect at least one stack frame to be shared.
146-
// Typically this will be the root most one. However, stack frames may be
147-
// cut off due to maximum stack limits. In this case, one maybe cut off
148-
// earlier than the other. We assume that the sample is longer or the same
149-
// and there for cut off earlier. So we should find the root most frame in
150-
// the sample somewhere in the control.
151-
c--;
195+
const sampleLines = sampleStack.split('\n');
196+
const controlLines = controlStack.split('\n');
197+
let s = 0;
198+
let c = 0;
199+
while (
200+
s < sampleLines.length &&
201+
!sampleLines[s].includes('DetermineComponentFrameRoot')
202+
) {
203+
s++;
204+
}
205+
while (
206+
c < controlLines.length &&
207+
!controlLines[c].includes('DetermineComponentFrameRoot')
208+
) {
209+
c++;
210+
}
211+
// We couldn't find our intentionally injected common root frame, attempt
212+
// to find another common root frame by search from the bottom of the
213+
// control stack...
214+
if (s === sampleLines.length || c === controlLines.length) {
215+
s = sampleLines.length - 1;
216+
c = controlLines.length - 1;
217+
while (s >= 1 && c >= 0 && sampleLines[s] !== controlLines[c]) {
218+
// We expect at least one stack frame to be shared.
219+
// Typically this will be the root most one. However, stack frames may be
220+
// cut off due to maximum stack limits. In this case, one maybe cut off
221+
// earlier than the other. We assume that the sample is longer or the same
222+
// and there for cut off earlier. So we should find the root most frame in
223+
// the sample somewhere in the control.
224+
c--;
225+
}
152226
}
153227
for (; s >= 1 && c >= 0; s--, c--) {
154228
// Next we find the first one that isn't the same which should be the
@@ -167,7 +241,15 @@ export function describeNativeComponentFrame(
167241
// The next one that isn't the same should be our match though.
168242
if (c < 0 || sampleLines[s] !== controlLines[c]) {
169243
// V8 adds a "new" prefix for native classes. Let's remove it to make it prettier.
170-
const frame = '\n' + sampleLines[s].replace(' at new ', ' at ');
244+
let frame = '\n' + sampleLines[s].replace(' at new ', ' at ');
245+
246+
// If our component frame is labeled "<anonymous>"
247+
// but we have a user-provided "displayName"
248+
// splice it in to make the stack more readable.
249+
if (fn.displayName && frame.includes('<anonymous>')) {
250+
frame = frame.replace('<anonymous>', fn.displayName);
251+
}
252+
171253
if (__DEV__) {
172254
if (typeof fn === 'function') {
173255
componentFrameCache.set(fn, frame);

packages/react-devtools-shared/src/backend/legacy/renderer.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -828,6 +828,7 @@ export function attach(
828828

829829
// Can view component source location.
830830
canViewSource: type === ElementTypeClass || type === ElementTypeFunction,
831+
source: null,
831832

832833
// Only legacy context exists in legacy versions.
833834
hasLegacyContext: true,

0 commit comments

Comments
 (0)