From c985975ad116d1c01fd1ba3f9d5cdb2ca792c97d Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sun, 30 Jun 2024 18:41:30 -0400 Subject: [PATCH 1/6] Track current owner in Fizz using the current task We use the ComponentStackNode to represent the current component. --- .../src/ReactInternalTypes.js | 3 ++- .../src/ReactFizzAsyncDispatcher.js | 13 +++++++++++-- .../react-server/src/ReactFizzCurrentTask.js | 19 +++++++++++++++++++ packages/react-server/src/ReactFizzServer.js | 11 +++++------ 4 files changed, 37 insertions(+), 9 deletions(-) create mode 100644 packages/react-server/src/ReactFizzCurrentTask.js diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js index 8e58b5c44aa84..beee88de2549b 100644 --- a/packages/react-reconciler/src/ReactInternalTypes.js +++ b/packages/react-reconciler/src/ReactInternalTypes.js @@ -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 = @@ -439,5 +440,5 @@ export type Dispatcher = { export type AsyncDispatcher = { getCacheForType: (resourceType: () => T) => T, // DEV-only (or !disableStringRefs) - getOwner: () => null | Fiber | ReactComponentInfo, + getOwner: () => null | Fiber | ReactComponentInfo | ComponentStackNode, }; diff --git a/packages/react-server/src/ReactFizzAsyncDispatcher.js b/packages/react-server/src/ReactFizzAsyncDispatcher.js index 8e8cb2e219992..3a548ae138039 100644 --- a/packages/react-server/src/ReactFizzAsyncDispatcher.js +++ b/packages/react-server/src/ReactFizzAsyncDispatcher.js @@ -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(resourceType: () => T): T { throw new Error('Not implemented.'); } @@ -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; }; diff --git a/packages/react-server/src/ReactFizzCurrentTask.js b/packages/react-server/src/ReactFizzCurrentTask.js new file mode 100644 index 0000000000000..3bd62f0a5cde0 --- /dev/null +++ b/packages/react-server/src/ReactFizzCurrentTask.js @@ -0,0 +1,19 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +import type {Task} from './ReactFizzServer'; + +// DEV-only global reference to the currently executing task +export let currentTaskInDEV: null | Task = null; + +export function setCurrentTaskInDEV(task: null | Task): void { + if (__DEV__) { + currentTaskInDEV = task; + } +} diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index b5e9ed28fde52..3466a823307cd 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -115,6 +115,7 @@ import { import {DefaultAsyncDispatcher} from './ReactFizzAsyncDispatcher'; import {getStackByComponentStackNode} from './ReactFizzComponentStack'; import {emptyTreeContext, pushTreeContext} from './ReactFizzTreeContext'; +import {currentTaskInDEV, setCurrentTaskInDEV} from './ReactFizzCurrentTask'; import { getIteratorFn, @@ -790,8 +791,6 @@ function createPendingSegment( }; } -// DEV-only global reference to the currently executing task -let currentTaskInDEV: null | Task = null; function getCurrentStackInDEV(): string { if (__DEV__) { if (currentTaskInDEV === null || currentTaskInDEV.componentStack === null) { @@ -3775,7 +3774,7 @@ function retryRenderTask( let prevTaskInDEV = null; if (__DEV__) { prevTaskInDEV = currentTaskInDEV; - currentTaskInDEV = task; + setCurrentTaskInDEV(task); } const childrenLength = segment.children.length; @@ -3852,7 +3851,7 @@ function retryRenderTask( return; } finally { if (__DEV__) { - currentTaskInDEV = prevTaskInDEV; + setCurrentTaskInDEV(prevTaskInDEV); } } } @@ -3870,7 +3869,7 @@ function retryReplayTask(request: Request, task: ReplayTask): void { let prevTaskInDEV = null; if (__DEV__) { prevTaskInDEV = currentTaskInDEV; - currentTaskInDEV = task; + setCurrentTaskInDEV(task); } try { @@ -3939,7 +3938,7 @@ function retryReplayTask(request: Request, task: ReplayTask): void { return; } finally { if (__DEV__) { - currentTaskInDEV = prevTaskInDEV; + setCurrentTaskInDEV(prevTaskInDEV); } } } From c645c4b43ab35d93522e9a855b1fe286bcf09c64 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sun, 30 Jun 2024 20:35:55 -0400 Subject: [PATCH 2/6] Track owner and stacks for component stack nodes In DEV. --- .../src/ReactFizzComponentStack.js | 8 + packages/react-server/src/ReactFizzServer.js | 209 +++++++++++++++--- 2 files changed, 191 insertions(+), 26 deletions(-) diff --git a/packages/react-server/src/ReactFizzComponentStack.js b/packages/react-server/src/ReactFizzComponentStack.js index 84b4d82d8d45f..a59c1238152ca 100644 --- a/packages/react-server/src/ReactFizzComponentStack.js +++ b/packages/react-server/src/ReactFizzComponentStack.js @@ -7,6 +7,8 @@ * @flow */ +import type {ReactComponentInfo} from 'shared/ReactTypes'; + import { describeBuiltInComponentFrame, describeFunctionComponentFrame, @@ -18,16 +20,22 @@ type BuiltInComponentStackNode = { tag: 0, parent: null | ComponentStackNode, type: string, + owner?: null | ReactComponentInfo | ComponentStackNode, // DEV only + stack?: null | Error, // DEV only }; type FunctionComponentStackNode = { tag: 1, parent: null | ComponentStackNode, type: Function, + owner?: null | ReactComponentInfo | ComponentStackNode, // DEV only + stack?: null | Error, // DEV only }; type ClassComponentStackNode = { tag: 2, parent: null | ComponentStackNode, type: Function, + owner?: null | ReactComponentInfo | ComponentStackNode, // DEV only + stack?: null | Error, // DEV only }; export type ComponentStackNode = | BuiltInComponentStackNode diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index 3466a823307cd..b89255a6ffa79 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -20,6 +20,7 @@ import type { Wakeable, Thenable, ReactFormState, + ReactComponentInfo, } from 'shared/ReactTypes'; import type {LazyComponent as LazyComponentType} from 'react/src/ReactLazy'; import type { @@ -809,7 +810,18 @@ function getStackFromNode(stackNode: ComponentStackNode): string { function createBuiltInComponentStack( task: Task, type: string, + owner: null | ReactComponentInfo | ComponentStackNode, // DEV only + stack: null | Error, // DEV only ): ComponentStackNode { + if (__DEV__) { + return { + tag: 0, + parent: task.componentStack, + type, + owner, + stack, + }; + } return { tag: 0, parent: task.componentStack, @@ -819,7 +831,18 @@ function createBuiltInComponentStack( function createFunctionComponentStack( task: Task, type: Function, + owner: null | ReactComponentInfo | ComponentStackNode, // DEV only + stack: null | Error, // DEV only ): ComponentStackNode { + if (__DEV__) { + return { + tag: 1, + parent: task.componentStack, + type, + owner, + stack, + }; + } return { tag: 1, parent: task.componentStack, @@ -829,7 +852,18 @@ function createFunctionComponentStack( function createClassComponentStack( task: Task, type: Function, + owner: null | ReactComponentInfo | ComponentStackNode, // DEV only + stack: null | Error, // DEV only ): ComponentStackNode { + if (__DEV__) { + return { + tag: 2, + parent: task.componentStack, + type, + owner, + stack, + }; + } return { tag: 2, parent: task.componentStack, @@ -840,14 +874,16 @@ function createClassComponentStack( function createComponentStackFromType( task: Task, type: Function | string, + owner: null | ReactComponentInfo | ComponentStackNode, // DEV only + stack: null | Error, // DEV only ): ComponentStackNode { if (typeof type === 'string') { - return createBuiltInComponentStack(task, type); + return createBuiltInComponentStack(task, type, owner, stack); } if (shouldConstruct(type)) { - return createClassComponentStack(task, type); + return createClassComponentStack(task, type, owner, stack); } - return createFunctionComponentStack(task, type); + return createFunctionComponentStack(task, type, owner, stack); } type ThrownInfo = { @@ -966,6 +1002,8 @@ function renderSuspenseBoundary( someTask: Task, keyPath: KeyNode, props: Object, + owner: null | ReactComponentInfo | ComponentStackNode, // DEV only + stack: null | Error, // DEV only ): void { if (someTask.replay !== null) { // If we're replaying through this pass, it means we're replaying through @@ -988,7 +1026,7 @@ function renderSuspenseBoundary( // If we end up creating the fallback task we need it to have the correct stack which is // the stack for the boundary itself. We stash it here so we can use it if needed later const suspenseComponentStack = (task.componentStack = - createBuiltInComponentStack(task, 'Suspense')); + createBuiltInComponentStack(task, 'Suspense', owner, stack)); const prevKeyPath = task.keyPath; const parentBoundary = task.blockedBoundary; @@ -1161,12 +1199,14 @@ function replaySuspenseBoundary( childSlots: ResumeSlots, fallbackNodes: Array, fallbackSlots: ResumeSlots, + owner: null | ReactComponentInfo | ComponentStackNode, // DEV only + stack: null | Error, // DEV only ): void { const previousComponentStack = task.componentStack; // If we end up creating the fallback task we need it to have the correct stack which is // the stack for the boundary itself. We stash it here so we can use it if needed later const suspenseComponentStack = (task.componentStack = - createBuiltInComponentStack(task, 'Suspense')); + createBuiltInComponentStack(task, 'Suspense', owner, stack)); const prevKeyPath = task.keyPath; const previousReplaySet: ReplaySet = task.replay; @@ -1294,9 +1334,16 @@ function renderBackupSuspenseBoundary( task: Task, keyPath: KeyNode, props: Object, + owner: null | ReactComponentInfo | ComponentStackNode, // DEV only + stack: null | Error, // DEV only ) { const previousComponentStack = task.componentStack; - task.componentStack = createBuiltInComponentStack(task, 'Suspense'); + task.componentStack = createBuiltInComponentStack( + task, + 'Suspense', + owner, + stack, + ); const content = props.children; const segment = task.blockedSegment; @@ -1321,9 +1368,11 @@ function renderHostElement( keyPath: KeyNode, type: string, props: Object, + owner: null | ReactComponentInfo | ComponentStackNode, // DEV only + stack: null | Error, // DEV only ): void { const previousComponentStack = task.componentStack; - task.componentStack = createBuiltInComponentStack(task, type); + task.componentStack = createBuiltInComponentStack(task, type, owner, stack); const segment = task.blockedSegment; if (segment === null) { // Replay @@ -1503,10 +1552,17 @@ function renderClassComponent( keyPath: KeyNode, Component: any, props: any, + owner: null | ReactComponentInfo | ComponentStackNode, // DEV only + stack: null | Error, // DEV only ): void { const resolvedProps = resolveClassComponentProps(Component, props); const previousComponentStack = task.componentStack; - task.componentStack = createClassComponentStack(task, Component); + task.componentStack = createClassComponentStack( + task, + Component, + owner, + stack, + ); const maskedContext = !disableLegacyContext ? getMaskedContext(Component, task.legacyContext) : undefined; @@ -1541,13 +1597,20 @@ function renderFunctionComponent( keyPath: KeyNode, Component: any, props: any, + owner: null | ReactComponentInfo | ComponentStackNode, // DEV only + stack: null | Error, // DEV only ): void { let legacyContext; if (!disableLegacyContext) { legacyContext = getMaskedContext(Component, task.legacyContext); } const previousComponentStack = task.componentStack; - task.componentStack = createFunctionComponentStack(task, Component); + task.componentStack = createFunctionComponentStack( + task, + Component, + owner, + stack, + ); if (__DEV__) { if ( @@ -1750,9 +1813,16 @@ function renderForwardRef( type: any, props: Object, ref: any, + owner: null | ReactComponentInfo | ComponentStackNode, // DEV only + stack: null | Error, // DEV only ): void { const previousComponentStack = task.componentStack; - task.componentStack = createFunctionComponentStack(task, type.render); + task.componentStack = createFunctionComponentStack( + task, + type.render, + owner, + stack, + ); let propsWithoutRef; if (enableRefAsProp && 'ref' in props) { @@ -1802,13 +1872,24 @@ function renderMemo( type: any, props: Object, ref: any, + owner: null | ReactComponentInfo | ComponentStackNode, // DEV only + stack: null | Error, // DEV only ): void { const innerType = type.type; const resolvedProps = resolveDefaultPropsOnNonClassComponent( innerType, props, ); - renderElement(request, task, keyPath, innerType, resolvedProps, ref); + renderElement( + request, + task, + keyPath, + innerType, + resolvedProps, + ref, + owner, + stack, + ); } function renderContextConsumer( @@ -1875,9 +1956,11 @@ function renderLazyComponent( lazyComponent: LazyComponentType, props: Object, ref: any, + owner: null | ReactComponentInfo | ComponentStackNode, // DEV only + stack: null | Error, // DEV only ): void { const previousComponentStack = task.componentStack; - task.componentStack = createBuiltInComponentStack(task, 'Lazy'); + task.componentStack = createBuiltInComponentStack(task, 'Lazy', owner, stack); const payload = lazyComponent._payload; const init = lazyComponent._init; const Component = init(payload); @@ -1885,7 +1968,16 @@ function renderLazyComponent( Component, props, ); - renderElement(request, task, keyPath, Component, resolvedProps, ref); + renderElement( + request, + task, + keyPath, + Component, + resolvedProps, + ref, + owner, + stack, + ); task.componentStack = previousComponentStack; } @@ -1916,18 +2008,28 @@ function renderElement( type: any, props: Object, ref: any, + owner: null | ReactComponentInfo | ComponentStackNode, // DEV only + stack: null | Error, // DEV only ): void { if (typeof type === 'function') { if (shouldConstruct(type)) { - renderClassComponent(request, task, keyPath, type, props); + renderClassComponent(request, task, keyPath, type, props, owner, stack); return; } else { - renderFunctionComponent(request, task, keyPath, type, props); + renderFunctionComponent( + request, + task, + keyPath, + type, + props, + owner, + stack, + ); return; } } if (typeof type === 'string') { - renderHostElement(request, task, keyPath, type, props); + renderHostElement(request, task, keyPath, type, props, owner, stack); return; } @@ -1958,7 +2060,12 @@ function renderElement( } case REACT_SUSPENSE_LIST_TYPE: { const preiousComponentStack = task.componentStack; - task.componentStack = createBuiltInComponentStack(task, 'SuspenseList'); + task.componentStack = createBuiltInComponentStack( + task, + 'SuspenseList', + owner, + stack, + ); // TODO: SuspenseList should control the boundaries. const prevKeyPath = task.keyPath; task.keyPath = keyPath; @@ -1982,9 +2089,16 @@ function renderElement( enableSuspenseAvoidThisFallbackFizz && props.unstable_avoidThisFallback === true ) { - renderBackupSuspenseBoundary(request, task, keyPath, props); + renderBackupSuspenseBoundary( + request, + task, + keyPath, + props, + owner, + stack, + ); } else { - renderSuspenseBoundary(request, task, keyPath, props); + renderSuspenseBoundary(request, task, keyPath, props, owner, stack); } return; } @@ -1993,11 +2107,20 @@ function renderElement( if (typeof type === 'object' && type !== null) { switch (type.$$typeof) { case REACT_FORWARD_REF_TYPE: { - renderForwardRef(request, task, keyPath, type, props, ref); + renderForwardRef( + request, + task, + keyPath, + type, + props, + ref, + owner, + stack, + ); return; } case REACT_MEMO_TYPE: { - renderMemo(request, task, keyPath, type, props, ref); + renderMemo(request, task, keyPath, type, props, ref, owner, stack); return; } case REACT_PROVIDER_TYPE: { @@ -2034,7 +2157,16 @@ function renderElement( // Fall through } case REACT_LAZY_TYPE: { - renderLazyComponent(request, task, keyPath, type, props); + renderLazyComponent( + request, + task, + keyPath, + type, + props, + ref, + owner, + stack, + ); return; } } @@ -2114,6 +2246,8 @@ function replayElement( props: Object, ref: any, replay: ReplaySet, + owner: null | ReactComponentInfo | ComponentStackNode, // DEV only + stack: null | Error, // DEV only ): void { // We're replaying. Find the path to follow. const replayNodes = replay.nodes; @@ -2141,7 +2275,7 @@ function replayElement( const currentNode = task.node; task.replay = {nodes: childNodes, slots: childSlots, pendingTasks: 1}; try { - renderElement(request, task, keyPath, type, props, ref); + renderElement(request, task, keyPath, type, props, ref, owner, stack); if ( task.replay.pendingTasks === 1 && task.replay.nodes.length > 0 @@ -2207,6 +2341,8 @@ function replayElement( node[3], node[4] === null ? [] : node[4][2], node[4] === null ? null : node[4][3], + owner, + stack, ); } // We finished rendering this node, so now we can consume this @@ -2367,6 +2503,9 @@ function renderNodeDestructive( ref = element.ref; } + const owner = __DEV__ ? element._owner : null; + const stack = __DEV__ && enableOwnerStacks ? element._debugStack : null; + const name = getComponentNameFromType(type); const keyOrIndex = key == null ? (childIndex === -1 ? 0 : childIndex) : key; @@ -2388,6 +2527,8 @@ function renderNodeDestructive( props, ref, task.replay, + owner, + stack, ), ); return; @@ -2404,6 +2545,8 @@ function renderNodeDestructive( props, ref, task.replay, + owner, + stack, ); // No matches found for this node. We assume it's already emitted in the // prelude and skip it during the replay. @@ -2421,12 +2564,14 @@ function renderNodeDestructive( type, props, ref, + owner, + stack, ), ); return; } } - renderElement(request, task, keyPath, type, props, ref); + renderElement(request, task, keyPath, type, props, ref, owner, stack); } return; } @@ -2437,7 +2582,12 @@ function renderNodeDestructive( ); case REACT_LAZY_TYPE: { const previousComponentStack = task.componentStack; - task.componentStack = createBuiltInComponentStack(task, 'Lazy'); + task.componentStack = createBuiltInComponentStack( + task, + 'Lazy', + null, + null, + ); const lazyNode: LazyComponentType = (node: any); const payload = lazyNode._payload; const init = lazyNode._init; @@ -2503,6 +2653,8 @@ function renderNodeDestructive( task.componentStack = createBuiltInComponentStack( task, 'AsyncIterable', + null, + null, ); // Restore the thenable state before resuming. @@ -2739,7 +2891,12 @@ function warnForMissingKey(request: Request, task: Task, child: mixed): void { didWarnForKey.add(parentStackFrame); // We create a fake component stack for the child to log the stack trace from. - const stackFrame = createComponentStackFromType(task, (child: any).type); + const stackFrame = createComponentStackFromType( + task, + (child: any).type, + (child: any)._owner, + enableOwnerStacks ? (child: any)._debugStack : null, + ); task.componentStack = stackFrame; console.error( 'Each child in a list should have a unique "key" prop.' + From 5a48c1e413afa66be7acc6fa4708fc413dd75083 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sun, 30 Jun 2024 21:49:54 -0400 Subject: [PATCH 3/6] Format owner stack in captureOwnerStack --- .../src/__tests__/ReactDOMFizzServer-test.js | 179 ++++++++++++++---- .../src/ReactFizzCallUserSpace.js | 38 ++++ .../src/ReactFizzComponentStack.js | 89 ++++++++- .../react-server/src/ReactFizzOwnerStack.js | 117 ++++++++++++ packages/react-server/src/ReactFizzServer.js | 52 ++++- 5 files changed, 423 insertions(+), 52 deletions(-) create mode 100644 packages/react-server/src/ReactFizzCallUserSpace.js create mode 100644 packages/react-server/src/ReactFizzOwnerStack.js diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index 79e05e9e0d89c..1e96e9eff010f 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -1766,9 +1766,9 @@ describe('ReactDOMFizzServer', () => { // Intentionally trigger a key warning here. return (
- {children.map(t => ( - {t} - ))} + {children.map(function mapper(t) { + return {t}; + })}
); } @@ -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 { @@ -1834,21 +1838,20 @@ describe('ReactDOMFizzServer', () => { '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
.', + ? '' + : '\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(); @@ -6519,24 +6522,25 @@ describe('ReactDOMFizzServer', () => { mockError(...args.map(normalizeCodeLocInfo)); }; + function App() { + return ( + + + + + + + + ); + } + try { await act(async () => { - const {pipe} = renderToPipeableStream( - - - - - - - , - ); + const {pipe} = renderToPipeableStream(); pipe(writable); }); @@ -6545,17 +6549,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); @@ -8148,4 +8164,89 @@ 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 ; + } + function Bar() { + return ( +
+ +
+ ); + } + function Baz() { + stack = React.captureOwnerStack(); + return hi; + } + + await act(() => { + const {pipe} = renderToPipeableStream( +
+ +
, + ); + 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 ; + } + function Bar() { + return ( +
+ +
+ ); + } + function Baz() { + throw thrownError; + } + + await expect(async () => { + await act(() => { + const {pipe} = renderToPipeableStream( +
+ +
, + { + onError(error, errorInfo) { + caughtError = error; + parentStack = errorInfo.componentStack; + ownerStack = React.captureOwnerStack(); + }, + }, + ); + 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 **)', + ); + }); }); diff --git a/packages/react-server/src/ReactFizzCallUserSpace.js b/packages/react-server/src/ReactFizzCallUserSpace.js new file mode 100644 index 0000000000000..4a376607dc6df --- /dev/null +++ b/packages/react-server/src/ReactFizzCallUserSpace.js @@ -0,0 +1,38 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +import type {LazyComponent} from 'react/src/ReactLazy'; + +// These indirections exists so we can exclude its stack frame in DEV (and anything below it). +// TODO: Consider marking the whole bundle instead of these boundaries. + +/** @noinline */ +export function callComponentInDEV( + Component: (p: Props, arg: Arg) => R, + props: Props, + secondArg: Arg, +): R { + return Component(props, secondArg); +} + +interface ClassInstance { + render(): R; +} + +/** @noinline */ +export function callRenderInDEV(instance: ClassInstance): R { + return instance.render(); +} + +/** @noinline */ +export function callLazyInitInDEV(lazy: LazyComponent): any { + const payload = lazy._payload; + const init = lazy._init; + return init(payload); +} diff --git a/packages/react-server/src/ReactFizzComponentStack.js b/packages/react-server/src/ReactFizzComponentStack.js index a59c1238152ca..a16b2c5f91351 100644 --- a/packages/react-server/src/ReactFizzComponentStack.js +++ b/packages/react-server/src/ReactFizzComponentStack.js @@ -15,27 +15,31 @@ import { describeClassComponentFrame, } from 'shared/ReactComponentStackFrame'; +import {enableOwnerStacks} from 'shared/ReactFeatureFlags'; + +import {formatOwnerStack} from './ReactFizzOwnerStack'; + // DEV-only reverse linked list representing the current component stack type BuiltInComponentStackNode = { tag: 0, parent: null | ComponentStackNode, type: string, owner?: null | ReactComponentInfo | ComponentStackNode, // DEV only - stack?: null | Error, // DEV only + stack?: null | string | Error, // DEV only }; type FunctionComponentStackNode = { tag: 1, parent: null | ComponentStackNode, type: Function, owner?: null | ReactComponentInfo | ComponentStackNode, // DEV only - stack?: null | Error, // DEV only + stack?: null | string | Error, // DEV only }; type ClassComponentStackNode = { tag: 2, parent: null | ComponentStackNode, type: Function, owner?: null | ReactComponentInfo | ComponentStackNode, // DEV only - stack?: null | Error, // DEV only + stack?: null | string | Error, // DEV only }; export type ComponentStackNode = | BuiltInComponentStackNode @@ -68,3 +72,82 @@ export function getStackByComponentStackNode( return '\nError generating stack: ' + x.message + '\n' + x.stack; } } + +function describeFunctionComponentFrameWithoutLineNumber(fn: Function): string { + // We use this because we don't actually want to describe the line of the component + // but just the component name. + const name = fn ? fn.displayName || fn.name : ''; + return name ? describeBuiltInComponentFrame(name) : ''; +} + +export function getOwnerStackByComponentStackNodeInDev( + componentStack: ComponentStackNode, +): string { + if (!enableOwnerStacks || !__DEV__) { + return ''; + } + try { + let info = ''; + + // The owner stack of the current component will be where it was created, i.e. inside its owner. + // There's no actual name of the currently executing component. Instead, that is available + // on the regular stack that's currently executing. However, for built-ins there is no such + // named stack frame and it would be ignored as being internal anyway. Therefore we add + // add one extra frame just to describe the "current" built-in component by name. + // Similarly, if there is no owner at all, then there's no stack frame so we add the name + // of the root component to the stack to know which component is currently executing. + switch (componentStack.tag) { + case 0: + info += describeBuiltInComponentFrame(componentStack.type); + break; + case 1: + case 2: + if (!componentStack.owner) { + // Only if we have no other data about the callsite do we add + // the component name as the single stack frame. + info += describeFunctionComponentFrameWithoutLineNumber( + componentStack.type, + ); + } + break; + } + + let owner: void | null | ComponentStackNode | ReactComponentInfo = + componentStack; + + while (owner) { + if (typeof owner.tag === 'number') { + const node: ComponentStackNode = (owner: any); + owner = node.owner; + let debugStack = node.stack; + // If we don't actually print the stack if there is no owner of this JSX element. + // In a real app it's typically not useful since the root app is always controlled + // by the framework. These also tend to have noisy stacks because they're not rooted + // in a React render but in some imperative bootstrapping code. It could be useful + // if the element was created in module scope. E.g. hoisted. We could add a a single + // stack frame for context for example but it doesn't say much if that's a wrapper. + if (owner && debugStack) { + if (typeof debugStack !== 'string') { + // Stash the formatted stack so that we can avoid redoing the filtering. + node.stack = debugStack = formatOwnerStack(debugStack); + } + if (debugStack !== '') { + info += '\n' + debugStack; + } + } + } else if (typeof owner.stack === 'string') { + // Server Component + if (owner.stack !== '') { + info += '\n' + owner.stack; + } + const componentInfo: ReactComponentInfo = (owner: any); + owner = componentInfo.owner; + } else { + break; + } + } + return info; + } catch (x) { + return '\nError generating stack: ' + x.message + '\n' + x.stack; + } +} diff --git a/packages/react-server/src/ReactFizzOwnerStack.js b/packages/react-server/src/ReactFizzOwnerStack.js new file mode 100644 index 0000000000000..8d6b5d94fd0db --- /dev/null +++ b/packages/react-server/src/ReactFizzOwnerStack.js @@ -0,0 +1,117 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +import {REACT_LAZY_TYPE} from 'shared/ReactSymbols'; + +import { + callLazyInitInDEV, + callComponentInDEV, + callRenderInDEV, +} from './ReactFizzCallUserSpace'; + +// TODO: Make this configurable on the root. +const externalRegExp = /\/node\_modules\/|\(\\)/; + +let callComponentFrame: null | string = null; +let callIteratorFrame: null | string = null; +let callLazyInitFrame: null | string = null; + +function isNotExternal(stackFrame: string): boolean { + return !externalRegExp.test(stackFrame); +} + +function initCallComponentFrame(): string { + // Extract the stack frame of the callComponentInDEV function. + const error = callComponentInDEV(Error, 'react-stack-top-frame', {}); + const stack = error.stack; + const startIdx = stack.startsWith('Error: react-stack-top-frame\n') ? 29 : 0; + const endIdx = stack.indexOf('\n', startIdx); + if (endIdx === -1) { + return stack.slice(startIdx); + } + return stack.slice(startIdx, endIdx); +} + +function initCallRenderFrame(): string { + // Extract the stack frame of the callRenderInDEV function. + try { + (callRenderInDEV: any)({render: null}); + return ''; + } catch (error) { + const stack = error.stack; + const startIdx = stack.startsWith('TypeError: ') + ? stack.indexOf('\n') + 1 + : 0; + const endIdx = stack.indexOf('\n', startIdx); + if (endIdx === -1) { + return stack.slice(startIdx); + } + return stack.slice(startIdx, endIdx); + } +} + +function initCallLazyInitFrame(): string { + // Extract the stack frame of the callLazyInitInDEV function. + const error = callLazyInitInDEV({ + $$typeof: REACT_LAZY_TYPE, + _init: Error, + _payload: 'react-stack-top-frame', + }); + const stack = error.stack; + const startIdx = stack.startsWith('Error: react-stack-top-frame\n') ? 29 : 0; + const endIdx = stack.indexOf('\n', startIdx); + if (endIdx === -1) { + return stack.slice(startIdx); + } + return stack.slice(startIdx, endIdx); +} + +function filterDebugStack(error: Error): string { + // Since stacks can be quite large and we pass a lot of them, we filter them out eagerly + // to save bandwidth even in DEV. We'll also replay these stacks on the client so by + // stripping them early we avoid that overhead. Otherwise we'd normally just rely on + // the DevTools or framework's ignore lists to filter them out. + let stack = error.stack; + if (stack.startsWith('Error: react-stack-top-frame\n')) { + // V8's default formatting prefixes with the error message which we + // don't want/need. + stack = stack.slice(29); + } + const frames = stack.split('\n').slice(1); + if (callComponentFrame === null) { + callComponentFrame = initCallComponentFrame(); + } + let lastFrameIdx = frames.indexOf(callComponentFrame); + if (lastFrameIdx === -1) { + if (callLazyInitFrame === null) { + callLazyInitFrame = initCallLazyInitFrame(); + } + lastFrameIdx = frames.indexOf(callLazyInitFrame); + if (lastFrameIdx === -1) { + if (callIteratorFrame === null) { + callIteratorFrame = initCallRenderFrame(); + } + lastFrameIdx = frames.indexOf(callIteratorFrame); + } + } + if (lastFrameIdx !== -1) { + // Cut off everything after our "callComponent" slot since it'll be Fiber internals. + frames.length = lastFrameIdx; + } else { + // We didn't find any internal callsite out to user space. + // This means that this was called outside an owner or the owner is fully internal. + // To keep things light we exclude the entire trace in this case. + return ''; + } + return frames.filter(isNotExternal).join('\n'); +} + +export function formatOwnerStack(ownerStackTrace: Error): string { + return filterDebugStack(ownerStackTrace); +} diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index b89255a6ffa79..ef1b277cf9ce4 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -114,9 +114,17 @@ import { getActionStateMatchingIndex, } from './ReactFizzHooks'; import {DefaultAsyncDispatcher} from './ReactFizzAsyncDispatcher'; -import {getStackByComponentStackNode} from './ReactFizzComponentStack'; +import { + getStackByComponentStackNode, + getOwnerStackByComponentStackNodeInDev, +} from './ReactFizzComponentStack'; import {emptyTreeContext, pushTreeContext} from './ReactFizzTreeContext'; import {currentTaskInDEV, setCurrentTaskInDEV} from './ReactFizzCurrentTask'; +import { + callLazyInitInDEV, + callComponentInDEV, + callRenderInDEV, +} from './ReactFizzCallUserSpace'; import { getIteratorFn, @@ -797,7 +805,11 @@ function getCurrentStackInDEV(): string { if (currentTaskInDEV === null || currentTaskInDEV.componentStack === null) { return ''; } - // TODO: Support owner based stacks for logs during SSR. + if (enableOwnerStacks) { + return getOwnerStackByComponentStackNodeInDev( + currentTaskInDEV.componentStack, + ); + } return getStackByComponentStackNode(currentTaskInDEV.componentStack); } return ''; @@ -1454,7 +1466,12 @@ function renderWithHooks( componentIdentity, prevThenableState, ); - const result = Component(props, secondArg); + let result; + if (__DEV__) { + result = callComponentInDEV(Component, props, secondArg); + } else { + result = Component(props, secondArg); + } return finishHooks(Component, props, result, secondArg); } @@ -1466,7 +1483,12 @@ function finishClassComponent( Component: any, props: any, ): ReactNodeList { - const nextChildren = instance.render(); + let nextChildren; + if (__DEV__) { + nextChildren = callRenderInDEV(instance); + } else { + nextChildren = instance.render(); + } if (__DEV__) { if (instance.props !== props) { @@ -1961,9 +1983,14 @@ function renderLazyComponent( ): void { const previousComponentStack = task.componentStack; task.componentStack = createBuiltInComponentStack(task, 'Lazy', owner, stack); - const payload = lazyComponent._payload; - const init = lazyComponent._init; - const Component = init(payload); + let Component; + if (__DEV__) { + Component = callLazyInitInDEV(lazyComponent); + } else { + const payload = lazyComponent._payload; + const init = lazyComponent._init; + Component = init(payload); + } const resolvedProps = resolveDefaultPropsOnNonClassComponent( Component, props, @@ -2589,9 +2616,14 @@ function renderNodeDestructive( null, ); const lazyNode: LazyComponentType = (node: any); - const payload = lazyNode._payload; - const init = lazyNode._init; - const resolvedNode = init(payload); + let resolvedNode; + if (__DEV__) { + resolvedNode = callLazyInitInDEV(lazyNode); + } else { + const payload = lazyNode._payload; + const init = lazyNode._init; + resolvedNode = init(payload); + } // We restore the stack before rendering the resolved node because once the Lazy // has resolved any future errors From c66bb4a0f89e8a7155a422dd81c6cc81d5abe22e Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Mon, 1 Jul 2024 09:40:32 -0400 Subject: [PATCH 4/6] Fix test I don't know why this doesn't work with the gating mechanism. --- packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index 1e96e9eff010f..12f218772cab1 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -8229,7 +8229,9 @@ describe('ReactDOMFizzServer', () => { onError(error, errorInfo) { caughtError = error; parentStack = errorInfo.componentStack; - ownerStack = React.captureOwnerStack(); + ownerStack = React.captureOwnerStack + ? React.captureOwnerStack() + : null; }, }, ); From 22384979b8b40bc71a7d0dfdad64ff566f6d629f Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Mon, 1 Jul 2024 09:50:27 -0400 Subject: [PATCH 5/6] Update another test --- .../__tests__/ReactServerRendering-test.js | 65 ++++++++++++------- 1 file changed, 41 insertions(+), 24 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactServerRendering-test.js b/packages/react-dom/src/__tests__/ReactServerRendering-test.js index a41488600655d..065f7cadd7f85 100644 --- a/packages/react-dom/src/__tests__/ReactServerRendering-test.js +++ b/packages/react-dom/src/__tests__/ReactServerRendering-test.js @@ -835,21 +835,30 @@ describe('ReactDOMServer', () => { expect(() => ReactDOMServer.renderToString()).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 **)'), ]); }); @@ -885,9 +894,11 @@ describe('ReactDOMServer', () => { expect(() => ReactDOMServer.renderToString()).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 **)', @@ -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 **)'), ]); }); From ec4b3fc6baa0692eb294af2521277ca0a69b4d70 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Mon, 1 Jul 2024 10:18:33 -0400 Subject: [PATCH 6/6] Add owner based debug information to key warning Now that we have owners in Fizz we can add the missing context in the error message that the client and jsx had. I don't think this context is very useful now that we have good stacks but it is at least parity. --- .../src/__tests__/ReactDOMFizzServer-test.js | 4 +- packages/react-server/src/ReactFizzServer.js | 39 ++++++++++++++++++- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index 12f218772cab1..62e0a17af06a4 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -1837,9 +1837,7 @@ 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) - ? '' - : '\n\nCheck the render method of `B`.', + '\n\nCheck the render method of `B`.', '', '\n' + (gate(flags => flags.enableOwnerStacks) diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index ef1b277cf9ce4..6c21a1828da58 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -2922,6 +2922,41 @@ function warnForMissingKey(request: Request, task: Task, child: mixed): void { } didWarnForKey.add(parentStackFrame); + const componentName = getComponentNameFromType(child.type); + const childOwner = child._owner; + const parentOwner = parentStackFrame.owner; + + let currentComponentErrorInfo = ''; + if (parentOwner && typeof parentOwner.tag === 'number') { + const name = getComponentNameFromType((parentOwner: any).type); + if (name) { + currentComponentErrorInfo = + '\n\nCheck the render method of `' + name + '`.'; + } + } + if (!currentComponentErrorInfo) { + if (componentName) { + currentComponentErrorInfo = `\n\nCheck the top-level render call using <${componentName}>.`; + } + } + + // Usually the current owner is the offender, but if it accepts children as a + // property, it may be the creator of the child that's responsible for + // assigning it a key. + let childOwnerAppendix = ''; + if (childOwner != null && parentOwner !== childOwner) { + let ownerName = null; + if (typeof childOwner.tag === 'number') { + ownerName = getComponentNameFromType((childOwner: any).type); + } else if (typeof childOwner.name === 'string') { + ownerName = childOwner.name; + } + if (ownerName) { + // Give the component that originally created this child. + childOwnerAppendix = ` It was passed a child from ${ownerName}.`; + } + } + // We create a fake component stack for the child to log the stack trace from. const stackFrame = createComponentStackFromType( task, @@ -2933,8 +2968,8 @@ function warnForMissingKey(request: Request, task: Task, child: mixed): void { console.error( 'Each child in a list should have a unique "key" prop.' + '%s%s See https://react.dev/link/warning-keys for more information.', - '', - '', + currentComponentErrorInfo, + childOwnerAppendix, ); task.componentStack = stackFrame.parent; }