Skip to content

Commit

Permalink
[DevTools] Implement Owner Stacks (facebook#30417)
Browse files Browse the repository at this point in the history 
Stacked on facebook#30410.

Use "owner stacks" as the appended component stack if it is available on
the Fiber. This will only be available if the enableOwnerStacks flag is
on. Otherwise it fallback to parent stacks. In prod, there's no owner so
it's never added there.

I was going back and forth on whether to inject essentially
`captureOwnerStack` as part of the DevTools hooks or replicate the
implementation but decided to replicate the implementation.

The DevTools needs all the same information from internals to implement
owner views elsewhere in the UI anyway so we're not saving anything in
terms of the scope of internals. Additionally, we really need this
information for non-current components as well like "rendered by" views
of the currently selected component.

It can also be useful if we need to change the format after the fact
like we did for parent stacks in:
facebook#30289

Injecting the implementation would lock us into specifics both in terms
of what the core needs to provide and what the DevTools can use.

The implementation depends on the technique used in facebook#30369 which tags
frames to strip out with `react-stack-bottom-frame`. That's how the
implementation knows how to materialize the error if it hasn't already.

Firefox:

<img width="487" alt="Screenshot 2024-07-21 at 11 33 37 PM"
src="https://github.com/user-attachments/assets/d3539b53-4578-4fdd-af25-25698b2bcc7d">

Follow up: One thing about this view is that it doesn't include the
current actual synchronous stack. When I used to append these I would
include both the real current stack and the owner stack. That's because
the owner stack doesn't include the name of the currently executing
component. I'll probably inject the current stack too in addition to the
owner stack. This is similar to how native Async Stacks are basically
just appended onto the current stack rather than its own.
  • Loading branch information
@sebmarkbage
sebmarkbage authored Jul 22, 2024
1 parent b7e7f1a commit 43dac1e
Show file tree
Hide file tree
Showing 8 changed files with 1,697 additions and 1,477 deletions.
2 changes: 1 addition & 1 deletion packages/react-devtools-extensions/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,6 @@
"workerize-loader": "^2.0.2"
},
"dependencies": {
"web-ext": "^4"
"web-ext": "^8"
}
}
31 changes: 21 additions & 10 deletions packages/react-devtools-shared/src/__tests__/componentStacks-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ describe('component stack', () => {
let act;
let mockError;
let mockWarn;
let supportsOwnerStacks;

beforeEach(() => {
// Intercept native console methods before DevTools bootstraps.
Expand All @@ -31,6 +32,12 @@ describe('component stack', () => {
act = utils.act;

React = require('react');
if (
React.version.startsWith('19') &&
React.version.includes('experimental')
) {
supportsOwnerStacks = true;
}
});

const {render} = getVersionedRenderImplementation();
Expand All @@ -49,13 +56,13 @@ describe('component stack', () => {

expect(mockError).toHaveBeenCalledWith(
'Test error.',
'\n in Child (at **)' +
(supportsOwnerStacks ? '' : '\n in Child (at **)') +
'\n in Parent (at **)' +
'\n in Grandparent (at **)',
);
expect(mockWarn).toHaveBeenCalledWith(
'Test warning.',
'\n in Child (at **)' +
(supportsOwnerStacks ? '' : '\n in Child (at **)') +
'\n in Parent (at **)' +
'\n in Grandparent (at **)',
);
Expand Down Expand Up @@ -112,17 +119,21 @@ describe('component stack', () => {

expect(mockError).toHaveBeenCalledWith(
'Test error.',
'\n in Child (at **)' +
'\n in ServerComponent (at **)' +
'\n in Parent (at **)' +
'\n in Grandparent (at **)',
supportsOwnerStacks
? '\n in Child (at **)'
: '\n in Child (at **)' +
'\n in ServerComponent (at **)' +
'\n in Parent (at **)' +
'\n in Grandparent (at **)',
);
expect(mockWarn).toHaveBeenCalledWith(
'Test warning.',
'\n in Child (at **)' +
'\n in ServerComponent (at **)' +
'\n in Parent (at **)' +
'\n in Grandparent (at **)',
supportsOwnerStacks
? '\n in Child (at **)'
: '\n in Child (at **)' +
'\n in ServerComponent (at **)' +
'\n in Parent (at **)' +
'\n in Grandparent (at **)',
);
});
});
79 changes: 61 additions & 18 deletions packages/react-devtools-shared/src/__tests__/console-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ let mockWarn;
let patchConsole;
let unpatchConsole;
let rendererID;
let supportsOwnerStacks = false;

describe('console', () => {
beforeEach(() => {
Expand Down Expand Up @@ -62,6 +63,12 @@ describe('console', () => {
};

React = require('react');
if (
React.version.startsWith('19') &&
React.version.includes('experimental')
) {
supportsOwnerStacks = true;
}
ReactDOMClient = require('react-dom/client');

const utils = require('./utils');
Expand Down Expand Up @@ -224,13 +231,17 @@ describe('console', () => {
expect(mockWarn.mock.calls[0]).toHaveLength(2);
expect(mockWarn.mock.calls[0][0]).toBe('warn');
expect(normalizeCodeLocInfo(mockWarn.mock.calls[0][1])).toEqual(
'\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
supportsOwnerStacks
? '\n in Parent (at **)'
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);
expect(mockError).toHaveBeenCalledTimes(1);
expect(mockError.mock.calls[0]).toHaveLength(2);
expect(mockError.mock.calls[0][0]).toBe('error');
expect(normalizeCodeLocInfo(mockError.mock.calls[0][1])).toBe(
'\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
supportsOwnerStacks
? '\n in Parent (at **)'
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);
});

Expand Down Expand Up @@ -267,23 +278,31 @@ describe('console', () => {
expect(mockWarn.mock.calls[0]).toHaveLength(2);
expect(mockWarn.mock.calls[0][0]).toBe('active warn');
expect(normalizeCodeLocInfo(mockWarn.mock.calls[0][1])).toEqual(
'\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
supportsOwnerStacks
? '\n in Parent (at **)'
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);
expect(mockWarn.mock.calls[1]).toHaveLength(2);
expect(mockWarn.mock.calls[1][0]).toBe('passive warn');
expect(normalizeCodeLocInfo(mockWarn.mock.calls[1][1])).toEqual(
'\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
supportsOwnerStacks
? '\n in Parent (at **)'
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);
expect(mockError).toHaveBeenCalledTimes(2);
expect(mockError.mock.calls[0]).toHaveLength(2);
expect(mockError.mock.calls[0][0]).toBe('active error');
expect(normalizeCodeLocInfo(mockError.mock.calls[0][1])).toBe(
'\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
supportsOwnerStacks
? '\n in Parent (at **)'
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);
expect(mockError.mock.calls[1]).toHaveLength(2);
expect(mockError.mock.calls[1][0]).toBe('passive error');
expect(normalizeCodeLocInfo(mockError.mock.calls[1][1])).toBe(
'\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
supportsOwnerStacks
? '\n in Parent (at **)'
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);
});

Expand Down Expand Up @@ -325,23 +344,31 @@ describe('console', () => {
expect(mockWarn.mock.calls[0]).toHaveLength(2);
expect(mockWarn.mock.calls[0][0]).toBe('didMount warn');
expect(normalizeCodeLocInfo(mockWarn.mock.calls[0][1])).toEqual(
'\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
supportsOwnerStacks
? '\n in Parent (at **)'
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);
expect(mockWarn.mock.calls[1]).toHaveLength(2);
expect(mockWarn.mock.calls[1][0]).toBe('didUpdate warn');
expect(normalizeCodeLocInfo(mockWarn.mock.calls[1][1])).toEqual(
'\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
supportsOwnerStacks
? '\n in Parent (at **)'
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);
expect(mockError).toHaveBeenCalledTimes(2);
expect(mockError.mock.calls[0]).toHaveLength(2);
expect(mockError.mock.calls[0][0]).toBe('didMount error');
expect(normalizeCodeLocInfo(mockError.mock.calls[0][1])).toBe(
'\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
supportsOwnerStacks
? '\n in Parent (at **)'
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);
expect(mockError.mock.calls[1]).toHaveLength(2);
expect(mockError.mock.calls[1][0]).toBe('didUpdate error');
expect(normalizeCodeLocInfo(mockError.mock.calls[1][1])).toBe(
'\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
supportsOwnerStacks
? '\n in Parent (at **)'
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);
});

Expand Down Expand Up @@ -375,13 +402,17 @@ describe('console', () => {
expect(mockWarn.mock.calls[0]).toHaveLength(2);
expect(mockWarn.mock.calls[0][0]).toBe('warn');
expect(normalizeCodeLocInfo(mockWarn.mock.calls[0][1])).toEqual(
'\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
supportsOwnerStacks
? '\n in Parent (at **)'
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);
expect(mockError).toHaveBeenCalledTimes(1);
expect(mockError.mock.calls[0]).toHaveLength(2);
expect(mockError.mock.calls[0][0]).toBe('error');
expect(normalizeCodeLocInfo(mockError.mock.calls[0][1])).toBe(
'\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
supportsOwnerStacks
? '\n in Parent (at **)'
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);
});

Expand Down Expand Up @@ -465,13 +496,17 @@ describe('console', () => {
expect(mockWarn.mock.calls[0]).toHaveLength(2);
expect(mockWarn.mock.calls[0][0]).toBe('warn');
expect(normalizeCodeLocInfo(mockWarn.mock.calls[0][1])).toEqual(
'\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
supportsOwnerStacks
? '\n in Parent (at **)'
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);
expect(mockError).toHaveBeenCalledTimes(1);
expect(mockError.mock.calls[0]).toHaveLength(2);
expect(mockError.mock.calls[0][0]).toBe('error');
expect(normalizeCodeLocInfo(mockError.mock.calls[0][1])).toBe(
'\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
supportsOwnerStacks
? '\n in Parent (at **)'
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);
});

Expand Down Expand Up @@ -996,29 +1031,37 @@ describe('console', () => {
expect(mockWarn).toHaveBeenCalledTimes(2);
expect(mockWarn.mock.calls[0]).toHaveLength(2);
expect(normalizeCodeLocInfo(mockWarn.mock.calls[0][1])).toEqual(
'\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
supportsOwnerStacks
? '\n in Parent (at **)'
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);
expect(mockWarn.mock.calls[1]).toHaveLength(3);
expect(mockWarn.mock.calls[1][0]).toEqual(
'\x1b[2;38;2;124;124;124m%s %o\x1b[0m',
);
expect(mockWarn.mock.calls[1][1]).toMatch('warn');
expect(normalizeCodeLocInfo(mockWarn.mock.calls[1][2]).trim()).toEqual(
'in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
supportsOwnerStacks
? 'in Parent (at **)'
: 'in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);

expect(mockError).toHaveBeenCalledTimes(2);
expect(mockError.mock.calls[0]).toHaveLength(2);
expect(normalizeCodeLocInfo(mockError.mock.calls[0][1])).toEqual(
'\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
supportsOwnerStacks
? '\n in Parent (at **)'
: '\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);
expect(mockError.mock.calls[1]).toHaveLength(3);
expect(mockError.mock.calls[1][0]).toEqual(
'\x1b[2;38;2;124;124;124m%s %o\x1b[0m',
);
expect(mockError.mock.calls[1][1]).toEqual('error');
expect(normalizeCodeLocInfo(mockError.mock.calls[1][2]).trim()).toEqual(
'in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
supportsOwnerStacks
? 'in Parent (at **)'
: 'in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);
});
});
Expand Down
Loading

0 comments on commit 43dac1e

Please sign in to comment.