Skip to content

Commit

Permalink
[Fizz] Enable owner stacks for SSR (#30152)
Browse files Browse the repository at this point in the history 
Stacked on #30142.

This tracks owners and their stacks in DEV in Fizz. We use the
ComponentStackNode as the data structure to track this information -
effectively like ReactComponentInfo (Server) or Fiber (Client). They're
the instance.

I then port them same logic from ReactFiberComponentStack,
ReactFiberOwnerStack and ReactFiberCallUserSpace to Fizz equivalents.

This gets us both owner stacks from `captureOwnerStack()`, as well as
appended to console.errors logged by Fizz, while rendering and in
onError.
  • Loading branch information
@sebmarkbage
sebmarkbage authored Jul 1, 2024
1 parent ad59ddf commit 315109b
Show file tree
Hide file tree
Showing 9 changed files with 727 additions and 111 deletions.
181 changes: 141 additions & 40 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
View file
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
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
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
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

0 comments on commit 315109b

Please sign in to comment.