Skip to content

[Fizz] Enable owner stacks for SSR #30152

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jul 1, 2024
Merged

[Fizz] Enable owner stacks for SSR #30152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c985975
Track current owner in Fizz using the current task
sebmarkbage Jun 30, 2024
c645c4b
Track owner and stacks for component stack nodes
sebmarkbage Jul 1, 2024
5a48c1e
Format owner stack in captureOwnerStack
sebmarkbage Jul 1, 2024
c66bb4a
Fix test
sebmarkbage Jul 1, 2024
2238497
Update another test
sebmarkbage Jul 1, 2024
ec4b3fc
Add owner based debug information to key warning
sebmarkbage Jul 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
181 changes: 141 additions & 40 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1766,9 +1766,9 @@ describe('ReactDOMFizzServer', () => {
// Intentionally trigger a key warning here.
return (
<div>
{children.map(t => (
<span>{t}</span>
))}
{children.map(function mapper(t) {
return <span>{t}</span>;
})}
</div>
);
}
Expand Down Expand Up @@ -1813,11 +1813,15 @@ describe('ReactDOMFizzServer', () => {
'<%s /> is using incorrect casing. Use PascalCase for React components, or lowercase for HTML elements.%s',
'inCorrectTag',
'\n' +
' in inCorrectTag (at **)\n' +
' in C (at **)\n' +
' in Suspense (at **)\n' +
' in div (at **)\n' +
' in A (at **)',
(gate(flags => flags.enableOwnerStacks)
? ' in inCorrectTag (at **)\n' +
' in C (at **)\n' +
' in A (at **)'
: ' in inCorrectTag (at **)\n' +
' in C (at **)\n' +
' in Suspense (at **)\n' +
' in div (at **)\n' +
' in A (at **)'),
);
mockError.mockClear();
} else {
Expand All @@ -1833,22 +1837,19 @@ describe('ReactDOMFizzServer', () => {
expect(mockError).toHaveBeenCalledWith(
'Each child in a list should have a unique "key" prop.%s%s' +
' See https://react.dev/link/warning-keys for more information.%s',
gate(flags => flags.enableOwnerStacks)
? // We currently don't track owners in Fizz which is responsible for this frame.
''
: '\n\nCheck the top-level render call using <div>.',
'\n\nCheck the render method of `B`.',
'',
'\n' +
' in span (at **)\n' +
// TODO: Because this validates after the div has been mounted, it is part of
// the parent stack but since owner stacks will switch to owners this goes away again.
(gate(flags => flags.enableOwnerStacks)
? ' in div (at **)\n'
: '') +
' in B (at **)\n' +
' in Suspense (at **)\n' +
' in div (at **)\n' +
' in A (at **)',
? ' in span (at **)\n' +
' in mapper (at **)\n' +
' in B (at **)\n' +
' in A (at **)'
: ' in span (at **)\n' +
' in B (at **)\n' +
' in Suspense (at **)\n' +
' in div (at **)\n' +
' in A (at **)'),
);
} else {
expect(mockError).not.toHaveBeenCalled();
Expand Down Expand Up @@ -6519,24 +6520,25 @@ describe('ReactDOMFizzServer', () => {
mockError(...args.map(normalizeCodeLocInfo));
};

function App() {
return (
<html>
<body>
<script>{2}</script>
<script>
{['try { foo() } catch (e) {} ;', 'try { bar() } catch (e) {} ;']}
</script>
<script>
<MyScript />
</script>
</body>
</html>
);
}

try {
await act(async () => {
const {pipe} = renderToPipeableStream(
<html>
<body>
<script>{2}</script>
<script>
{[
'try { foo() } catch (e) {} ;',
'try { bar() } catch (e) {} ;',
]}
</script>
<script>
<MyScript />
</script>
</body>
</html>,
);
const {pipe} = renderToPipeableStream(<App />);
pipe(writable);
});

Expand All @@ -6545,17 +6547,29 @@ describe('ReactDOMFizzServer', () => {
expect(mockError.mock.calls[0]).toEqual([
'A script element was rendered with %s. If script element has children it must be a single string. Consider using dangerouslySetInnerHTML or passing a plain string as children.%s',
'a number for children',
componentStack(['script', 'body', 'html']),
componentStack(
gate(flags => flags.enableOwnerStacks)
? ['script', 'App']
: ['script', 'body', 'html', 'App'],
),
]);
expect(mockError.mock.calls[1]).toEqual([
'A script element was rendered with %s. If script element has children it must be a single string. Consider using dangerouslySetInnerHTML or passing a plain string as children.%s',
'an array for children',
componentStack(['script', 'body', 'html']),
componentStack(
gate(flags => flags.enableOwnerStacks)
? ['script', 'App']
: ['script', 'body', 'html', 'App'],
),
]);
expect(mockError.mock.calls[2]).toEqual([
'A script element was rendered with %s. If script element has children it must be a single string. Consider using dangerouslySetInnerHTML or passing a plain string as children.%s',
'something unexpected for children',
componentStack(['script', 'body', 'html']),
componentStack(
gate(flags => flags.enableOwnerStacks)
? ['script', 'App']
: ['script', 'body', 'html', 'App'],
),
]);
} else {
expect(mockError.mock.calls.length).toBe(0);
Expand Down Expand Up @@ -8148,4 +8162,91 @@ describe('ReactDOMFizzServer', () => {

expect(document.body.textContent).toBe('HelloWorld');
});

// @gate __DEV__ && enableOwnerStacks
it('can get the component owner stacks during rendering in dev', async () => {
let stack;

function Foo() {
return <Bar />;
}
function Bar() {
return (
<div>
<Baz />
</div>
);
}
function Baz() {
stack = React.captureOwnerStack();
return <span>hi</span>;
}

await act(() => {
const {pipe} = renderToPipeableStream(
<div>
<Foo />
</div>,
);
pipe(writable);
});

expect(normalizeCodeLocInfo(stack)).toBe(
'\n in Bar (at **)' + '\n in Foo (at **)',
);
});

// @gate __DEV__ && enableOwnerStacks
it('can get the component owner stacks for onError in dev', async () => {
const thrownError = new Error('hi');
let caughtError;
let parentStack;
let ownerStack;

function Foo() {
return <Bar />;
}
function Bar() {
return (
<div>
<Baz />
</div>
);
}
function Baz() {
throw thrownError;
}

await expect(async () => {
await act(() => {
const {pipe} = renderToPipeableStream(
<div>
<Foo />
</div>,
{
onError(error, errorInfo) {
caughtError = error;
parentStack = errorInfo.componentStack;
ownerStack = React.captureOwnerStack
? React.captureOwnerStack()
: null;
},
},
);
pipe(writable);
});
}).rejects.toThrow(thrownError);

expect(caughtError).toBe(thrownError);
expect(normalizeCodeLocInfo(parentStack)).toBe(
'\n in Baz (at **)' +
'\n in div (at **)' +
'\n in Bar (at **)' +
'\n in Foo (at **)' +
'\n in div (at **)',
);
expect(normalizeCodeLocInfo(ownerStack)).toBe(
'\n in Bar (at **)' + '\n in Foo (at **)',
);
});
});
65 changes: 41 additions & 24 deletions packages/react-dom/src/__tests__/ReactServerRendering-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -835,21 +835,30 @@ describe('ReactDOMServer', () => {

expect(() => ReactDOMServer.renderToString(<App />)).toErrorDev([
'Invalid ARIA attribute `ariaTypo`. ARIA attributes follow the pattern aria-* and must be lowercase.\n' +
' in span (at **)\n' +
' in b (at **)\n' +
' in C (at **)\n' +
' in font (at **)\n' +
' in B (at **)\n' +
' in Child (at **)\n' +
' in span (at **)\n' +
' in div (at **)\n' +
' in App (at **)',
(gate(flags => flags.enableOwnerStacks)
? ' in span (at **)\n' +
' in B (at **)\n' +
' in Child (at **)\n' +
' in App (at **)'
: ' in span (at **)\n' +
' in b (at **)\n' +
' in C (at **)\n' +
' in font (at **)\n' +
' in B (at **)\n' +
' in Child (at **)\n' +
' in span (at **)\n' +
' in div (at **)\n' +
' in App (at **)'),
'Invalid ARIA attribute `ariaTypo2`. ARIA attributes follow the pattern aria-* and must be lowercase.\n' +
' in span (at **)\n' +
' in Child (at **)\n' +
' in span (at **)\n' +
' in div (at **)\n' +
' in App (at **)',
(gate(flags => flags.enableOwnerStacks)
? ' in span (at **)\n' +
' in Child (at **)\n' +
' in App (at **)'
: ' in span (at **)\n' +
' in Child (at **)\n' +
' in span (at **)\n' +
' in div (at **)\n' +
' in App (at **)'),
]);
});

Expand Down Expand Up @@ -885,9 +894,11 @@ describe('ReactDOMServer', () => {
expect(() => ReactDOMServer.renderToString(<App />)).toErrorDev([
// ReactDOMServer(App > div > span)
'Invalid ARIA attribute `ariaTypo`. ARIA attributes follow the pattern aria-* and must be lowercase.\n' +
' in span (at **)\n' +
' in div (at **)\n' +
' in App (at **)',
(gate(flags => flags.enableOwnerStacks)
? ' in span (at **)\n' + ' in App (at **)'
: ' in span (at **)\n' +
' in div (at **)\n' +
' in App (at **)'),
// ReactDOMServer(App > div > Child) >>> ReactDOMServer(App2) >>> ReactDOMServer(blink)
'Invalid ARIA attribute `ariaTypo2`. ARIA attributes follow the pattern aria-* and must be lowercase.\n' +
' in blink (at **)',
Expand All @@ -898,15 +909,21 @@ describe('ReactDOMServer', () => {
' in App2 (at **)',
// ReactDOMServer(App > div > Child > span)
'Invalid ARIA attribute `ariaTypo4`. ARIA attributes follow the pattern aria-* and must be lowercase.\n' +
' in span (at **)\n' +
' in Child (at **)\n' +
' in div (at **)\n' +
' in App (at **)',
(gate(flags => flags.enableOwnerStacks)
? ' in span (at **)\n' +
' in Child (at **)\n' +
' in App (at **)'
: ' in span (at **)\n' +
' in Child (at **)\n' +
' in div (at **)\n' +
' in App (at **)'),
// ReactDOMServer(App > div > font)
'Invalid ARIA attribute `ariaTypo5`. ARIA attributes follow the pattern aria-* and must be lowercase.\n' +
' in font (at **)\n' +
' in div (at **)\n' +
' in App (at **)',
(gate(flags => flags.enableOwnerStacks)
? ' in font (at **)\n' + ' in App (at **)'
: ' in font (at **)\n' +
' in div (at **)\n' +
' in App (at **)'),
]);
});

Expand Down
3 changes: 2 additions & 1 deletion packages/react-reconciler/src/ReactInternalTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import type {
Transition,
} from './ReactFiberTracingMarkerComponent';
import type {ConcurrentUpdate} from './ReactFiberConcurrentUpdates';
import type {ComponentStackNode} from 'react-server/src/ReactFizzComponentStack';

// Unwind Circular: moved from ReactFiberHooks.old
export type HookType =
Expand Down Expand Up @@ -439,5 +440,5 @@ export type Dispatcher = {
export type AsyncDispatcher = {
getCacheForType: <T>(resourceType: () => T) => T,
// DEV-only (or !disableStringRefs)
getOwner: () => null | Fiber | ReactComponentInfo,
getOwner: () => null | Fiber | ReactComponentInfo | ComponentStackNode,
};
13 changes: 11 additions & 2 deletions packages/react-server/src/ReactFizzAsyncDispatcher.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,12 @@
*/

import type {AsyncDispatcher} from 'react-reconciler/src/ReactInternalTypes';
import type {ComponentStackNode} from './ReactFizzComponentStack';

import {disableStringRefs} from 'shared/ReactFeatureFlags';

import {currentTaskInDEV} from './ReactFizzCurrentTask';

function getCacheForType<T>(resourceType: () => T): T {
throw new Error('Not implemented.');
}
Expand All @@ -19,8 +22,14 @@ export const DefaultAsyncDispatcher: AsyncDispatcher = ({
getCacheForType,
}: any);

if (__DEV__ || !disableStringRefs) {
// Fizz never tracks owner but the JSX runtime looks for this.
if (__DEV__) {
DefaultAsyncDispatcher.getOwner = (): ComponentStackNode | null => {
if (currentTaskInDEV === null) {
return null;
}
return currentTaskInDEV.componentStack;
};
} else if (!disableStringRefs) {
DefaultAsyncDispatcher.getOwner = (): null => {
return null;
};
Expand Down
Loading