Skip to content

Commit 38f04cf

Browse files
committed
refactor[devtools]: lazily define source for fiber based on component stack
1 parent c979895 commit 38f04cf

File tree

14 files changed

+297
-112
lines changed

14 files changed

+297
-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: 19 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -225,10 +225,14 @@ describe('Store component filters', () => {
225225
});
226226

227227
// @reactVersion >= 16.0
228-
it('should filter by path', async () => {
229-
const Component = () => <div>Hi</div>;
228+
xit('should filter by path', async () => {
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
@@ -81,8 +81,6 @@ export function describeNativeComponentFrame(
8181
}
8282
}
8383

84-
let control;
85-
8684
const previousPrepareStackTrace = Error.prepareStackTrace;
8785
// $FlowFixMe[incompatible-type] It does accept undefined.
8886
Error.prepareStackTrace = undefined;
@@ -98,64 +96,140 @@ export function describeNativeComponentFrame(
9896
currentDispatcherRef.current = null;
9997
disableLogs();
10098

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