From 8f28fba15d264a04a04b539cf2cc2ce6c3e2eac2 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Thu, 3 Jun 2021 00:04:52 -0400 Subject: [PATCH 1/2] Implement component stacks This uses a reverse linked list in DEV-only to keep track of where we're currently executing. --- .../src/ReactFizzComponentStack.js | 61 ++++++++++ packages/react-server/src/ReactFizzServer.js | 111 +++++++++++++++++- 2 files changed, 168 insertions(+), 4 deletions(-) create mode 100644 packages/react-server/src/ReactFizzComponentStack.js diff --git a/packages/react-server/src/ReactFizzComponentStack.js b/packages/react-server/src/ReactFizzComponentStack.js new file mode 100644 index 0000000000000..1fa49453d31b2 --- /dev/null +++ b/packages/react-server/src/ReactFizzComponentStack.js @@ -0,0 +1,61 @@ +/** + * Copyright (c) Facebook, Inc. and its 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 { + describeBuiltInComponentFrame, + describeFunctionComponentFrame, + describeClassComponentFrame, +} from 'shared/ReactComponentStackFrame'; + +// DEV-only reverse linked list representing the current component stack +type BuiltInComponentStackNode = { + tag: 0, + parent: null | ComponentStackNode, + type: string, +}; +type FunctionComponentStackNode = { + tag: 1, + parent: null | ComponentStackNode, + type: Function, +}; +type ClassComponentStackNode = { + tag: 2, + parent: null | ComponentStackNode, + type: Function, +}; +export type ComponentStackNode = + | BuiltInComponentStackNode + | FunctionComponentStackNode + | ClassComponentStackNode; + +export function getStackByComponentStackNode( + componentStack: ComponentStackNode, +): string { + try { + let info = ''; + let node = componentStack; + do { + switch (node.tag) { + case 0: + info += describeBuiltInComponentFrame(node.type, null, null); + break; + case 1: + info += describeFunctionComponentFrame(node.type, null, null); + break; + case 2: + info += describeClassComponentFrame(node.type, null, null); + break; + } + node = node.parent; + } while (node); + return info; + } catch (x) { + return '\nError generating stack: ' + x.message + '\n' + x.stack; + } +} diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index f272c18b462db..4780003534b61 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -24,6 +24,7 @@ import type { FormatContext, } from './ReactServerFormatConfig'; import type {ContextSnapshot} from './ReactFizzNewContext'; +import type {ComponentStackNode} from './ReactFizzComponentStack'; import { scheduleWork, @@ -77,6 +78,7 @@ import { currentResponseState, setCurrentResponseState, } from './ReactFizzHooks'; +import {getStackByComponentStackNode} from './ReactFizzComponentStack'; import { getIteratorFn, @@ -110,6 +112,7 @@ import invariant from 'shared/invariant'; import isArray from 'shared/isArray'; const ReactCurrentDispatcher = ReactSharedInternals.ReactCurrentDispatcher; +const ReactDebugCurrentFrame = ReactSharedInternals.ReactDebugCurrentFrame; type LegacyContext = { [key: string]: any, @@ -135,6 +138,7 @@ type Task = { legacyContext: LegacyContext, // the current legacy context that this task is executing in context: ContextSnapshot, // the current new context that this task is executing in assignID: null | SuspenseBoundaryID, // id to assign to the content + componentStack: null | ComponentStackNode, // DEV-only component stack }; const PENDING = 0; @@ -299,7 +303,7 @@ function createTask( } else { blockedBoundary.pendingTasks++; } - const task = { + const task: Task = ({ node, ping: () => pingTask(request, task), blockedBoundary, @@ -308,7 +312,10 @@ function createTask( legacyContext, context, assignID, - }; + }: any); + if (__DEV__) { + task.componentStack = currentTaskInDEV ? task.componentStack : null; + } abortSet.add(task); return task; } @@ -331,6 +338,57 @@ 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) { + return ''; + } + return getStackByComponentStackNode(currentTaskInDEV.componentStack); + } + return ''; +} + +function pushBuiltInComponentStackInDEV(task: Task, type: string): void { + if (__DEV__) { + task.componentStack = { + tag: 0, + parent: task.componentStack, + type, + }; + } +} +function pushFunctionComponentStackInDEV(task: Task, type: Function): void { + if (__DEV__) { + task.componentStack = { + tag: 1, + parent: task.componentStack, + type, + }; + } +} +function pushClassComponentStackInDEV(task: Task, type: Function): void { + if (__DEV__) { + task.componentStack = { + tag: 2, + parent: task.componentStack, + type, + }; + } +} +function popComponentStackInDEV(task: Task): void { + if (__DEV__) { + if (task.componentStack === null) { + console.error( + 'Unexpectedly popped too many stack frames. This is a bug in React.', + ); + } else { + task.componentStack = task.componentStack.parent; + } + } +} + function reportError(request: Request, error: mixed): void { // If this callback errors, we intentionally let that error bubble up to become a fatal error // so that someone fixes the error reporting instead of hiding it. @@ -351,6 +409,7 @@ function renderSuspenseBoundary( task: Task, props: Object, ): void { + pushBuiltInComponentStackInDEV(task, 'Suspense'); const parentBoundary = task.blockedBoundary; const parentSegment = task.blockedSegment; @@ -418,6 +477,7 @@ function renderSuspenseBoundary( } finally { task.blockedBoundary = parentBoundary; task.blockedSegment = parentSegment; + popComponentStackInDEV(task); } // This injects an extra segment just to contain an empty tag with an ID. @@ -456,6 +516,7 @@ function renderHostElement( type: string, props: Object, ): void { + pushBuiltInComponentStackInDEV(task, type); const segment = task.blockedSegment; const children = pushStartInstance( segment.chunks, @@ -476,6 +537,7 @@ function renderHostElement( // the correct context. Therefore this is not in a finally. segment.formatContext = prevContext; pushEndInstance(segment.chunks, type, props); + popComponentStackInDEV(task); } function shouldConstruct(Component) { @@ -564,12 +626,14 @@ function renderClassComponent( Component: any, props: any, ): void { + pushClassComponentStackInDEV(task, Component); const maskedContext = !disableLegacyContext ? getMaskedContext(Component, task.legacyContext) : undefined; const instance = constructClassInstance(Component, props, maskedContext); mountClassInstance(instance, Component, props, maskedContext); finishClassComponent(request, task, instance, Component, props); + popComponentStackInDEV(task); } const didWarnAboutBadClass = {}; @@ -594,6 +658,7 @@ function renderIndeterminateComponent( if (!disableLegacyContext) { legacyContext = getMaskedContext(Component, task.legacyContext); } + pushFunctionComponentStackInDEV(task, Component); if (__DEV__) { if ( @@ -688,6 +753,7 @@ function renderIndeterminateComponent( // the previous task every again, so we can use the destructive recursive form. renderNodeDestructive(request, task, value); } + popComponentStackInDEV(task); } function validateFunctionComponentInDev(Component: any): void { @@ -768,8 +834,10 @@ function renderForwardRef( props: Object, ref: any, ): void { + pushFunctionComponentStackInDEV(task, type.render); const children = renderWithHooks(request, task, type.render, props, ref); renderNodeDestructive(request, task, children); + popComponentStackInDEV(task); } function renderMemo( @@ -866,11 +934,13 @@ function renderLazyComponent( props: Object, ref: any, ): void { + pushBuiltInComponentStackInDEV(task, 'Lazy'); const payload = lazyComponent._payload; const init = lazyComponent._init; const Component = init(payload); const resolvedProps = resolveDefaultProps(Component, props); - return renderElement(request, task, Component, resolvedProps, ref); + renderElement(request, task, Component, resolvedProps, ref); + popComponentStackInDEV(task); } function renderElement( @@ -907,11 +977,17 @@ function renderElement( case REACT_DEBUG_TRACING_MODE_TYPE: case REACT_STRICT_MODE_TYPE: case REACT_PROFILER_TYPE: - case REACT_SUSPENSE_LIST_TYPE: // TODO: SuspenseList should control the boundaries. case REACT_FRAGMENT_TYPE: { renderNodeDestructive(request, task, props.children); return; } + case REACT_SUSPENSE_LIST_TYPE: { + pushBuiltInComponentStackInDEV(task, 'SuspenseList'); + // TODO: SuspenseList should control the boundaries. + renderNodeDestructive(request, task, props.children); + popComponentStackInDEV(task); + return; + } case REACT_SCOPE_TYPE: { if (enableScopeAPI) { renderNodeDestructive(request, task, props.children); @@ -1174,6 +1250,10 @@ function renderNode(request: Request, task: Task, node: ReactNodeList): void { const previousFormatContext = task.blockedSegment.formatContext; const previousLegacyContext = task.legacyContext; const previousContext = task.context; + let previousComponentStack = null; + if (__DEV__) { + previousComponentStack = task.componentStack; + } try { return renderNodeDestructive(request, task, node); } catch (x) { @@ -1187,6 +1267,9 @@ function renderNode(request: Request, task: Task, node: ReactNodeList): void { task.context = previousContext; // Restore all active ReactContexts to what they were before. switchContext(previousContext); + if (__DEV__) { + task.componentStack = previousComponentStack; + } } else { // Restore the context. We assume that this will be restored by the inner // functions in case nothing throws so we don't use "finally" here. @@ -1195,6 +1278,9 @@ function renderNode(request: Request, task: Task, node: ReactNodeList): void { task.context = previousContext; // Restore all active ReactContexts to what they were before. switchContext(previousContext); + if (__DEV__) { + task.componentStack = previousComponentStack; + } // We assume that we don't need the correct context. // Let's terminate the rest of the tree and don't render any siblings. throw x; @@ -1360,6 +1446,11 @@ function retryTask(request: Request, task: Task): void { // We don't restore it after we leave because it's likely that we'll end up // needing a very similar context soon again. switchContext(task.context); + let prevTaskInDEV = null; + if (__DEV__) { + prevTaskInDEV = currentTaskInDEV; + currentTaskInDEV = task; + } 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. @@ -1379,6 +1470,10 @@ function retryTask(request: Request, task: Task): void { segment.status = ERRORED; erroredTask(request, task.blockedBoundary, segment, x); } + } finally { + if (__DEV__) { + currentTaskInDEV = prevTaskInDEV; + } } } @@ -1389,6 +1484,11 @@ export function performWork(request: Request): void { const prevContext = getActiveContext(); const prevDispatcher = ReactCurrentDispatcher.current; ReactCurrentDispatcher.current = Dispatcher; + let prevGetCurrentStackImpl; + if (__DEV__) { + prevGetCurrentStackImpl = ReactDebugCurrentFrame.getCurrentStack; + ReactDebugCurrentFrame.getCurrentStack = getCurrentStackInDEV; + } const prevResponseState = currentResponseState; setCurrentResponseState(request.responseState); try { @@ -1408,6 +1508,9 @@ export function performWork(request: Request): void { } finally { setCurrentResponseState(prevResponseState); ReactCurrentDispatcher.current = prevDispatcher; + if (__DEV__) { + ReactDebugCurrentFrame.getCurrentStack = prevGetCurrentStackImpl; + } if (prevDispatcher === Dispatcher) { // This means that we were in a reentrant work loop. This could happen // in a renderer that supports synchronous work like renderToString, From 3d2a1ab912af683a9d12da80f36856880e5e8669 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Thu, 3 Jun 2021 15:50:22 -0400 Subject: [PATCH 2/2] Fix bug that wasn't picking up the right stack at suspended boundaries This makes it more explicit which stack we pass in to be retained by the task. --- .../src/__tests__/ReactDOMFizzServer-test.js | 112 ++++++++++++++++++ packages/react-server/src/ReactFizzServer.js | 16 ++- 2 files changed, 126 insertions(+), 2 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index 872f7042ec6fe..eb9eb0e4ef836 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -1058,6 +1058,118 @@ describe('ReactDOMFizzServer', () => { ); }); + function normalizeCodeLocInfo(str) { + return ( + str && + str.replace(/\n +(?:at|in) ([\S]+)[^\n]*/g, function(m, name) { + return '\n in ' + name + ' (at **)'; + }) + ); + } + + // @gate experimental + it('should include a component stack across suspended boundaries', async () => { + function B() { + const children = [readText('Hello'), readText('World')]; + // Intentionally trigger a key warning here. + return ( +
+ {children.map(t => ( + {t} + ))} +
+ ); + } + function C() { + return ( + + + + ); + } + function A() { + return ( +
+ }> + + +
+ ); + } + + // We can't use the toErrorDev helper here because this is an async act. + const originalConsoleError = console.error; + const mockError = jest.fn(); + console.error = (...args) => { + mockError(...args.map(normalizeCodeLocInfo)); + }; + + try { + await act(async () => { + const {startWriting} = ReactDOMFizzServer.pipeToNodeWritable( + , + writable, + ); + startWriting(); + }); + + expect(getVisibleChildren(container)).toEqual( +
+ Loading +
, + ); + + if (__DEV__) { + expect(mockError).toHaveBeenCalledWith( + 'Warning: <%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 **)', + ); + mockError.mockClear(); + } else { + expect(mockError).not.toHaveBeenCalled(); + } + + await act(async () => { + resolveText('Hello'); + resolveText('World'); + }); + + if (__DEV__) { + expect(mockError).toHaveBeenCalledWith( + 'Warning: Each child in a list should have a unique "key" prop.%s%s' + + ' See https://reactjs.org/link/warning-keys for more information.%s', + '\n\nCheck the top-level render call using
.', + '', + '\n' + + ' in span (at **)\n' + + ' in B (at **)\n' + + ' in Suspense (at **)\n' + + ' in div (at **)\n' + + ' in A (at **)', + ); + } else { + expect(mockError).not.toHaveBeenCalled(); + } + + expect(getVisibleChildren(container)).toEqual( +
+
+ Hello + World +
+
, + ); + } finally { + console.error = originalConsoleError; + } + }); + // @gate experimental it('should can suspend in a class component with legacy context', async () => { class TestProvider extends React.Component { diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index 4780003534b61..d7189ab06c976 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -314,7 +314,7 @@ function createTask( assignID, }: any); if (__DEV__) { - task.componentStack = currentTaskInDEV ? task.componentStack : null; + task.componentStack = null; } abortSet.add(task); return task; @@ -465,6 +465,7 @@ function renderSuspenseBoundary( // This must have been the last segment we were waiting on. This boundary is now complete. // Therefore we won't need the fallback. We early return so that we don't have to create // the fallback. + popComponentStackInDEV(task); return; } } catch (error) { @@ -477,7 +478,6 @@ function renderSuspenseBoundary( } finally { task.blockedBoundary = parentBoundary; task.blockedSegment = parentSegment; - popComponentStackInDEV(task); } // This injects an extra segment just to contain an empty tag with an ID. @@ -505,9 +505,14 @@ function renderSuspenseBoundary( task.context, null, ); + if (__DEV__) { + suspendedFallbackTask.componentStack = task.componentStack; + } // 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); + + popComponentStackInDEV(task); } function renderHostElement( @@ -1233,6 +1238,13 @@ function spawnNewSuspendedTask( task.context, task.assignID, ); + if (__DEV__) { + if (task.componentStack !== null) { + // We pop one task off the stack because the node that suspended will be tried again, + // which will add it back onto the stack. + newTask.componentStack = task.componentStack.parent; + } + } // We've delegated the assignment. task.assignID = null; const ping = newTask.ping;