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 
… stack
  • Loading branch information
@hoxyq
hoxyq committed Feb 16, 2024
1 parent 2e470a7 commit 440fc26
Show file tree
Hide file tree
Showing 12 changed files with 279 additions and 95 deletions.
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
40 changes: 18 additions & 22 deletions packages/react-devtools-shared/src/__tests__/storeComponentFilters-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,9 +226,13 @@ describe('Store component filters', () => {

// @reactVersion >= 16.0
it('should filter by path', async () => {
const Component = () => <div>Hi</div>;
// 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 +246,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 +495,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
194 changes: 138 additions & 56 deletions packages/react-devtools-shared/src/backend/DevToolsComponentStackFrame.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,6 @@ export function describeNativeComponentFrame(
}
}

let control;

const previousPrepareStackTrace = Error.prepareStackTrace;
// $FlowFixMe[incompatible-type] It does accept undefined.
Error.prepareStackTrace = undefined;
Expand All @@ -91,64 +89,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
Expand All @@ -167,7 +241,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 "<anonymous>"
// but we have a user-provided "displayName"
// splice it in to make the stack more readable.
if (fn.displayName && frame.includes('<anonymous>')) {
frame = frame.replace('<anonymous>', fn.displayName);
}

if (__DEV__) {
if (typeof fn === 'function') {
componentFrameCache.set(fn, frame);
Expand Down
1 change: 1 addition & 0 deletions packages/react-devtools-shared/src/backend/legacy/renderer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading

0 comments on commit 440fc26

Please sign in to comment.