From d6cfa0f295f4c8b366af15fd20c84e27cdd1fab7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Sat, 25 May 2024 11:58:17 -0400 Subject: [PATCH] [Fiber] Use Owner/JSX Stack When Appending Stacks to Console (#29206) This one should be fully behind the `enableOwnerStacks` flag. Instead of printing the parent Component stack all the way to the root, this now prints the owner stack of every JSX callsite. It also includes intermediate callsites between the Component and the JSX call so it has potentially more frames. Mainly it provides the line number of the JSX callsite. In terms of the number of components is a subset of the parent component stack so it's less information in that regard. This is usually better since it's more focused on components that might affect the output but if it's contextual based on rendering it's still good to have parent stack. Therefore, I still use the parent stack when printing DOM nesting warnings but I plan on switching that format to a diff view format instead (Next.js already reformats the parent stack like this). __Follow ups__ - Server Components show up in the owner stack for client logs but logs done by Server Components don't yet get their owner stack printed as they're replayed. They're also not yet printed in the server logs of the RSC server. - Server Component stack frames are formatted as the server and added to the end but this might be a different format than the browser. E.g. if server is running V8 and browser is running JSC or vice versa. Ideally we can reformat them in terms of the client formatting. - This doesn't yet update Fizz or DevTools. Those will be follow ups. Fizz still prints parent stacks in the server side logs. The stacks added to user space `console.error` calls by DevTools still get the parent stacks instead. - It also doesn't yet expose these to user space so there's no way to get them inside `onCaughtError` for example or inside a custom `console.error` override. - In another follow up I'll use `console.createTask` instead and completely remove these stacks if it's available. --- .eslintrc.js | 1 + .../src/__tests__/ReactFlight-test.js | 16 +-- .../src/client/validateDOMNesting.js | 50 ++++++-- .../__tests__/ReactChildReconciler-test.js | 8 +- .../src/__tests__/ReactComponent-test.js | 8 +- .../react-dom/src/__tests__/ReactDOM-test.js | 4 +- .../src/__tests__/ReactMultiChild-test.js | 8 +- .../src/__tests__/ReactUpdates-test.js | 13 +- .../react-reconciler/src/ReactChildFiber.js | 58 ++++++--- .../react-reconciler/src/ReactCurrentFiber.js | 22 +++- packages/react-reconciler/src/ReactFiber.js | 13 ++ .../src/ReactFiberBeginWork.js | 55 +++++---- .../src/ReactFiberCallUserSpace.js | 46 +++++++ .../src/ReactFiberComponentStack.js | 105 ++++++++++++++++ .../react-reconciler/src/ReactFiberHooks.js | 10 +- .../src/ReactFiberOwnerStack.js | 112 ++++++++++++++++++ .../src/ReactInternalTypes.js | 2 + .../src/__tests__/ReactHooks-test.internal.js | 2 + .../ReactIncrementalErrorLogging-test.js | 30 +++-- .../src/__tests__/ReactLazy-test.internal.js | 21 ++-- .../src/__tests__/ReactMemo-test.js | 24 ++-- packages/react-server/src/ReactFizzServer.js | 1 + packages/react/src/ReactForwardRef.js | 4 +- packages/react/src/ReactMemo.js | 4 +- .../src/__tests__/ReactCreateRef-test.js | 12 +- .../ReactElementValidator-test.internal.js | 9 +- .../ReactJSXElementValidator-test.js | 2 - .../src/__tests__/ReactJSXRuntime-test.js | 7 +- .../createReactClassIntegration-test.js | 2 + .../react/src/__tests__/forwardRef-test.js | 26 ++-- packages/react/src/jsx/ReactJSXElement.js | 48 +++++++- packages/shared/ReactElementType.js | 4 - scripts/flow/environment.js | 4 + scripts/jest/matchers/toWarnDev.js | 5 + 34 files changed, 591 insertions(+), 145 deletions(-) create mode 100644 packages/react-reconciler/src/ReactFiberCallUserSpace.js create mode 100644 packages/react-reconciler/src/ReactFiberOwnerStack.js diff --git a/.eslintrc.js b/.eslintrc.js index 0197c75b76..cf5b585870 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -486,6 +486,7 @@ module.exports = { $ReadOnlyArray: 'readonly', $ArrayBufferView: 'readonly', $Shape: 'readonly', + ConsoleTask: 'readonly', // TOOD: Figure out what the official name of this will be. ReturnType: 'readonly', AnimationFrameID: 'readonly', // For Flow type annotation. Only `BigInt` is valid at runtime. diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index b7a73eb2ab..bc660a207f 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -1123,10 +1123,11 @@ describe('ReactFlight', () => { } function App() { - return ( - - - + // We use the ReactServer runtime here to get the Server owner. + return ReactServer.createElement( + Indirection, + null, + ReactServer.createElement(ClientComponent), ); } @@ -1143,11 +1144,10 @@ describe('ReactFlight', () => { '\n' + 'Check the render method of `Component`. See https://react.dev/link/warning-keys for more information.\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 Component (at **)\n' + - ' in Indirection (at **)\n' + + (gate(flags => flags.enableOwnerStacks) + ? '' + : ' in Indirection (at **)\n') + ' in App (at **)', ); }); diff --git a/packages/react-dom-bindings/src/client/validateDOMNesting.js b/packages/react-dom-bindings/src/client/validateDOMNesting.js index ab49cb4f4b..b7e99757ca 100644 --- a/packages/react-dom-bindings/src/client/validateDOMNesting.js +++ b/packages/react-dom-bindings/src/client/validateDOMNesting.js @@ -7,6 +7,8 @@ * @flow */ +import {getCurrentParentStackInDev} from 'react-reconciler/src/ReactCurrentFiber'; + type Info = {tag: string}; export type AncestorInfoDev = { current: ?Info, @@ -476,19 +478,31 @@ function validateDOMNesting( ' Add a , or to your code to match the DOM tree generated by ' + 'the browser.'; } - console.error( - 'In HTML, %s cannot be a child of <%s>.%s\n' + - 'This will cause a hydration error.', + // Don't transform into consoleWithStackDev here because we add a manual stack. + // We use the parent stack here instead of the owner stack because the parent + // stack has more useful context for nesting. + // TODO: Format this as a linkified "diff view" with props instead of + // a stack trace since the stack trace format is now for owner stacks. + console['error']( + 'Warning: In HTML, %s cannot be a child of <%s>.%s\n' + + 'This will cause a hydration error.%s', tagDisplayName, ancestorTag, info, + getCurrentParentStackInDev(), ); } else { - console.error( - 'In HTML, %s cannot be a descendant of <%s>.\n' + - 'This will cause a hydration error.', + // Don't transform into consoleWithStackDev here because we add a manual stack. + // We use the parent stack here instead of the owner stack because the parent + // stack has more useful context for nesting. + // TODO: Format this as a linkified "diff view" with props instead of + // a stack trace since the stack trace format is now for owner stacks. + console['error']( + 'Warning: In HTML, %s cannot be a descendant of <%s>.\n' + + 'This will cause a hydration error.%s', tagDisplayName, ancestorTag, + getCurrentParentStackInDev(), ); } return false; @@ -510,18 +524,30 @@ function validateTextNesting(childText: string, parentTag: string): boolean { didWarn[warnKey] = true; if (/\S/.test(childText)) { - console.error( - 'In HTML, text nodes cannot be a child of <%s>.\n' + - 'This will cause a hydration error.', + // Don't transform into consoleWithStackDev here because we add a manual stack. + // We use the parent stack here instead of the owner stack because the parent + // stack has more useful context for nesting. + // TODO: Format this as a linkified "diff view" with props instead of + // a stack trace since the stack trace format is now for owner stacks. + console['error']( + 'Warning: In HTML, text nodes cannot be a child of <%s>.\n' + + 'This will cause a hydration error.%s', parentTag, + getCurrentParentStackInDev(), ); } else { - console.error( - 'In HTML, whitespace text nodes cannot be a child of <%s>. ' + + // Don't transform into consoleWithStackDev here because we add a manual stack. + // We use the parent stack here instead of the owner stack because the parent + // stack has more useful context for nesting. + // TODO: Format this as a linkified "diff view" with props instead of + // a stack trace since the stack trace format is now for owner stacks. + console['error']( + 'Warning: In HTML, whitespace text nodes cannot be a child of <%s>. ' + "Make sure you don't have any extra whitespace between tags on " + 'each line of your source code.\n' + - 'This will cause a hydration error.', + 'This will cause a hydration error.%s', parentTag, + getCurrentParentStackInDev(), ); } return false; diff --git a/packages/react-dom/src/__tests__/ReactChildReconciler-test.js b/packages/react-dom/src/__tests__/ReactChildReconciler-test.js index ef584a7856..425f4abd13 100644 --- a/packages/react-dom/src/__tests__/ReactChildReconciler-test.js +++ b/packages/react-dom/src/__tests__/ReactChildReconciler-test.js @@ -130,7 +130,9 @@ describe('ReactChildReconciler', () => { 'could change in a future version.\n' + ' in div (at **)\n' + ' in Component (at **)\n' + - ' in Parent (at **)\n' + + (gate(flags => flags.enableOwnerStacks) + ? '' + : ' in Parent (at **)\n') + ' in GrandParent (at **)', ); }); @@ -189,7 +191,9 @@ describe('ReactChildReconciler', () => { 'could change in a future version.\n' + ' in div (at **)\n' + ' in Component (at **)\n' + - ' in Parent (at **)\n' + + (gate(flags => flags.enableOwnerStacks) + ? '' + : ' in Parent (at **)\n') + ' in GrandParent (at **)', ); }); diff --git a/packages/react-dom/src/__tests__/ReactComponent-test.js b/packages/react-dom/src/__tests__/ReactComponent-test.js index 61538fed69..1951f4ba73 100644 --- a/packages/react-dom/src/__tests__/ReactComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactComponent-test.js @@ -761,7 +761,9 @@ describe('ReactComponent', () => { 'Or maybe you meant to call this function rather than return it.\n' + ' {Foo}\n' + ' in span (at **)\n' + - ' in div (at **)\n' + + (gate(flags => flags.enableOwnerStacks) + ? '' + : ' in div (at **)\n') + ' in Foo (at **)', ); }); @@ -820,7 +822,9 @@ describe('ReactComponent', () => { 'Or maybe you meant to call this function rather than return it.\n' + ' {Foo}\n' + ' in span (at **)\n' + - ' in div (at **)\n' + + (gate(flags => flags.enableOwnerStacks) + ? '' + : ' in div (at **)\n') + ' in Foo (at **)', ]); await act(() => { diff --git a/packages/react-dom/src/__tests__/ReactDOM-test.js b/packages/react-dom/src/__tests__/ReactDOM-test.js index 72fa57fb1f..697b65dd60 100644 --- a/packages/react-dom/src/__tests__/ReactDOM-test.js +++ b/packages/react-dom/src/__tests__/ReactDOM-test.js @@ -552,7 +552,7 @@ describe('ReactDOM', () => { // ReactDOM(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' + + (gate(flags => flags.enableOwnerStacks) ? '' : ' in div (at **)\n') + ' in App (at **)', // ReactDOM(App > div > ServerEntry) >>> ReactDOMServer(Child) >>> ReactDOMServer(App2) >>> ReactDOMServer(blink) 'Invalid ARIA attribute `ariaTypo2`. ARIA attributes follow the pattern aria-* and must be lowercase.\n' + @@ -569,7 +569,7 @@ describe('ReactDOM', () => { // ReactDOM(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' + + (gate(flags => flags.enableOwnerStacks) ? '' : ' in div (at **)\n') + ' in App (at **)', ]); }); diff --git a/packages/react-dom/src/__tests__/ReactMultiChild-test.js b/packages/react-dom/src/__tests__/ReactMultiChild-test.js index 44d71fe7fc..1ffc793f96 100644 --- a/packages/react-dom/src/__tests__/ReactMultiChild-test.js +++ b/packages/react-dom/src/__tests__/ReactMultiChild-test.js @@ -229,7 +229,9 @@ describe('ReactMultiChild', () => { 'could change in a future version.\n' + ' in div (at **)\n' + ' in WrapperComponent (at **)\n' + - ' in div (at **)\n' + + (gate(flags => flags.enableOwnerStacks) + ? '' + : ' in div (at **)\n') + ' in Parent (at **)', ); }); @@ -292,7 +294,9 @@ describe('ReactMultiChild', () => { 'could change in a future version.\n' + ' in div (at **)\n' + ' in WrapperComponent (at **)\n' + - ' in div (at **)\n' + + (gate(flags => flags.enableOwnerStacks) + ? '' + : ' in div (at **)\n') + ' in Parent (at **)', ); }); diff --git a/packages/react-dom/src/__tests__/ReactUpdates-test.js b/packages/react-dom/src/__tests__/ReactUpdates-test.js index 9a10a8d061..1c40e72a78 100644 --- a/packages/react-dom/src/__tests__/ReactUpdates-test.js +++ b/packages/react-dom/src/__tests__/ReactUpdates-test.js @@ -1848,7 +1848,7 @@ describe('ReactUpdates', () => { it('warns about a deferred infinite update loop with useEffect', async () => { function NonTerminating() { const [step, setStep] = React.useState(0); - React.useEffect(() => { + React.useEffect(function myEffect() { setStep(x => x + 1); }); return step; @@ -1860,10 +1860,12 @@ describe('ReactUpdates', () => { let error = null; let stack = null; + let nativeStack = null; const originalConsoleError = console.error; console.error = (e, s) => { error = e; stack = s; + nativeStack = new Error().stack; Scheduler.log('stop'); }; try { @@ -1876,7 +1878,14 @@ describe('ReactUpdates', () => { } expect(error).toContain('Maximum update depth exceeded'); - expect(stack).toContain('at NonTerminating'); + // The currently executing effect should be on the native stack + expect(nativeStack).toContain('at myEffect'); + if (!gate(flags => flags.enableOwnerStacks)) { + // The currently running component's name is not in the owner + // stack because it's just its JSX callsite. + expect(stack).toContain('at NonTerminating'); + } + expect(stack).toContain('at App'); }); it('can have nested updates if they do not cross the limit', async () => { diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index 0f14af18af..3346c73953 100644 --- a/packages/react-reconciler/src/ReactChildFiber.js +++ b/packages/react-reconciler/src/ReactChildFiber.js @@ -61,6 +61,7 @@ import {getIsHydrating} from './ReactFiberHydrationContext'; import {pushTreeFork} from './ReactFiberTreeContext'; import {createThenableState, trackUsedThenable} from './ReactFiberThenable'; import {readContextDuringReconciliation} from './ReactFiberNewContext'; +import {callLazyInitInDEV} from './ReactFiberCallUserSpace'; import { getCurrentFiber as getCurrentDebugFiberInDEV, @@ -362,6 +363,9 @@ function warnOnSymbolType(returnFiber: Fiber, invalidChild: symbol) { } function resolveLazy(lazyType: any) { + if (__DEV__) { + return callLazyInitInDEV(lazyType); + } const payload = lazyType._payload; const init = lazyType._init; return init(payload); @@ -683,11 +687,17 @@ function createChildReconciler( return created; } case REACT_LAZY_TYPE: { - const payload = newChild._payload; - const init = newChild._init; + let resolvedChild; + if (__DEV__) { + resolvedChild = callLazyInitInDEV(newChild); + } else { + const payload = newChild._payload; + const init = newChild._init; + resolvedChild = init(payload); + } return createChild( returnFiber, - init(payload), + resolvedChild, lanes, mergeDebugInfo(debugInfo, newChild._debugInfo), // call merge after init ); @@ -811,12 +821,18 @@ function createChildReconciler( } } case REACT_LAZY_TYPE: { - const payload = newChild._payload; - const init = newChild._init; + let resolvedChild; + if (__DEV__) { + resolvedChild = callLazyInitInDEV(newChild); + } else { + const payload = newChild._payload; + const init = newChild._init; + resolvedChild = init(payload); + } return updateSlot( returnFiber, oldFiber, - init(payload), + resolvedChild, lanes, mergeDebugInfo(debugInfo, newChild._debugInfo), ); @@ -937,17 +953,24 @@ function createChildReconciler( debugInfo, ); } - case REACT_LAZY_TYPE: - const payload = newChild._payload; - const init = newChild._init; + case REACT_LAZY_TYPE: { + let resolvedChild; + if (__DEV__) { + resolvedChild = callLazyInitInDEV(newChild); + } else { + const payload = newChild._payload; + const init = newChild._init; + resolvedChild = init(payload); + } return updateFromMap( existingChildren, returnFiber, newIdx, - init(payload), + resolvedChild, lanes, mergeDebugInfo(debugInfo, newChild._debugInfo), ); + } } if ( @@ -1047,11 +1070,18 @@ function createChildReconciler( key, ); break; - case REACT_LAZY_TYPE: - const payload = child._payload; - const init = (child._init: any); - warnOnInvalidKey(init(payload), knownKeys, returnFiber); + case REACT_LAZY_TYPE: { + let resolvedChild; + if (__DEV__) { + resolvedChild = callLazyInitInDEV((child: any)); + } else { + const payload = child._payload; + const init = (child._init: any); + resolvedChild = init(payload); + } + warnOnInvalidKey(resolvedChild, knownKeys, returnFiber); break; + } default: break; } diff --git a/packages/react-reconciler/src/ReactCurrentFiber.js b/packages/react-reconciler/src/ReactCurrentFiber.js index c0ff5d16df..fa2a11c00a 100644 --- a/packages/react-reconciler/src/ReactCurrentFiber.js +++ b/packages/react-reconciler/src/ReactCurrentFiber.js @@ -10,8 +10,12 @@ import type {Fiber} from './ReactInternalTypes'; import ReactSharedInternals from 'shared/ReactSharedInternals'; -import {getStackByFiberInDevAndProd} from './ReactFiberComponentStack'; +import { + getStackByFiberInDevAndProd, + getOwnerStackByFiberInDev, +} from './ReactFiberComponentStack'; import {getComponentNameFromOwner} from 'react-reconciler/src/getComponentNameFromFiber'; +import {enableOwnerStacks} from 'shared/ReactFeatureFlags'; export let current: Fiber | null = null; export let isRendering: boolean = false; @@ -29,6 +33,17 @@ export function getCurrentFiberOwnerNameInDevOrNull(): string | null { return null; } +export function getCurrentParentStackInDev(): string { + // This is used to get the parent stack even with owner stacks turned on. + if (__DEV__) { + if (current === null) { + return ''; + } + return getStackByFiberInDevAndProd(current); + } + return ''; +} + function getCurrentFiberStackInDev(): string { if (__DEV__) { if (current === null) { @@ -36,6 +51,11 @@ function getCurrentFiberStackInDev(): string { } // Safe because if current fiber exists, we are reconciling, // and it is guaranteed to be the work-in-progress version. + // TODO: The above comment is not actually true. We might be + // in a commit phase or preemptive set state callback. + if (enableOwnerStacks) { + return getOwnerStackByFiberInDev(current); + } return getStackByFiberInDevAndProd(current); } return ''; diff --git a/packages/react-reconciler/src/ReactFiber.js b/packages/react-reconciler/src/ReactFiber.js index 03bdd13f56..7234117a50 100644 --- a/packages/react-reconciler/src/ReactFiber.js +++ b/packages/react-reconciler/src/ReactFiber.js @@ -38,6 +38,7 @@ import { enableDO_NOT_USE_disableStrictPassiveEffect, enableRenderableContext, disableLegacyMode, + enableOwnerStacks, } from 'shared/ReactFeatureFlags'; import {NoFlags, Placement, StaticMask} from './ReactFiberFlags'; import {ConcurrentRoot} from './ReactRootTags'; @@ -205,6 +206,10 @@ function FiberNode( // This isn't directly used but is handy for debugging internals: this._debugInfo = null; this._debugOwner = null; + if (enableOwnerStacks) { + this._debugStack = null; + this._debugTask = null; + } this._debugNeedsRemount = false; this._debugHookTypes = null; if (!hasBadMapPolyfill && typeof Object.preventExtensions === 'function') { @@ -278,6 +283,10 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber { // DEV-only fields workInProgress._debugOwner = current._debugOwner; + if (enableOwnerStacks) { + workInProgress._debugStack = current._debugStack; + workInProgress._debugTask = current._debugTask; + } workInProgress._debugHookTypes = current._debugHookTypes; } @@ -683,6 +692,10 @@ export function createFiberFromElement( ); if (__DEV__) { fiber._debugOwner = element._owner; + if (enableOwnerStacks) { + fiber._debugStack = element._debugStack; + fiber._debugTask = element._debugTask; + } } return fiber; } diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index e638580bb0..6eeb7ab377 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -110,6 +110,7 @@ import { disableLegacyMode, disableDefaultPropsExceptForClasses, disableStringRefs, + enableOwnerStacks, } from 'shared/ReactFeatureFlags'; import isArray from 'shared/isArray'; import shallowEqual from 'shared/shallowEqual'; @@ -124,7 +125,6 @@ import { } from 'shared/ReactSymbols'; import { getCurrentFiberOwnerNameInDevOrNull, - setIsRendering, setCurrentFiber, } from './ReactCurrentFiber'; import { @@ -297,6 +297,11 @@ import { pushRootMarkerInstance, TransitionTracingMarker, } from './ReactFiberTracingMarkerComponent'; +import { + callLazyInitInDEV, + callComponentInDEV, + callRenderInDEV, +} from './ReactFiberCallUserSpace'; // A special exception that's used to unwind the stack when an update flows // into a dehydrated boundary. @@ -432,7 +437,6 @@ function updateForwardRef( markComponentRenderStarted(workInProgress); } if (__DEV__) { - setIsRendering(true); nextChildren = renderWithHooks( current, workInProgress, @@ -442,7 +446,6 @@ function updateForwardRef( renderLanes, ); hasId = checkDidRenderIdHook(); - setIsRendering(false); } else { nextChildren = renderWithHooks( current, @@ -1149,7 +1152,6 @@ function updateFunctionComponent( markComponentRenderStarted(workInProgress); } if (__DEV__) { - setIsRendering(true); nextChildren = renderWithHooks( current, workInProgress, @@ -1159,7 +1161,6 @@ function updateFunctionComponent( renderLanes, ); hasId = checkDidRenderIdHook(); - setIsRendering(false); } else { nextChildren = renderWithHooks( current, @@ -1393,20 +1394,18 @@ function finishClassComponent( markComponentRenderStarted(workInProgress); } if (__DEV__) { - setIsRendering(true); - nextChildren = instance.render(); + nextChildren = callRenderInDEV(instance); if ( debugRenderPhaseSideEffectsForStrictMode && workInProgress.mode & StrictLegacyMode ) { setIsStrictModeForDevtools(true); try { - instance.render(); + callRenderInDEV(instance); } finally { setIsStrictModeForDevtools(false); } } - setIsRendering(false); } else { nextChildren = instance.render(); } @@ -1766,9 +1765,14 @@ function mountLazyComponent( const props = workInProgress.pendingProps; const lazyComponent: LazyComponentType = elementType; - const payload = lazyComponent._payload; - const init = lazyComponent._init; - let Component = init(payload); + let Component; + if (__DEV__) { + Component = callLazyInitInDEV(lazyComponent); + } else { + const payload = lazyComponent._payload; + const init = lazyComponent._init; + Component = init(payload); + } // Store the unwrapped component in the type. workInProgress.type = Component; @@ -3417,9 +3421,7 @@ function updateContextConsumer( } let newChildren; if (__DEV__) { - setIsRendering(true); - newChildren = render(newValue); - setIsRendering(false); + newChildren = callComponentInDEV(render, newValue, undefined); } else { newChildren = render(newValue); } @@ -3831,18 +3833,19 @@ function beginWork( if (__DEV__) { if (workInProgress._debugNeedsRemount && current !== null) { // This will restart the begin phase with a new fiber. - return remountFiber( - current, - workInProgress, - createFiberFromTypeAndProps( - workInProgress.type, - workInProgress.key, - workInProgress.pendingProps, - workInProgress._debugOwner || null, - workInProgress.mode, - workInProgress.lanes, - ), + const copiedFiber = createFiberFromTypeAndProps( + workInProgress.type, + workInProgress.key, + workInProgress.pendingProps, + workInProgress._debugOwner || null, + workInProgress.mode, + workInProgress.lanes, ); + if (enableOwnerStacks) { + copiedFiber._debugStack = workInProgress._debugStack; + copiedFiber._debugTask = workInProgress._debugTask; + } + return remountFiber(current, workInProgress, copiedFiber); } } diff --git a/packages/react-reconciler/src/ReactFiberCallUserSpace.js b/packages/react-reconciler/src/ReactFiberCallUserSpace.js new file mode 100644 index 0000000000..dfc88b64be --- /dev/null +++ b/packages/react-reconciler/src/ReactFiberCallUserSpace.js @@ -0,0 +1,46 @@ +/** + * 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'; + +import {setIsRendering} from './ReactCurrentFiber'; + +// 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 { + setIsRendering(true); + const result = Component(props, secondArg); + setIsRendering(false); + return result; +} + +interface ClassInstance { + render(): R; +} + +/** @noinline */ +export function callRenderInDEV(instance: ClassInstance): R { + setIsRendering(true); + const result = instance.render(); + setIsRendering(false); + return result; +} + +/** @noinline */ +export function callLazyInitInDEV(lazy: LazyComponent): any { + const payload = lazy._payload; + const init = lazy._init; + return init(payload); +} diff --git a/packages/react-reconciler/src/ReactFiberComponentStack.js b/packages/react-reconciler/src/ReactFiberComponentStack.js index 193c27aef1..e5e25f6746 100644 --- a/packages/react-reconciler/src/ReactFiberComponentStack.js +++ b/packages/react-reconciler/src/ReactFiberComponentStack.js @@ -7,7 +7,9 @@ * @flow */ +import {enableOwnerStacks} from 'shared/ReactFeatureFlags'; import type {Fiber} from './ReactInternalTypes'; +import type {ReactComponentInfo} from 'shared/ReactTypes'; import { HostComponent, @@ -20,6 +22,7 @@ import { ForwardRef, SimpleMemoComponent, ClassComponent, + HostText, } from './ReactWorkTags'; import { describeBuiltInComponentFrame, @@ -27,6 +30,7 @@ import { describeClassComponentFrame, describeDebugInfoFrame, } from 'shared/ReactComponentStackFrame'; +import {formatOwnerStack} from './ReactFiberOwnerStack'; function describeFiber(fiber: Fiber): string { switch (fiber.tag) { @@ -78,3 +82,104 @@ export function getStackByFiberInDevAndProd(workInProgress: Fiber): string { 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 getOwnerStackByFiberInDev(workInProgress: Fiber): string { + if (!enableOwnerStacks || !__DEV__) { + return ''; + } + try { + let info = ''; + + if (workInProgress.tag === HostText) { + // Text nodes never have an owner/stack because they're not created through JSX. + // We use the parent since text nodes are always created through a host parent. + workInProgress = (workInProgress.return: any); + } + + // The owner stack of the current fiber 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 (workInProgress.tag) { + case HostHoistable: + case HostSingleton: + case HostComponent: + info += describeBuiltInComponentFrame(workInProgress.type); + break; + case SuspenseComponent: + info += describeBuiltInComponentFrame('Suspense'); + break; + case SuspenseListComponent: + info += describeBuiltInComponentFrame('SuspenseList'); + break; + case FunctionComponent: + case SimpleMemoComponent: + case ClassComponent: + if (!workInProgress._debugOwner) { + info += describeFunctionComponentFrameWithoutLineNumber( + workInProgress.type, + ); + } + break; + case ForwardRef: + if (!workInProgress._debugOwner) { + info += describeFunctionComponentFrameWithoutLineNumber( + workInProgress.type.render, + ); + } + break; + } + + let owner: void | null | Fiber | ReactComponentInfo = workInProgress; + + while (owner) { + if (typeof owner.tag === 'number') { + const fiber: Fiber = (owner: any); + owner = fiber._debugOwner; + let debugStack = fiber._debugStack; + // 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. + fiber._debugStack = debugStack = formatOwnerStack(debugStack); + } + if (debugStack !== '') { + info += '\n' + debugStack; + } + } + } else if (typeof owner.stack === 'string') { + // Server Component + // The Server Component stack can come from a different VM that formats it different. + // Likely V8. Since Chrome based browsers support createTask which is going to use + // another code path anyway. I.e. this is likely NOT a V8 based browser. + // This will cause some of the stack to have different formatting. + // TODO: Normalize server component stacks to the client formatting. + 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-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 17ee55814a..3cb99b3519 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -156,6 +156,8 @@ import {requestTransitionLane} from './ReactFiberRootScheduler'; import {isCurrentTreeHidden} from './ReactFiberHiddenContext'; import {requestCurrentTransition} from './ReactFiberTransition'; +import {callComponentInDEV} from './ReactFiberCallUserSpace'; + export type Update = { lane: Lane, revertLane: Lane, @@ -587,7 +589,9 @@ export function renderWithHooks( (workInProgress.mode & StrictLegacyMode) !== NoMode; shouldDoubleInvokeUserFnsInHooksDEV = shouldDoubleRenderDEV; - let children = Component(props, secondArg); + let children = __DEV__ + ? callComponentInDEV(Component, props, secondArg) + : Component(props, secondArg); shouldDoubleInvokeUserFnsInHooksDEV = false; // Check if there was a render phase update @@ -819,7 +823,9 @@ function renderWithHooksAgain( ? HooksDispatcherOnRerenderInDEV : HooksDispatcherOnRerender; - children = Component(props, secondArg); + children = __DEV__ + ? callComponentInDEV(Component, props, secondArg) + : Component(props, secondArg); } while (didScheduleRenderPhaseUpdateDuringThisPass); return children; } diff --git a/packages/react-reconciler/src/ReactFiberOwnerStack.js b/packages/react-reconciler/src/ReactFiberOwnerStack.js new file mode 100644 index 0000000000..b8510ebf6b --- /dev/null +++ b/packages/react-reconciler/src/ReactFiberOwnerStack.js @@ -0,0 +1,112 @@ +/** + * 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 './ReactFiberCallUserSpace'; + +// 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; + } + return frames.filter(isNotExternal).join('\n'); +} + +export function formatOwnerStack(ownerStackTrace: Error): string { + return filterDebugStack(ownerStackTrace); +} diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js index 6fa9142e2d..8e58b5c44a 100644 --- a/packages/react-reconciler/src/ReactInternalTypes.js +++ b/packages/react-reconciler/src/ReactInternalTypes.js @@ -195,6 +195,8 @@ export type Fiber = { _debugInfo?: ReactDebugInfo | null, _debugOwner?: ReactComponentInfo | Fiber | null, + _debugStack?: string | Error | null, + _debugTask?: ConsoleTask | null, _debugIsCurrentlyTiming?: boolean, _debugNeedsRemount?: boolean, diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 476838776f..8cc5c15a9e 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -716,6 +716,7 @@ describe('ReactHooks', () => { useImperativeHandle(ref, () => {}, props.deps); return null; }); + App.displayName = 'App'; await expect(async () => { await act(() => { @@ -846,6 +847,7 @@ describe('ReactHooks', () => { }); return null; }); + App.displayName = 'App'; await expect(async () => { await act(() => { diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js index 3bd934af38..8333445c69 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js @@ -93,9 +93,11 @@ describe('ReactIncrementalErrorLogging', () => { ), expect.stringMatching( new RegExp( - '\\s+(in|at) ErrorThrowingComponent (.*)\n' + - '\\s+(in|at) span(.*)\n' + - '\\s+(in|at) div(.*)', + gate(flags => flags.enableOwnerStacks) + ? '\\s+(in|at) ErrorThrowingComponent' + : '\\s+(in|at) ErrorThrowingComponent (.*)\n' + + '\\s+(in|at) span(.*)\n' + + '\\s+(in|at) div(.*)', ), ), ); @@ -139,9 +141,11 @@ describe('ReactIncrementalErrorLogging', () => { ), expect.stringMatching( new RegExp( - '\\s+(in|at) ErrorThrowingComponent (.*)\n' + - '\\s+(in|at) span(.*)\n' + - '\\s+(in|at) div(.*)', + gate(flags => flags.enableOwnerStacks) + ? '\\s+(in|at) ErrorThrowingComponent' + : '\\s+(in|at) ErrorThrowingComponent (.*)\n' + + '\\s+(in|at) span(.*)\n' + + '\\s+(in|at) div(.*)', ), ), ); @@ -197,10 +201,12 @@ describe('ReactIncrementalErrorLogging', () => { ), expect.stringMatching( new RegExp( - '\\s+(in|at) ErrorThrowingComponent (.*)\n' + - '\\s+(in|at) span(.*)\n' + - '\\s+(in|at) ErrorBoundary(.*)\n' + - '\\s+(in|at) div(.*)', + gate(flags => flags.enableOwnerStacks) + ? '\\s+(in|at) ErrorThrowingComponent' + : '\\s+(in|at) ErrorThrowingComponent (.*)\n' + + '\\s+(in|at) span(.*)\n' + + '\\s+(in|at) ErrorBoundary(.*)\n' + + '\\s+(in|at) div(.*)', ), ), ); @@ -278,9 +284,7 @@ describe('ReactIncrementalErrorLogging', () => { ), expect.stringMatching( gate(flag => flag.enableOwnerStacks) - ? // With owner stacks the return path is cut off but in this case - // this is also what the owner stack looks like. - new RegExp('\\s+(in|at) Foo (.*)') + ? new RegExp('\\s+(in|at) Foo') : new RegExp( '\\s+(in|at) Foo (.*)\n' + '\\s+(in|at) ErrorBoundary(.*)', ), diff --git a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js index b2874a9952..3ec58b2f70 100644 --- a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js @@ -228,10 +228,18 @@ describe('ReactLazy', () => { expect(error.message).toMatch('Element type is invalid'); assertLog(['Loading...']); - assertConsoleErrorDev([ - 'Expected the result of a dynamic import() call', - 'Expected the result of a dynamic import() call', - ]); + assertConsoleErrorDev( + [ + 'Expected the result of a dynamic import() call', + 'Expected the result of a dynamic import() call', + ], + gate(flags => flags.enableOwnerStacks) + ? { + // There's no owner + withoutStack: true, + } + : undefined, + ); expect(root).not.toMatchRenderedOutput('Hi'); }); @@ -996,10 +1004,7 @@ describe('ReactLazy', () => { await act(() => resolveFakeImport(Foo)); assertLog(['A', 'B']); }).toErrorDev( - ' in Text (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' : '') + + (gate(flags => flags.enableOwnerStacks) ? '' : ' in Text (at **)\n') + ' in Foo (at **)', ); expect(root).toMatchRenderedOutput(
AB
); diff --git a/packages/react-reconciler/src/__tests__/ReactMemo-test.js b/packages/react-reconciler/src/__tests__/ReactMemo-test.js index 4b7497893d..db2d2e2aeb 100644 --- a/packages/react-reconciler/src/__tests__/ReactMemo-test.js +++ b/packages/react-reconciler/src/__tests__/ReactMemo-test.js @@ -602,7 +602,7 @@ describe('memo', () => { 'Each child in a list should have a unique "key" prop. ' + 'See https://react.dev/link/warning-keys for more information.\n' + ' in span (at **)\n' + - ' in p (at **)', + ' in ', ); }); @@ -622,16 +622,16 @@ describe('memo', () => { '\n\nCheck the top-level render call using . It was passed a child from Inner. ' + 'See https://react.dev/link/warning-keys for more information.\n' + ' in span (at **)\n' + - ' in Inner (at **)\n' + - ' in p (at **)', + ' in Inner (at **)' + + (gate(flags => flags.enableOwnerStacks) ? '' : '\n in p (at **)'), ); }); - it('should use the inner displayName in the stack', async () => { + it('should use the inner name in the stack', async () => { const fn = (props, ref) => { return []; }; - fn.displayName = 'Inner'; + Object.defineProperty(fn, 'name', {value: 'Inner'}); const MemoComponent = React.memo(fn); ReactNoop.render(

@@ -645,8 +645,8 @@ describe('memo', () => { '\n\nCheck the top-level render call using . It was passed a child from Inner. ' + 'See https://react.dev/link/warning-keys for more information.\n' + ' in span (at **)\n' + - ' in Inner (at **)\n' + - ' in p (at **)', + ' in Inner (at **)' + + (gate(flags => flags.enableOwnerStacks) ? '' : '\n in p (at **)'), ); }); @@ -667,8 +667,8 @@ describe('memo', () => { '\n\nCheck the top-level render call using . It was passed a child from Outer. ' + 'See https://react.dev/link/warning-keys for more information.\n' + ' in span (at **)\n' + - ' in Outer (at **)\n' + - ' in p (at **)', + ' in Outer (at **)' + + (gate(flags => flags.enableOwnerStacks) ? '' : '\n in p (at **)'), ); }); @@ -676,7 +676,7 @@ describe('memo', () => { const fn = (props, ref) => { return []; }; - fn.displayName = 'Inner'; + Object.defineProperty(fn, 'name', {value: 'Inner'}); const MemoComponent = React.memo(fn); MemoComponent.displayName = 'Outer'; ReactNoop.render( @@ -691,8 +691,8 @@ describe('memo', () => { '\n\nCheck the top-level render call using . It was passed a child from Inner. ' + 'See https://react.dev/link/warning-keys for more information.\n' + ' in span (at **)\n' + - ' in Inner (at **)\n' + - ' in p (at **)', + ' in Inner (at **)' + + (gate(flags => flags.enableOwnerStacks) ? '' : '\n in p (at **)'), ); }); } diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index 029671bbda..6060e3ba60 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -752,6 +752,7 @@ function getCurrentStackInDEV(): string { if (currentTaskInDEV === null || currentTaskInDEV.componentStack === null) { return ''; } + // TODO: Support owner based stacks for logs during SSR. return getStackByComponentStackNode(currentTaskInDEV.componentStack); } return ''; diff --git a/packages/react/src/ReactForwardRef.js b/packages/react/src/ReactForwardRef.js index 3d86241430..ad9f5a9090 100644 --- a/packages/react/src/ReactForwardRef.js +++ b/packages/react/src/ReactForwardRef.js @@ -68,7 +68,9 @@ export function forwardRef( // React.forwardRef((props, ref) => {...}); // This kind of inner function is not used elsewhere so the side effect is okay. if (!render.name && !render.displayName) { - render.displayName = name; + Object.defineProperty(render, 'name', { + value: name, + }); } }, }); diff --git a/packages/react/src/ReactMemo.js b/packages/react/src/ReactMemo.js index 91f4864999..2948a28193 100644 --- a/packages/react/src/ReactMemo.js +++ b/packages/react/src/ReactMemo.js @@ -48,7 +48,9 @@ export function memo( // React.memo((props) => {...}); // This kind of inner function is not used elsewhere so the side effect is okay. if (!type.name && !type.displayName) { - type.displayName = name; + Object.defineProperty(type, 'name', { + value: name, + }); } }, }); diff --git a/packages/react/src/__tests__/ReactCreateRef-test.js b/packages/react/src/__tests__/ReactCreateRef-test.js index 34dc5aa3aa..616c62e00c 100644 --- a/packages/react/src/__tests__/ReactCreateRef-test.js +++ b/packages/react/src/__tests__/ReactCreateRef-test.js @@ -45,8 +45,10 @@ describe('ReactCreateRef', () => { ).toErrorDev( 'Unexpected ref object provided for div. ' + 'Use either a ref-setter function or React.createRef().\n' + - ' in div (at **)\n' + - ' in Wrapper (at **)', + ' in div (at **)' + + (gate(flags => flags.enableOwnerStacks) + ? '' + : '\n in Wrapper (at **)'), ); expect(() => @@ -60,8 +62,10 @@ describe('ReactCreateRef', () => { ).toErrorDev( 'Unexpected ref object provided for ExampleComponent. ' + 'Use either a ref-setter function or React.createRef().\n' + - ' in ExampleComponent (at **)\n' + - ' in Wrapper (at **)', + ' in ExampleComponent (at **)' + + (gate(flags => flags.enableOwnerStacks) + ? '' + : '\n in Wrapper (at **)'), ); }); }); diff --git a/packages/react/src/__tests__/ReactElementValidator-test.internal.js b/packages/react/src/__tests__/ReactElementValidator-test.internal.js index 6d133fc8d9..7cac9d7c83 100644 --- a/packages/react/src/__tests__/ReactElementValidator-test.internal.js +++ b/packages/react/src/__tests__/ReactElementValidator-test.internal.js @@ -142,11 +142,10 @@ describe('ReactElementValidator', () => { '"key" prop.\n\nCheck the render method of `Component`. See ' + 'https://react.dev/link/warning-keys for more information.\n' + ' in div (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 Component (at **)\n' + - ' in Parent (at **)\n' + + (gate(flags => flags.enableOwnerStacks) + ? '' + : ' in Parent (at **)\n') + ' in GrandParent (at **)', ); }); @@ -262,8 +261,6 @@ describe('ReactElementValidator', () => { 'Each child in a list should have a unique "key" prop.' + '\n\nCheck the render method of `ParentComp`. It was passed a child from MyComp. ' + 'See https://react.dev/link/warning-keys for more information.\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. ' in div (at **)\n' + ' in MyComp (at **)\n' + ' in ParentComp (at **)', diff --git a/packages/react/src/__tests__/ReactJSXElementValidator-test.js b/packages/react/src/__tests__/ReactJSXElementValidator-test.js index bbafbc7401..7ac4ead790 100644 --- a/packages/react/src/__tests__/ReactJSXElementValidator-test.js +++ b/packages/react/src/__tests__/ReactJSXElementValidator-test.js @@ -209,8 +209,6 @@ describe('ReactJSXElementValidator', () => { 'Each child in a list should have a unique "key" prop.' + '\n\nCheck the render method of `ParentComp`. It was passed a child from MyComp. ' + 'See https://react.dev/link/warning-keys for more information.\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. ' in div (at **)\n' + ' in MyComp (at **)\n' + ' in ParentComp (at **)', diff --git a/packages/react/src/__tests__/ReactJSXRuntime-test.js b/packages/react/src/__tests__/ReactJSXRuntime-test.js index ae26e133fb..6e6c111589 100644 --- a/packages/react/src/__tests__/ReactJSXRuntime-test.js +++ b/packages/react/src/__tests__/ReactJSXRuntime-test.js @@ -299,10 +299,9 @@ describe('ReactJSXRuntime', () => { }).toErrorDev( 'Warning: Each child in a list should have a unique "key" prop.\n\n' + 'Check the render method of `Parent`. See https://react.dev/link/warning-keys for more information.\n' + - ' in Child (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' : '') + + (gate(flags => flags.enableOwnerStacks) + ? '' + : ' in Child (at **)\n') + ' in Parent (at **)', ); }); diff --git a/packages/react/src/__tests__/createReactClassIntegration-test.js b/packages/react/src/__tests__/createReactClassIntegration-test.js index 0b65b14257..cfdc7174c7 100644 --- a/packages/react/src/__tests__/createReactClassIntegration-test.js +++ b/packages/react/src/__tests__/createReactClassIntegration-test.js @@ -594,6 +594,7 @@ describe('create-react-class-integration', () => { return null; }, }); + Component.displayName = 'Component'; await expect(async () => { await expect(async () => { @@ -643,6 +644,7 @@ describe('create-react-class-integration', () => { return null; }, }); + Component.displayName = 'Component'; await expect(async () => { await expect(async () => { diff --git a/packages/react/src/__tests__/forwardRef-test.js b/packages/react/src/__tests__/forwardRef-test.js index 96b0ee96d2..fdf6d20ae0 100644 --- a/packages/react/src/__tests__/forwardRef-test.js +++ b/packages/react/src/__tests__/forwardRef-test.js @@ -197,7 +197,7 @@ describe('forwardRef', () => { '\n\nCheck the top-level render call using . It was passed a child from ForwardRef. ' + 'See https://react.dev/link/warning-keys for more information.\n' + ' in span (at **)\n' + - ' in p (at **)', + ' in ', ); }); @@ -217,16 +217,16 @@ describe('forwardRef', () => { '\n\nCheck the top-level render call using . It was passed a child from ForwardRef(Inner). ' + 'See https://react.dev/link/warning-keys for more information.\n' + ' in span (at **)\n' + - ' in Inner (at **)\n' + - ' in p (at **)', + ' in Inner (at **)' + + (gate(flags => flags.enableOwnerStacks) ? '' : '\n in p (at **)'), ); }); - it('should use the inner displayName in the stack', async () => { + it('should use the inner name in the stack', async () => { const fn = (props, ref) => { return []; }; - fn.displayName = 'Inner'; + Object.defineProperty(fn, 'name', {value: 'Inner'}); const RefForwardingComponent = React.forwardRef(fn); ReactNoop.render(

@@ -240,8 +240,8 @@ describe('forwardRef', () => { '\n\nCheck the top-level render call using . It was passed a child from ForwardRef(Inner). ' + 'See https://react.dev/link/warning-keys for more information.\n' + ' in span (at **)\n' + - ' in Inner (at **)\n' + - ' in p (at **)', + ' in Inner (at **)' + + (gate(flags => flags.enableOwnerStacks) ? '' : '\n in p (at **)'), ); }); @@ -262,16 +262,16 @@ describe('forwardRef', () => { '\n\nCheck the top-level render call using . It was passed a child from Outer. ' + 'See https://react.dev/link/warning-keys for more information.\n' + ' in span (at **)\n' + - ' in Outer (at **)\n' + - ' in p (at **)', + ' in Outer (at **)' + + (gate(flags => flags.enableOwnerStacks) ? '' : '\n in p (at **)'), ); }); - it('should prefer the inner to the outer displayName in the stack', async () => { + it('should prefer the inner name to the outer displayName in the stack', async () => { const fn = (props, ref) => { return []; }; - fn.displayName = 'Inner'; + Object.defineProperty(fn, 'name', {value: 'Inner'}); const RefForwardingComponent = React.forwardRef(fn); RefForwardingComponent.displayName = 'Outer'; ReactNoop.render( @@ -286,8 +286,8 @@ describe('forwardRef', () => { '\n\nCheck the top-level render call using . It was passed a child from Outer. ' + 'See https://react.dev/link/warning-keys for more information.\n' + ' in span (at **)\n' + - ' in Inner (at **)\n' + - ' in p (at **)', + ' in Inner (at **)' + + (gate(flags => flags.enableOwnerStacks) ? '' : '\n in p (at **)'), ); }); diff --git a/packages/react/src/jsx/ReactJSXElement.js b/packages/react/src/jsx/ReactJSXElement.js index f76a84f899..a5a9880b05 100644 --- a/packages/react/src/jsx/ReactJSXElement.js +++ b/packages/react/src/jsx/ReactJSXElement.js @@ -492,7 +492,16 @@ export function jsxProdSignatureRunningInDevWithDynamicChildren( ) { if (__DEV__) { const isStaticChildren = false; - return jsxDEV(type, config, maybeKey, isStaticChildren, source, self); + return jsxDEVImpl( + type, + config, + maybeKey, + isStaticChildren, + source, + self, + __DEV__ && enableOwnerStacks ? Error('react-stack-top-frame') : undefined, + __DEV__ && enableOwnerStacks ? createTask(getTaskName(type)) : undefined, + ); } } @@ -505,7 +514,16 @@ export function jsxProdSignatureRunningInDevWithStaticChildren( ) { if (__DEV__) { const isStaticChildren = true; - return jsxDEV(type, config, maybeKey, isStaticChildren, source, self); + return jsxDEVImpl( + type, + config, + maybeKey, + isStaticChildren, + source, + self, + __DEV__ && enableOwnerStacks ? Error('react-stack-top-frame') : undefined, + __DEV__ && enableOwnerStacks ? createTask(getTaskName(type)) : undefined, + ); } } @@ -518,6 +536,28 @@ const didWarnAboutKeySpread = {}; * @param {string} key */ export function jsxDEV(type, config, maybeKey, isStaticChildren, source, self) { + return jsxDEVImpl( + type, + config, + maybeKey, + isStaticChildren, + source, + self, + __DEV__ && enableOwnerStacks ? Error('react-stack-top-frame') : undefined, + __DEV__ && enableOwnerStacks ? createTask(getTaskName(type)) : undefined, + ); +} + +function jsxDEVImpl( + type, + config, + maybeKey, + isStaticChildren, + source, + self, + debugStack, + debugTask, +) { if (__DEV__) { if (!isValidElementType(type)) { // This is an invalid element type. @@ -716,8 +756,8 @@ export function jsxDEV(type, config, maybeKey, isStaticChildren, source, self) { source, getOwner(), props, - __DEV__ && enableOwnerStacks ? Error('react-stack-top-frame') : undefined, - __DEV__ && enableOwnerStacks ? createTask(getTaskName(type)) : undefined, + debugStack, + debugTask, ); } } diff --git a/packages/shared/ReactElementType.js b/packages/shared/ReactElementType.js index 6136f1ac65..6860932176 100644 --- a/packages/shared/ReactElementType.js +++ b/packages/shared/ReactElementType.js @@ -9,10 +9,6 @@ import type {ReactDebugInfo} from './ReactTypes'; -interface ConsoleTask { - run(f: () => T): T; -} - export type ReactElement = { $$typeof: any, type: any, diff --git a/scripts/flow/environment.js b/scripts/flow/environment.js index 88683e7ee8..f71e5bba2a 100644 --- a/scripts/flow/environment.js +++ b/scripts/flow/environment.js @@ -29,6 +29,10 @@ declare module 'create-react-class' { declare const exports: React$CreateClass; } +declare interface ConsoleTask { + run(f: () => T): T; +} + // Flow hides the props of React$Element, this overrides it to unhide // them for React internals. // prettier-ignore diff --git a/scripts/jest/matchers/toWarnDev.js b/scripts/jest/matchers/toWarnDev.js index 50267cc201..1114b65dfa 100644 --- a/scripts/jest/matchers/toWarnDev.js +++ b/scripts/jest/matchers/toWarnDev.js @@ -16,6 +16,11 @@ function normalizeCodeLocInfo(str) { // React format: // in Component (at filename.js:123) return str.replace(/\n +(?:at|in) ([\S]+)[^\n]*/g, function (m, name) { + if (name.endsWith('.render')) { + // Class components will have the `render` method as part of their stack trace. + // We strip that out in our normalization to make it look more like component stacks. + name = name.slice(0, name.length - 7); + } return '\n in ' + name + ' (at **)'; }); }