From 85081ddf7ef7771dca8dbc17dc9146956b531e11 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sun, 7 Jul 2024 15:36:46 -0400 Subject: [PATCH 1/6] Add reminder that Lazy stack frames will likely become Promises later --- .../src/backend/DevToolsFiberComponentStack.js | 1 + packages/react-reconciler/src/ReactFiberComponentStack.js | 1 + packages/react-server/src/ReactFizzServer.js | 2 +- 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/react-devtools-shared/src/backend/DevToolsFiberComponentStack.js b/packages/react-devtools-shared/src/backend/DevToolsFiberComponentStack.js index 73887c16d8584..1e64b3e15fec3 100644 --- a/packages/react-devtools-shared/src/backend/DevToolsFiberComponentStack.js +++ b/packages/react-devtools-shared/src/backend/DevToolsFiberComponentStack.js @@ -47,6 +47,7 @@ export function describeFiber( case HostComponent: return describeBuiltInComponentFrame(workInProgress.type); case LazyComponent: + // TODO: When we support Thenables as component types we should rename this. return describeBuiltInComponentFrame('Lazy'); case SuspenseComponent: return describeBuiltInComponentFrame('Suspense'); diff --git a/packages/react-reconciler/src/ReactFiberComponentStack.js b/packages/react-reconciler/src/ReactFiberComponentStack.js index 18f65530ea5b8..a06411acadd3e 100644 --- a/packages/react-reconciler/src/ReactFiberComponentStack.js +++ b/packages/react-reconciler/src/ReactFiberComponentStack.js @@ -39,6 +39,7 @@ function describeFiber(fiber: Fiber): string { case HostComponent: return describeBuiltInComponentFrame(fiber.type); case LazyComponent: + // TODO: When we support Thenables as component types we should rename this. return describeBuiltInComponentFrame('Lazy'); case SuspenseComponent: return describeBuiltInComponentFrame('Suspense'); diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index cf44e055ca675..9a778d90841a6 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -2078,7 +2078,7 @@ function renderLazyComponent( stack: null | Error, // DEV only ): void { const previousComponentStack = task.componentStack; - // TODO: Do we really need this stack frame? We don't on the client. + // TODO: When we support Thenables as component types we should rename this. task.componentStack = createBuiltInComponentStack(task, 'Lazy', owner, stack); let Component; if (__DEV__) { From cf1b76708a1d797b057e3b778ac15807d55441e0 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sun, 7 Jul 2024 17:23:27 -0400 Subject: [PATCH 2/6] Test component stacks --- .../src/__tests__/ReactDOMFizzServer-test.js | 57 ++++++++++----- .../src/__tests__/ReactHTMLServer-test.js | 4 +- .../src/__tests__/ReactFlightDOMEdge-test.js | 70 +++++++++++++++++++ 3 files changed, 109 insertions(+), 22 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index 62e0a17af06a4..d534df76a55d8 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -700,17 +700,39 @@ describe('ReactDOMFizzServer', () => { it('should client render a boundary if a lazy component rejects', async () => { let rejectComponent; + const promise = new Promise((resolve, reject) => { + rejectComponent = reject; + }); const LazyComponent = React.lazy(() => { - return new Promise((resolve, reject) => { - rejectComponent = reject; - }); + return promise; + }); + + const LazyLazy = React.lazy(async () => { + return { + default: LazyComponent, + }; + }); + + function Wrapper({children}) { + return children; + } + const LazyWrapper = React.lazy(() => { + return { + then(callback) { + callback({ + default: Wrapper, + }); + }, + }; }); function App({isClient}) { return (
}> - {isClient ? : } + + {isClient ? : } +
); @@ -744,6 +766,7 @@ describe('ReactDOMFizzServer', () => { }); pipe(writable); }); + expect(loggedErrors).toEqual([]); expect(bootstrapped).toBe(true); @@ -772,7 +795,7 @@ describe('ReactDOMFizzServer', () => { 'Switched to client rendering because the server rendering errored:\n\n' + theError.message, expectedDigest, - componentStack(['Lazy', 'Suspense', 'div', 'App']), + componentStack(['Lazy', 'Wrapper', 'Suspense', 'div', 'App']), ], ], [ @@ -852,13 +875,9 @@ describe('ReactDOMFizzServer', () => { } await act(() => { - const {pipe} = renderToPipeableStream( - , - - { - onError, - }, - ); + const {pipe} = renderToPipeableStream(, { + onError, + }); pipe(writable); }); expect(loggedErrors).toEqual([]); @@ -896,7 +915,7 @@ describe('ReactDOMFizzServer', () => { 'Switched to client rendering because the server rendering errored:\n\n' + theError.message, expectedDigest, - componentStack(['Lazy', 'Suspense', 'div', 'App']), + componentStack(['Suspense', 'div', 'App']), ], ], [ @@ -1395,13 +1414,13 @@ describe('ReactDOMFizzServer', () => { 'The render was aborted by the server without a reason.', expectedDigest, // We get the stack of the task when it was aborted which is why we see `h1` - componentStack(['h1', 'Suspense', 'div', 'App']), + componentStack(['AsyncText', 'h1', 'Suspense', 'div', 'App']), ], [ 'Switched to client rendering because the server rendering aborted due to:\n\n' + 'The render was aborted by the server without a reason.', expectedDigest, - componentStack(['Suspense', 'main', 'div', 'App']), + componentStack(['AsyncText', 'Suspense', 'main', 'div', 'App']), ], ], [ @@ -3523,13 +3542,13 @@ describe('ReactDOMFizzServer', () => { 'Switched to client rendering because the server rendering aborted due to:\n\n' + 'foobar', 'a digest', - componentStack(['Suspense', 'p', 'div', 'App']), + componentStack(['AsyncText', 'Suspense', 'p', 'div', 'App']), ], [ 'Switched to client rendering because the server rendering aborted due to:\n\n' + 'foobar', 'a digest', - componentStack(['Suspense', 'span', 'div', 'App']), + componentStack(['AsyncText', 'Suspense', 'span', 'div', 'App']), ], ], [ @@ -3606,13 +3625,13 @@ describe('ReactDOMFizzServer', () => { 'Switched to client rendering because the server rendering aborted due to:\n\n' + 'uh oh', 'a digest', - componentStack(['Suspense', 'p', 'div', 'App']), + componentStack(['AsyncText', 'Suspense', 'p', 'div', 'App']), ], [ 'Switched to client rendering because the server rendering aborted due to:\n\n' + 'uh oh', 'a digest', - componentStack(['Suspense', 'span', 'div', 'App']), + componentStack(['AsyncText', 'Suspense', 'span', 'div', 'App']), ], ], [ diff --git a/packages/react-html/src/__tests__/ReactHTMLServer-test.js b/packages/react-html/src/__tests__/ReactHTMLServer-test.js index 01c5873b569e9..98678083d54d5 100644 --- a/packages/react-html/src/__tests__/ReactHTMLServer-test.js +++ b/packages/react-html/src/__tests__/ReactHTMLServer-test.js @@ -250,9 +250,7 @@ if (!__EXPERIMENTAL__) { '\n in Bar (at **)' + '\n in Foo (at **)' + '\n in div (at **)' - : '\n in Lazy (at **)' + - '\n in div (at **)' + - '\n in div (at **)', + : '\n in div (at **)' + '\n in div (at **)', ); expect(normalizeCodeLocInfo(caughtErrors[0].ownerStack)).toBe( __DEV__ && gate(flags => flags.enableOwnerStacks) diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js index 4997184078572..9d4a123ac3b75 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js @@ -930,4 +930,74 @@ describe('ReactFlightDOMEdge', () => { '\n in Bar (at **)' + '\n in Foo (at **)', ); }); + + it('supports server components in ssr component stacks', async () => { + let reject; + const promise = new Promise((_, r) => (reject = r)); + async function Erroring() { + await promise; + return 'should not render'; + } + + const model = { + root: ReactServer.createElement(Erroring), + }; + + const stream = ReactServerDOMServer.renderToReadableStream( + model, + webpackMap, + { + onError() {}, + }, + ); + + const rootModel = await ReactServerDOMClient.createFromReadableStream( + stream, + { + ssrManifest: { + moduleMap: null, + moduleLoading: null, + }, + }, + ); + + const errors = []; + const result = ReactDOMServer.renderToReadableStream( +
{rootModel.root}
, + { + onError(error, {componentStack}) { + errors.push({ + error, + componentStack: normalizeCodeLocInfo(componentStack), + }); + }, + }, + ); + + const theError = new Error('my error'); + reject(theError); + + const expectedMessage = __DEV__ + ? 'my error' + : 'An error occurred in the Server Components render. The specific message is omitted in production builds to avoid leaking sensitive details. A digest property is included on this error instance which may provide additional details about the nature of the error.'; + + try { + await result; + } catch (x) { + expect(x).toEqual( + expect.objectContaining({ + message: expectedMessage, + }), + ); + } + + expect(errors).toEqual([ + { + error: expect.objectContaining({ + message: expectedMessage, + }), + componentStack: (__DEV__ ? '\n in Erroring' : '') + '\n in div', + }, + ]); + }); }); From acd39bae69d7681218178de8780a7ea9453d093f Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sun, 7 Jul 2024 19:08:25 -0400 Subject: [PATCH 3/6] Don't readd component stacks in the retry phase --- packages/react-server/src/ReactFizzServer.js | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index 9a778d90841a6..58ed1e9b3411b 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -2604,6 +2604,13 @@ function renderNodeDestructive( task.node = node; task.childIndex = childIndex; + retryNode(request, task); +} + +function retryNode(request: Request, task: Task): void { + const node = task.node; + const childIndex = task.childIndex; + if (node === null) { return; } @@ -4196,7 +4203,7 @@ function retryRenderTask( // We call the destructive form that mutates this task. That way if something // suspends again, we can reuse the same task instead of spawning a new one. - renderNodeDestructive(request, task, task.node, task.childIndex); + retryNode(request, task); pushSegmentFinale( segment.chunks, request.renderState, @@ -4299,8 +4306,12 @@ function retryReplayTask(request: Request, task: ReplayTask): void { try { // We call the destructive form that mutates this task. That way if something // suspends again, we can reuse the same task instead of spawning a new one. - - renderNodeDestructive(request, task, task.node, task.childIndex); + if (typeof task.replay.slots === 'number') { + const resumeSegmentID = task.replay.slots; + resumeNode(request, task, resumeSegmentID, task.node, task.childIndex); + } else { + retryNode(request, task); + } if (task.replay.pendingTasks === 1 && task.replay.nodes.length > 0) { throw new Error( From b76201d43d28c64205caba8151060ba18eaeab63 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 9 Jul 2024 13:51:19 -0400 Subject: [PATCH 4/6] Perform type checking of stack frames lazily Now that checking the type of an element is no longer free it makes more sense to do those checks lazily when we generate a stack and just store them as a generic "type". --- .../src/ReactFizzComponentStack.js | 199 ++++---- packages/react-server/src/ReactFizzServer.js | 454 +++++------------- 2 files changed, 219 insertions(+), 434 deletions(-) diff --git a/packages/react-server/src/ReactFizzComponentStack.js b/packages/react-server/src/ReactFizzComponentStack.js index 6e3a31b2841b1..b280983359f79 100644 --- a/packages/react-server/src/ReactFizzComponentStack.js +++ b/packages/react-server/src/ReactFizzComponentStack.js @@ -8,52 +8,96 @@ */ import type {ReactComponentInfo} from 'shared/ReactTypes'; +import type {LazyComponent} from 'react/src/ReactLazy'; import { describeBuiltInComponentFrame, describeFunctionComponentFrame, describeClassComponentFrame, + describeDebugInfoFrame, } from 'shared/ReactComponentStackFrame'; +import { + REACT_FORWARD_REF_TYPE, + REACT_MEMO_TYPE, + REACT_LAZY_TYPE, + REACT_SUSPENSE_LIST_TYPE, + REACT_SUSPENSE_TYPE, +} from 'shared/ReactSymbols'; + 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 | string | Error, // DEV only -}; -type FunctionComponentStackNode = { - tag: 1, - parent: null | ComponentStackNode, - type: Function, - owner?: null | ReactComponentInfo | ComponentStackNode, // DEV only - stack?: null | string | Error, // DEV only -}; -type ClassComponentStackNode = { - tag: 2, +export type ComponentStackNode = { parent: null | ComponentStackNode, - type: Function, + type: + | symbol + | string + | Function + | LazyComponent + | ReactComponentInfo, owner?: null | ReactComponentInfo | ComponentStackNode, // DEV only stack?: null | string | Error, // DEV only }; -type ServerComponentStackNode = { - // DEV only - tag: 3, - parent: null | ComponentStackNode, - type: string, // name + env - owner?: null | ReactComponentInfo | ComponentStackNode, // DEV only - stack?: null | string | Error, // DEV only -}; -export type ComponentStackNode = - | BuiltInComponentStackNode - | FunctionComponentStackNode - | ClassComponentStackNode - | ServerComponentStackNode; + +function shouldConstruct(Component: any) { + return Component.prototype && Component.prototype.isReactComponent; +} + +function describeComponentStackByType( + type: + | symbol + | string + | Function + | LazyComponent + | ReactComponentInfo, +): string { + if (typeof type === 'string') { + return describeBuiltInComponentFrame(type); + } + if (typeof type === 'function') { + if (shouldConstruct(type)) { + return describeClassComponentFrame(type); + } else { + return describeFunctionComponentFrame(type); + } + } + if (typeof type === 'object' && type !== null) { + switch (type.$$typeof) { + case REACT_FORWARD_REF_TYPE: { + return describeFunctionComponentFrame((type: any).render); + } + case REACT_MEMO_TYPE: { + return describeFunctionComponentFrame((type: any).type); + } + case REACT_LAZY_TYPE: { + const lazyComponent: LazyComponent = (type: any); + const payload = lazyComponent._payload; + const init = lazyComponent._init; + try { + type = init(payload); + } catch (x) { + // TODO: When we support Thenables as component types we should rename this. + return describeBuiltInComponentFrame('Lazy'); + } + return describeComponentStackByType(type); + } + } + if (typeof type.name === 'string') { + return describeDebugInfoFrame(type.name, type.env); + } + } + switch (type) { + case REACT_SUSPENSE_LIST_TYPE: { + return describeBuiltInComponentFrame('SuspenseList'); + } + case REACT_SUSPENSE_TYPE: { + return describeBuiltInComponentFrame('Suspense'); + } + } + return ''; +} export function getStackByComponentStackNode( componentStack: ComponentStackNode, @@ -62,22 +106,7 @@ export function getStackByComponentStackNode( let info = ''; let node: ComponentStackNode = componentStack; do { - switch (node.tag) { - case 0: - info += describeBuiltInComponentFrame(node.type); - break; - case 1: - info += describeFunctionComponentFrame(node.type); - break; - case 2: - info += describeClassComponentFrame(node.type); - break; - case 3: - if (__DEV__) { - info += describeBuiltInComponentFrame(node.type); - break; - } - } + info += describeComponentStackByType(node.type); // $FlowFixMe[incompatible-type] we bail out when we get a null node = node.parent; } while (node); @@ -110,59 +139,41 @@ export function getOwnerStackByComponentStackNodeInDev( // 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; - case 3: - if (!componentStack.owner) { - info += describeBuiltInComponentFrame(componentStack.type); - } - break; + if (typeof componentStack.type === 'string') { + info += describeBuiltInComponentFrame(componentStack.type); + } else if (typeof componentStack.type === 'function') { + 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, + ); + } + } else { + if (!componentStack.owner) { + info += describeComponentStackByType(componentStack.type); + } } 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 - const ownerStack: string = owner.stack; - owner = owner.owner; - if (owner && ownerStack !== '') { - info += '\n' + ownerStack; - } - } else { - break; + let debugStack: void | null | string | Error = owner.stack; + if (typeof debugStack !== 'string' && debugStack != null) { + // Stash the formatted stack so that we can avoid redoing the filtering. + // $FlowFixMe[cannot-write]: This has been refined to a ComponentStackNode. + owner.stack = debugStack = formatOwnerStack(debugStack); + } + owner = owner.owner; + // 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) { + info += '\n' + debugStack; } } return info; diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index 58ed1e9b3411b..afdc9efbb282c 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -472,6 +472,7 @@ function RequestInstance( emptyContextObject, null, ); + pushComponentStack(rootTask); pingedTasks.push(rootTask); } @@ -615,6 +616,7 @@ export function resumeRequest( emptyContextObject, null, ); + pushComponentStack(rootTask); pingedTasks.push(rootTask); return request; } @@ -642,6 +644,7 @@ export function resumeRequest( emptyContextObject, null, ); + pushComponentStack(rootTask); pingedTasks.push(rootTask); return request; } @@ -837,69 +840,6 @@ function getStackFromNode(stackNode: ComponentStackNode): string { return getStackByComponentStackNode(stackNode); } -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, - type, - }; -} -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, - type, - }; -} -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, - type, - }; -} function pushServerComponentStack( task: Task, debugInfo: void | null | ReactDebugInfo, @@ -921,15 +861,9 @@ function pushServerComponentStack( if (enableOwnerStacks && componentInfo.stack === undefined) { continue; } - let name = componentInfo.name; - const env = componentInfo.env; - if (env) { - name += ' [' + env + ']'; - } task.componentStack = { - tag: 3, parent: task.componentStack, - type: name, + type: componentInfo, owner: componentInfo.owner, stack: componentInfo.stack, }; @@ -940,19 +874,70 @@ function pushServerComponentStack( } } +function pushComponentStack(task: Task): void { + const node = task.node; + // Create the Component Stack frame for the element we're about to try. + // It's unfortunate that we need to do this refinement twice. Once for + // the stack frame and then once again while actually + if (typeof node === 'object' && node !== null) { + switch ((node: any).$$typeof) { + case REACT_ELEMENT_TYPE: { + const element: any = node; + const type = element.type; + const owner = __DEV__ ? element._owner : null; + const stack = __DEV__ && enableOwnerStacks ? element._debugStack : null; + if (__DEV__) { + pushServerComponentStack(task, element._debugInfo); + if (enableOwnerStacks) { + task.debugTask = element._debugTask; + } + } + task.componentStack = createComponentStackFromType( + task.componentStack, + type, + owner, + stack, + ); + break; + } + case REACT_LAZY_TYPE: { + if (__DEV__) { + const lazyNode: LazyComponentType = (node: any); + pushServerComponentStack(task, lazyNode._debugInfo); + } + break; + } + default: { + if (__DEV__) { + const maybeUsable: Object = node; + if (typeof maybeUsable.then === 'function') { + const thenable: Thenable = (maybeUsable: any); + pushServerComponentStack(task, thenable._debugInfo); + } + } + } + } + } +} + function createComponentStackFromType( - task: Task, - type: Function | string, + parent: null | ComponentStackNode, + type: Function | string | symbol, owner: null | ReactComponentInfo | ComponentStackNode, // DEV only stack: null | Error, // DEV only ): ComponentStackNode { - if (typeof type === 'string') { - return createBuiltInComponentStack(task, type, owner, stack); - } - if (shouldConstruct(type)) { - return createClassComponentStack(task, type, owner, stack); + if (__DEV__) { + return { + parent, + type, + owner, + stack, + }; } - return createFunctionComponentStack(task, type, owner, stack); + return { + parent, + type, + }; } type ThrownInfo = { @@ -1088,8 +1073,6 @@ 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 @@ -1108,12 +1091,6 @@ function renderSuspenseBoundary( // $FlowFixMe: Refined. const task: RenderTask = someTask; - 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', owner, stack)); - const prevKeyPath = task.keyPath; const parentBoundary = task.blockedBoundary; const parentHoistableState = task.hoistableState; @@ -1189,9 +1166,6 @@ function renderSuspenseBoundary( // Therefore we won't need the fallback. We early return so that we don't have to create // the fallback. newBoundary.status = COMPLETED; - - // We are returning early so we need to restore the - task.componentStack = previousComponentStack; return; } } catch (error: mixed) { @@ -1234,7 +1208,6 @@ function renderSuspenseBoundary( task.hoistableState = parentHoistableState; task.blockedSegment = parentSegment; task.keyPath = prevKeyPath; - task.componentStack = previousComponentStack; } const fallbackKeyPath = [keyPath[0], 'Suspense Fallback', keyPath[2]]; @@ -1274,13 +1247,12 @@ function renderSuspenseBoundary( task.formatContext, task.context, task.treeContext, - // This stack should be the Suspense boundary stack because while the fallback is actually a child segment - // of the parent boundary from a component standpoint the fallback is a child of the Suspense boundary itself - suspenseComponentStack, + task.componentStack, true, !disableLegacyContext ? task.legacyContext : emptyContextObject, __DEV__ && enableOwnerStacks ? task.debugTask : null, ); + pushComponentStack(suspendedFallbackTask); // TODO: This should be queued at a separate lower priority queue so that we only work // on preparing fallbacks if we don't have any more main content to task on. request.pingedTasks.push(suspendedFallbackTask); @@ -1296,15 +1268,7 @@ 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', owner, stack)); - const prevKeyPath = task.keyPath; const previousReplaySet: ReplaySet = task.replay; @@ -1400,7 +1364,6 @@ function replaySuspenseBoundary( task.hoistableState = parentHoistableState; task.replay = previousReplaySet; task.keyPath = prevKeyPath; - task.componentStack = previousComponentStack; } const fallbackKeyPath = [keyPath[0], 'Suspense Fallback', keyPath[2]]; @@ -1425,13 +1388,12 @@ function replaySuspenseBoundary( task.formatContext, task.context, task.treeContext, - // This stack should be the Suspense boundary stack because while the fallback is actually a child segment - // of the parent boundary from a component standpoint the fallback is a child of the Suspense boundary itself - suspenseComponentStack, + task.componentStack, true, !disableLegacyContext ? task.legacyContext : emptyContextObject, __DEV__ && enableOwnerStacks ? task.debugTask : null, ); + pushComponentStack(suspendedFallbackTask); // TODO: This should be queued at a separate lower priority queue so that we only work // on preparing fallbacks if we don't have any more main content to task on. request.pingedTasks.push(suspendedFallbackTask); @@ -1442,17 +1404,7 @@ 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', - owner, - stack, - ); - const content = props.children; const segment = task.blockedSegment; const prevKeyPath = task.keyPath; @@ -1467,7 +1419,6 @@ function renderBackupSuspenseBoundary( pushEndCompletedSuspenseBoundary(segment.chunks); } task.keyPath = prevKeyPath; - task.componentStack = previousComponentStack; } function renderHostElement( @@ -1476,11 +1427,7 @@ 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, owner, stack); const segment = task.blockedSegment; if (segment === null) { // Replay @@ -1534,7 +1481,6 @@ function renderHostElement( ); segment.lastPushedText = false; } - task.componentStack = previousComponentStack; } function shouldConstruct(Component: any) { @@ -1670,17 +1616,8 @@ 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, - owner, - stack, - ); const maskedContext = !disableLegacyContext ? getMaskedContext(Component, task.legacyContext) : undefined; @@ -1698,7 +1635,6 @@ function renderClassComponent( Component, resolvedProps, ); - task.componentStack = previousComponentStack; } const didWarnAboutBadClass: {[string]: boolean} = {}; @@ -1715,21 +1651,11 @@ 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, - owner, - stack, - ); - if (__DEV__) { if ( Component.prototype && @@ -1782,7 +1708,6 @@ function renderFunctionComponent( actionStateCount, actionStateMatchingIndex, ); - task.componentStack = previousComponentStack; } function finishFunctionComponent( @@ -1931,17 +1856,7 @@ 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, - owner, - stack, - ); - let propsWithoutRef; if (enableRefAsProp && 'ref' in props) { // `ref` is just a prop now, but `forwardRef` expects it to not appear in @@ -1980,7 +1895,6 @@ function renderForwardRef( actionStateCount, actionStateMatchingIndex, ); - task.componentStack = previousComponentStack; } function renderMemo( @@ -1990,24 +1904,13 @@ 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, - owner, - stack, - ); + renderElement(request, task, keyPath, innerType, resolvedProps, ref); } function renderContextConsumer( @@ -2074,12 +1977,7 @@ function renderLazyComponent( lazyComponent: LazyComponentType, props: Object, ref: any, - owner: null | ReactComponentInfo | ComponentStackNode, // DEV only - stack: null | Error, // DEV only ): void { - const previousComponentStack = task.componentStack; - // TODO: When we support Thenables as component types we should rename this. - task.componentStack = createBuiltInComponentStack(task, 'Lazy', owner, stack); let Component; if (__DEV__) { Component = callLazyInitInDEV(lazyComponent); @@ -2092,17 +1990,7 @@ function renderLazyComponent( Component, props, ); - renderElement( - request, - task, - keyPath, - Component, - resolvedProps, - ref, - owner, - stack, - ); - task.componentStack = previousComponentStack; + renderElement(request, task, keyPath, Component, resolvedProps, ref); } function renderOffscreen( @@ -2132,28 +2020,18 @@ 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, owner, stack); + renderClassComponent(request, task, keyPath, type, props); return; } else { - renderFunctionComponent( - request, - task, - keyPath, - type, - props, - owner, - stack, - ); + renderFunctionComponent(request, task, keyPath, type, props); return; } } if (typeof type === 'string') { - renderHostElement(request, task, keyPath, type, props, owner, stack); + renderHostElement(request, task, keyPath, type, props); return; } @@ -2183,19 +2061,11 @@ function renderElement( return; } case REACT_SUSPENSE_LIST_TYPE: { - const preiousComponentStack = task.componentStack; - task.componentStack = createBuiltInComponentStack( - task, - 'SuspenseList', - owner, - stack, - ); // TODO: SuspenseList should control the boundaries. const prevKeyPath = task.keyPath; task.keyPath = keyPath; renderNodeDestructive(request, task, props.children, -1); task.keyPath = prevKeyPath; - task.componentStack = preiousComponentStack; return; } case REACT_SCOPE_TYPE: { @@ -2213,16 +2083,9 @@ function renderElement( enableSuspenseAvoidThisFallbackFizz && props.unstable_avoidThisFallback === true ) { - renderBackupSuspenseBoundary( - request, - task, - keyPath, - props, - owner, - stack, - ); + renderBackupSuspenseBoundary(request, task, keyPath, props); } else { - renderSuspenseBoundary(request, task, keyPath, props, owner, stack); + renderSuspenseBoundary(request, task, keyPath, props); } return; } @@ -2231,20 +2094,11 @@ function renderElement( if (typeof type === 'object' && type !== null) { switch (type.$$typeof) { case REACT_FORWARD_REF_TYPE: { - renderForwardRef( - request, - task, - keyPath, - type, - props, - ref, - owner, - stack, - ); + renderForwardRef(request, task, keyPath, type, props, ref); return; } case REACT_MEMO_TYPE: { - renderMemo(request, task, keyPath, type, props, ref, owner, stack); + renderMemo(request, task, keyPath, type, props, ref); return; } case REACT_PROVIDER_TYPE: { @@ -2281,16 +2135,7 @@ function renderElement( // Fall through } case REACT_LAZY_TYPE: { - renderLazyComponent( - request, - task, - keyPath, - type, - props, - ref, - owner, - stack, - ); + renderLazyComponent(request, task, keyPath, type, props, ref); return; } } @@ -2370,8 +2215,6 @@ 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; @@ -2399,7 +2242,7 @@ function replayElement( const currentNode = task.node; task.replay = {nodes: childNodes, slots: childSlots, pendingTasks: 1}; try { - renderElement(request, task, keyPath, type, props, ref, owner, stack); + renderElement(request, task, keyPath, type, props, ref); if ( task.replay.pendingTasks === 1 && task.replay.nodes.length > 0 @@ -2466,8 +2309,6 @@ 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 @@ -2496,7 +2337,7 @@ function validateIterable( const isGeneratorComponent = childIndex === -1 && // Only the root child is valid task.componentStack !== null && - task.componentStack.tag === 1 && // FunctionComponent + typeof task.componentStack.type === 'function' && // FunctionComponent // $FlowFixMe[method-unbinding] Object.prototype.toString.call(task.componentStack.type) === '[object GeneratorFunction]' && @@ -2543,7 +2384,7 @@ function validateAsyncIterable( const isGeneratorComponent = childIndex === -1 && // Only the root child is valid task.componentStack !== null && - task.componentStack.tag === 1 && // FunctionComponent + typeof task.componentStack.type === 'function' && // FunctionComponent // $FlowFixMe[method-unbinding] Object.prototype.toString.call(task.componentStack.type) === '[object AsyncGeneratorFunction]' && @@ -2604,7 +2445,18 @@ function renderNodeDestructive( task.node = node; task.childIndex = childIndex; + const previousComponentStack = task.componentStack; + const previousDebugTask = + __DEV__ && enableOwnerStacks ? task.debugTask : null; + + pushComponentStack(task); + retryNode(request, task); + + task.componentStack = previousComponentStack; + if (__DEV__ && enableOwnerStacks) { + task.debugTask = previousDebugTask; + } } function retryNode(request: Request, task: Task): void { @@ -2635,21 +2487,8 @@ function retryNode(request: Request, task: Task): void { ref = element.ref; } - const owner = __DEV__ ? element._owner : null; - const stack = __DEV__ && enableOwnerStacks ? element._debugStack : null; - - let previousDebugTask: null | ConsoleTask = null; - const previousComponentStack = task.componentStack; - let debugTask: null | ConsoleTask; - if (__DEV__) { - if (enableOwnerStacks) { - previousDebugTask = task.debugTask; - } - pushServerComponentStack(task, element._debugInfo); - if (enableOwnerStacks) { - task.debugTask = debugTask = element._debugTask; - } - } + const debugTask: null | ConsoleTask = + __DEV__ && enableOwnerStacks ? task.debugTask : null; const name = getComponentNameFromType(type); const keyOrIndex = @@ -2670,8 +2509,6 @@ function retryNode(request: Request, task: Task): void { props, ref, task.replay, - owner, - stack, ), ); } else { @@ -2686,8 +2523,6 @@ function retryNode(request: Request, task: Task): void { props, ref, task.replay, - owner, - stack, ); } // No matches found for this node. We assume it's already emitted in the @@ -2704,27 +2539,10 @@ function retryNode(request: Request, task: Task): void { type, props, ref, - owner, - stack, ), ); } else { - renderElement( - request, - task, - keyPath, - type, - props, - ref, - owner, - stack, - ); - } - } - if (__DEV__) { - task.componentStack = previousComponentStack; - if (enableOwnerStacks) { - task.debugTask = previousDebugTask; + renderElement(request, task, keyPath, type, props, ref); } } return; @@ -2736,23 +2554,6 @@ function retryNode(request: Request, task: Task): void { ); case REACT_LAZY_TYPE: { const lazyNode: LazyComponentType = (node: any); - const previousComponentStack = task.componentStack; - let previousDebugTask = null; - if (__DEV__) { - if (enableOwnerStacks) { - previousDebugTask = task.debugTask; - } - pushServerComponentStack(task, lazyNode._debugInfo); - } - if (!__DEV__ || task.componentStack === previousComponentStack) { - // TODO: Do we really need this stack frame? We don't on the client. - task.componentStack = createBuiltInComponentStack( - task, - 'Lazy', - null, - null, - ); - } let resolvedNode; if (__DEV__) { resolvedNode = callLazyInitInDEV(lazyNode); @@ -2761,14 +2562,6 @@ function retryNode(request: Request, task: Task): void { 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 - task.componentStack = previousComponentStack; - if (__DEV__ && enableOwnerStacks) { - task.debugTask = previousDebugTask; - } - // Now we render the resolved node renderNodeDestructive(request, task, resolvedNode, childIndex); return; @@ -2820,15 +2613,6 @@ function retryNode(request: Request, task: Task): void { // for new iterators, but we currently warn for rendering these // so needs some refactoring to deal with the warning. - // We need to push a component stack because if this suspends, we'll pop a stack. - const previousComponentStack = task.componentStack; - task.componentStack = createBuiltInComponentStack( - task, - 'AsyncIterable', - null, - null, - ); - // Restore the thenable state before resuming. const prevThenableState = task.thenableState; task.thenableState = null; @@ -2866,7 +2650,6 @@ function retryNode(request: Request, task: Task): void { step = unwrapThenable(iterator.next()); } } - task.componentStack = previousComponentStack; renderChildrenArray(request, task, children, childIndex); return; } @@ -2886,19 +2669,12 @@ function retryNode(request: Request, task: Task): void { // Clear any previous thenable state that was created by the unwrapping. task.thenableState = null; const thenable: Thenable = (maybeUsable: any); - const previousComponentStack = task.componentStack; - if (__DEV__) { - pushServerComponentStack(task, thenable._debugInfo); - } const result = renderNodeDestructive( request, task, unwrapThenable(thenable), childIndex, ); - if (__DEV__) { - task.componentStack = previousComponentStack; - } return result; } @@ -3076,8 +2852,8 @@ function warnForMissingKey(request: Request, task: Task, child: mixed): void { const parentOwner = parentStackFrame.owner; let currentComponentErrorInfo = ''; - if (parentOwner && typeof parentOwner.tag === 'number') { - const name = getComponentNameFromType((parentOwner: any).type); + if (parentOwner && typeof parentOwner.type !== 'undefined') { + const name = getComponentNameFromType(parentOwner.type); if (name) { currentComponentErrorInfo = '\n\nCheck the render method of `' + name + '`.'; @@ -3095,8 +2871,8 @@ function warnForMissingKey(request: Request, task: Task, child: mixed): void { let childOwnerAppendix = ''; if (childOwner != null && parentOwner !== childOwner) { let ownerName = null; - if (typeof childOwner.tag === 'number') { - ownerName = getComponentNameFromType((childOwner: any).type); + if (typeof childOwner.type !== 'undefined') { + ownerName = getComponentNameFromType(childOwner.type); } else if (typeof childOwner.name === 'string') { ownerName = childOwner.name; } @@ -3107,8 +2883,9 @@ function warnForMissingKey(request: Request, task: Task, child: mixed): void { } // We create a fake component stack for the child to log the stack trace from. + const previousComponentStack = task.componentStack; const stackFrame = createComponentStackFromType( - task, + task.componentStack, (child: any).type, (child: any)._owner, enableOwnerStacks ? (child: any)._debugStack : null, @@ -3120,7 +2897,7 @@ function warnForMissingKey(request: Request, task: Task, child: mixed): void { currentComponentErrorInfo, childOwnerAppendix, ); - task.componentStack = stackFrame.parent; + task.componentStack = previousComponentStack; } } @@ -3455,9 +3232,7 @@ function spawnNewSuspendedReplayTask( task.formatContext, task.context, task.treeContext, - // We pop one task off the stack because the node that suspended will be tried again, - // which will add it back onto the stack. - task.componentStack !== null ? task.componentStack.parent : null, + task.componentStack, task.isFallback, !disableLegacyContext ? task.legacyContext : emptyContextObject, __DEV__ && enableOwnerStacks ? task.debugTask : null, @@ -3502,9 +3277,7 @@ function spawnNewSuspendedRenderTask( task.formatContext, task.context, task.treeContext, - // We pop one task off the stack because the node that suspended will be tried again, - // which will add it back onto the stack. - task.componentStack !== null ? task.componentStack.parent : null, + task.componentStack, task.isFallback, !disableLegacyContext ? task.legacyContext : emptyContextObject, __DEV__ && enableOwnerStacks ? task.debugTask : null, @@ -3532,6 +3305,8 @@ function renderNode( const previousKeyPath = task.keyPath; const previousTreeContext = task.treeContext; const previousComponentStack = task.componentStack; + const previousDebugTask = + __DEV__ && enableOwnerStacks ? task.debugTask : null; let x; // Store how much we've pushed at this point so we can reset it in case something // suspended partially through writing something. @@ -3576,6 +3351,9 @@ function renderNode( task.keyPath = previousKeyPath; task.treeContext = previousTreeContext; task.componentStack = previousComponentStack; + if (__DEV__ && enableOwnerStacks) { + task.debugTask = previousDebugTask; + } // Restore all active ReactContexts to what they were before. switchContext(previousContext); return; @@ -3630,6 +3408,9 @@ function renderNode( task.keyPath = previousKeyPath; task.treeContext = previousTreeContext; task.componentStack = previousComponentStack; + if (__DEV__ && enableOwnerStacks) { + task.debugTask = previousDebugTask; + } // Restore all active ReactContexts to what they were before. switchContext(previousContext); return; @@ -3666,6 +3447,9 @@ function renderNode( task.keyPath = previousKeyPath; task.treeContext = previousTreeContext; task.componentStack = previousComponentStack; + if (__DEV__ && enableOwnerStacks) { + task.debugTask = previousDebugTask; + } // Restore all active ReactContexts to what they were before. switchContext(previousContext); return; @@ -4238,11 +4022,6 @@ function retryRenderTask( const ping = task.ping; x.then(ping, ping); task.thenableState = getThenableStateAfterSuspending(); - // We pop one task off the stack because the node that suspended will be tried again, - // which will add it back onto the stack. - if (task.componentStack !== null) { - task.componentStack = task.componentStack.parent; - } return; } else if ( enablePostpone && @@ -4343,11 +4122,6 @@ function retryReplayTask(request: Request, task: ReplayTask): void { const ping = task.ping; x.then(ping, ping); task.thenableState = getThenableStateAfterSuspending(); - // We pop one task off the stack because the node that suspended will be tried again, - // which will add it back onto the stack. - if (task.componentStack !== null) { - task.componentStack = task.componentStack.parent; - } return; } } From 296263eeed88315cb671df94081be734bad9b6f9 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 9 Jul 2024 13:53:47 -0400 Subject: [PATCH 5/6] This gets called when generating stack frames --- packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js index eb41a627b780e..1e93c2420b8c8 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js @@ -585,7 +585,7 @@ describe('ReactDOMFizzServerNode', () => { let isComplete = false; let rendered = false; const promise = new Promise(r => (resolve = r)); - function Wait() { + function Wait({prop}) { if (!hasLoaded) { throw promise; } From f0a72be12a3fb4c81ccd7e92bc402ee853896657 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 9 Jul 2024 13:59:44 -0400 Subject: [PATCH 6/6] The extra stack frame makes stacks longer now This makes the escaping loop longer. --- scripts/babel/transform-prevent-infinite-loops.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/babel/transform-prevent-infinite-loops.js b/scripts/babel/transform-prevent-infinite-loops.js index aa88377cc04c5..635526c14e99b 100644 --- a/scripts/babel/transform-prevent-infinite-loops.js +++ b/scripts/babel/transform-prevent-infinite-loops.js @@ -13,7 +13,7 @@ // This should be reasonable for all loops in the source. // Note that if the numbers are too large, the tests will take too long to fail // for this to be useful (each individual test case might hit an infinite loop). -const MAX_SOURCE_ITERATIONS = 5000; +const MAX_SOURCE_ITERATIONS = 6000; // Code in tests themselves is permitted to run longer. // For example, in the fuzz tester. const MAX_TEST_ITERATIONS = 5000;