From ba6866326a0b2069ec815fb31907aff995f4973a Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Thu, 22 Aug 2024 21:38:23 -0400 Subject: [PATCH 1/2] Optimize getSourceForFiber Skip intermediate maps. Store result on instance instead of itermediate stack. Skip generating stack frames for parents. This means that we don't jump to the source of nearest parent if this node doesn't have a stack. That was already a bug / weird. --- .../fiber/DevToolsFiberComponentStack.js | 17 ++++++ .../src/backend/fiber/renderer.js | 58 ++++++++----------- 2 files changed, 40 insertions(+), 35 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/fiber/DevToolsFiberComponentStack.js b/packages/react-devtools-shared/src/backend/fiber/DevToolsFiberComponentStack.js index 5a1fc4ee5337d..17475cab2860f 100644 --- a/packages/react-devtools-shared/src/backend/fiber/DevToolsFiberComponentStack.js +++ b/packages/react-devtools-shared/src/backend/fiber/DevToolsFiberComponentStack.js @@ -108,6 +108,23 @@ export function getStackByFiberInDevAndProd( } } +export function getSourceLocationByFiber( + workTagMap: WorkTagMap, + fiber: Fiber, + currentDispatcherRef: CurrentDispatcherRef, +): null | string { + // This is like getStackByFiberInDevAndProd but just the first stack frame. + try { + const info = describeFiber(workTagMap, fiber, currentDispatcherRef); + if (info !== '') { + return info.slice(1); // skip the leading newline + } + } catch (x) { + console.error(x); + } + return null; +} + export function supportsConsoleTasks(fiber: Fiber): boolean { // If this Fiber supports native console.createTask then we are already running // inside a native async stack trace if it's active - meaning the DevTools is open. diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index 62eacdf5dbe9a..212cb8609bf90 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -131,7 +131,7 @@ import type { Plugins, } from 'react-devtools-shared/src/frontend/types'; import type {Source} from 'react-devtools-shared/src/shared/types'; -import {getStackByFiberInDevAndProd} from './DevToolsFiberComponentStack'; +import {getSourceLocationByFiber} from './DevToolsFiberComponentStack'; // Kinds const FIBER_INSTANCE = 0; @@ -152,7 +152,7 @@ type FiberInstance = { previousSibling: null | DevToolsInstance, // filtered next sibling, including virtual nextSibling: null | DevToolsInstance, // filtered next sibling, including virtual flags: number, // Force Error/Suspense - componentStack: null | string, + source: null | Source, // the source location of this component function errors: null | Map, // error messages and count warnings: null | Map, // warning messages and count data: Fiber, // one of a Fiber pair @@ -167,7 +167,7 @@ function createFiberInstance(fiber: Fiber): FiberInstance { previousSibling: null, nextSibling: null, flags: 0, - componentStack: null, + source: null, errors: null, warnings: null, data: fiber, @@ -187,7 +187,7 @@ type VirtualInstance = { previousSibling: null | DevToolsInstance, // filtered next sibling, including virtual nextSibling: null | DevToolsInstance, // filtered next sibling, including virtual flags: number, - componentStack: null | string, + source: null, // Errors and Warnings happen per ReactComponentInfo which can appear in // multiple places but we track them per stateful VirtualInstance so // that old errors/warnings don't disappear when the instance is refreshed. @@ -209,7 +209,7 @@ function createVirtualInstance( previousSibling: null, nextSibling: null, flags: 0, - componentStack: null, + source: null, errors: null, warnings: null, data: debugEntry, @@ -4328,7 +4328,7 @@ export function attach( let source = null; if (canViewSource) { - source = getSourceForFiber(fiber); + source = getSourceForFiberInstance(fiberInstance); } return { @@ -5668,39 +5668,27 @@ export function attach( return idToDevToolsInstanceMap.has(id); } - function getComponentStackForFiber(fiber: Fiber): string | null { - // TODO: This should really just take an DevToolsInstance directly. - let fiberInstance = fiberToFiberInstanceMap.get(fiber); - if (fiberInstance === undefined && fiber.alternate !== null) { - fiberInstance = fiberToFiberInstanceMap.get(fiber.alternate); - } - if (fiberInstance === undefined) { - // We're no longer tracking this instance. - return null; - } - if (fiberInstance.componentStack !== null) { - // Cached entry. - return fiberInstance.componentStack; + function getSourceForFiberInstance( + fiberInstance: FiberInstance, + ): Source | null { + if (fiberInstance.source !== null) { + return fiberInstance.source; } const dispatcherRef = getDispatcherRef(renderer); - if (dispatcherRef == null) { - return null; - } - - return (fiberInstance.componentStack = getStackByFiberInDevAndProd( - ReactTypeOfWork, - fiber, - dispatcherRef, - )); - } - - function getSourceForFiber(fiber: Fiber): Source | null { - const componentStack = getComponentStackForFiber(fiber); - if (componentStack == null) { + const stackFrame = + dispatcherRef == null + ? null + : getSourceLocationByFiber( + ReactTypeOfWork, + fiberInstance.data, + dispatcherRef, + ); + if (stackFrame === null) { return null; } - - return parseSourceFromComponentStack(componentStack); + const source = parseSourceFromComponentStack(stackFrame); + fiberInstance.source = source; + return source; } return { From b6229507d2f7939ea5ddb9253424360e98cbe672 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Thu, 22 Aug 2024 22:26:45 -0400 Subject: [PATCH 2/2] Get the source location from the first owned child's stack frame If JSX is an owner of something then it must have the owner on the stack. There could be an edge case like if it's a bound function without a stack but in general it'll have a stack frame. We can use this as the source location of the component itself. --- .../src/backend/fiber/renderer.js | 81 ++++++++++++++++--- 1 file changed, 72 insertions(+), 9 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index 212cb8609bf90..15844d5d3dd22 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -101,6 +101,14 @@ import { import {enableStyleXFeatures} from 'react-devtools-feature-flags'; import is from 'shared/objectIs'; import hasOwnProperty from 'shared/hasOwnProperty'; + +// $FlowFixMe[method-unbinding] +const toString = Object.prototype.toString; + +function isError(object: mixed) { + return toString.call(object) === '[object Error]'; +} + import {getStyleXData} from '../StyleX/utils'; import {createProfilingHooks} from '../profilingHooks'; @@ -132,6 +140,7 @@ import type { } from 'react-devtools-shared/src/frontend/types'; import type {Source} from 'react-devtools-shared/src/shared/types'; import {getSourceLocationByFiber} from './DevToolsFiberComponentStack'; +import {formatOwnerStack} from '../shared/DevToolsOwnerStack'; // Kinds const FIBER_INSTANCE = 0; @@ -152,7 +161,7 @@ type FiberInstance = { previousSibling: null | DevToolsInstance, // filtered next sibling, including virtual nextSibling: null | DevToolsInstance, // filtered next sibling, including virtual flags: number, // Force Error/Suspense - source: null | Source, // the source location of this component function + source: null | string | Error | Source, // source location of this component function, or owned child stack errors: null | Map, // error messages and count warnings: null | Map, // warning messages and count data: Fiber, // one of a Fiber pair @@ -187,7 +196,7 @@ type VirtualInstance = { previousSibling: null | DevToolsInstance, // filtered next sibling, including virtual nextSibling: null | DevToolsInstance, // filtered next sibling, including virtual flags: number, - source: null, + source: null | string | Error | Source, // source location of this server component, or owned child stack // Errors and Warnings happen per ReactComponentInfo which can appear in // multiple places but we track them per stateful VirtualInstance so // that old errors/warnings don't disappear when the instance is refreshed. @@ -2154,6 +2163,16 @@ export function attach( parentInstance, debugOwner, ); + if ( + ownerInstance !== null && + debugOwner === fiber._debugOwner && + fiber._debugStack != null && + ownerInstance.source === null + ) { + // The new Fiber is directly owned by the ownerInstance. Therefore somewhere on + // the debugStack will be a stack frame inside the ownerInstance's source. + ownerInstance.source = fiber._debugStack; + } const ownerID = ownerInstance === null ? 0 : ownerInstance.id; const parentID = parentInstance ? parentInstance.id : 0; @@ -2228,6 +2247,16 @@ export function attach( // away so maybe it's not so bad. const debugOwner = getUnfilteredOwner(componentInfo); const ownerInstance = findNearestOwnerInstance(parentInstance, debugOwner); + if ( + ownerInstance !== null && + debugOwner === componentInfo.owner && + componentInfo.debugStack != null && + ownerInstance.source === null + ) { + // The new Fiber is directly owned by the ownerInstance. Therefore somewhere on + // the debugStack will be a stack frame inside the ownerInstance's source. + ownerInstance.source = componentInfo.debugStack; + } const ownerID = ownerInstance === null ? 0 : ownerInstance.id; const parentID = parentInstance ? parentInstance.id : 0; @@ -4402,7 +4431,8 @@ export function attach( function inspectVirtualInstanceRaw( virtualInstance: VirtualInstance, ): InspectedElement | null { - const canViewSource = false; + const canViewSource = true; + const source = getSourceForInstance(virtualInstance); const componentInfo = virtualInstance.data; const key = @@ -4442,9 +4472,6 @@ export function attach( stylex: null, }; - // TODO: Support getting the source location from the owner stack. - const source = null; - return { id: virtualInstance.id, @@ -5671,8 +5698,14 @@ export function attach( function getSourceForFiberInstance( fiberInstance: FiberInstance, ): Source | null { - if (fiberInstance.source !== null) { - return fiberInstance.source; + const unresolvedSource = fiberInstance.source; + if ( + unresolvedSource !== null && + typeof unresolvedSource === 'object' && + !isError(unresolvedSource) + ) { + // $FlowFixMe: isError should have refined it. + return unresolvedSource; } const dispatcherRef = getDispatcherRef(renderer); const stackFrame = @@ -5684,13 +5717,43 @@ export function attach( dispatcherRef, ); if (stackFrame === null) { - return null; + // If we don't find a source location by throwing, try to get one + // from an owned child if possible. This is the same branch as + // for virtual instances. + return getSourceForInstance(fiberInstance); } const source = parseSourceFromComponentStack(stackFrame); fiberInstance.source = source; return source; } + function getSourceForInstance(instance: DevToolsInstance): Source | null { + let unresolvedSource = instance.source; + if (unresolvedSource === null) { + // We don't have any source yet. We can try again later in case an owned child mounts later. + // TODO: We won't have any information here if the child is filtered. + return null; + } + + // If we have the debug stack (the creation stack of the JSX) for any owned child of this + // component, then at the bottom of that stack will be a stack frame that is somewhere within + // the component's function body. Typically it would be the callsite of the JSX unless there's + // any intermediate utility functions. This won't point to the top of the component function + // but it's at least somewhere within it. + if (isError(unresolvedSource)) { + unresolvedSource = formatOwnerStack((unresolvedSource: any)); + } + if (typeof unresolvedSource === 'string') { + const idx = unresolvedSource.lastIndexOf('\n'); + const lastLine = + idx === -1 ? unresolvedSource : unresolvedSource.slice(idx + 1); + return (instance.source = parseSourceFromComponentStack(lastLine)); + } + + // $FlowFixMe: refined. + return unresolvedSource; + } + return { cleanup, clearErrorsAndWarnings,