From 13ddf1084b4304a60059e3b96fc3c039d23e9432 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Fri, 16 Aug 2024 19:52:11 -0400 Subject: [PATCH 1/2] [DevTools] Find owners from the parent path that matches the Fiber or ReactComponentInfo (#30717) This enables finding Server Components on the owner path. Server Components aren't stateful so there's not actually one specific owner that it necessarily matches. So it can't be a global look up. E.g. the same Server Component can be rendered in two places or even nested inside each other. Therefore we need to find an appropriate instance using a heuristic. We can do that by traversing the parent path since the owner is likely also a parent. Not always but almost always. To simplify things we can also do the same for Fibers. That brings us one step closer to being able to get rid of the global fiberToFiberInstance map since we can just use the shadow tree to find this information. This does mean that we can't find owners that aren't parents which is usually ok. However, there is a test case that's interesting where you have a React ART tree inside a DOM tree. In that case the owners actually span multiple renderers and roots so the owner is not on the parent stack. Usually this is fine since you'd just care about the owners within React ART but ideally we'd support this. However, I think that really the fix to this is that the React ART tree itself should actually show up inside the DOM tree in DevTools and in the virtual shadow tree because that's conceptually where it belongs. That would then solve this particular issue. We'd just need some way to associate the root with a DOM parent when it gets mounted. --- .../src/__tests__/inspectedElement-test.js | 43 ++-- .../src/backend/fiber/renderer.js | 208 ++++++++++++------ 2 files changed, 159 insertions(+), 92 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js index c77013aba3e71..843c6dff9c08f 100644 --- a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js +++ b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js @@ -2893,26 +2893,29 @@ describe('InspectedElement', () => { `); const inspectedElement = await inspectElementAtIndex(4); - expect(inspectedElement.owners).toMatchInlineSnapshot(` - [ - { - "compiledWithForget": false, - "displayName": "Child", - "hocDisplayNames": null, - "id": 8, - "key": null, - "type": 5, - }, - { - "compiledWithForget": false, - "displayName": "App", - "hocDisplayNames": null, - "id": 7, - "key": null, - "type": 5, - }, - ] - `); + // TODO: Ideally this should match the owners of the Group but those are + // part of a different parent tree. Ideally the Group would be parent of + // that parent tree though which would fix this issue. + // + // [ + // { + // "compiledWithForget": false, + // "displayName": "Child", + // "hocDisplayNames": null, + // "id": 8, + // "key": null, + // "type": 5, + // }, + // { + // "compiledWithForget": false, + // "displayName": "App", + // "hocDisplayNames": null, + // "id": 7, + // "key": null, + // "type": 5, + // }, + // ] + expect(inspectedElement.owners).toMatchInlineSnapshot(`[]`); }); describe('error boundary', () => { diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index a48b22f647548..62eacdf5dbe9a 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -2143,29 +2143,18 @@ export function attach( const {key} = fiber; const displayName = getDisplayNameForFiber(fiber); const elementType = getElementTypeForFiber(fiber); - const debugOwner = fiber._debugOwner; - - // Ideally we should call getFiberIDThrows() for _debugOwner, - // since owners are almost always higher in the tree (and so have already been processed), - // but in some (rare) instances reported in open source, a descendant mounts before an owner. - // Since this is a DEV only field it's probably okay to also just lazily generate and ID here if needed. - // See https://github.com/facebook/react/issues/21445 - let ownerID: number; - if (debugOwner != null) { - if (typeof debugOwner.tag === 'number') { - const ownerFiberInstance = getFiberInstanceUnsafe((debugOwner: any)); - if (ownerFiberInstance !== null) { - ownerID = ownerFiberInstance.id; - } else { - ownerID = 0; - } - } else { - // TODO: Track Server Component Owners. - ownerID = 0; - } - } else { - ownerID = 0; - } + + // Finding the owner instance might require traversing the whole parent path which + // doesn't have great big O notation. Ideally we'd lazily fetch the owner when we + // need it but we have some synchronous operations in the front end like Alt+Left + // which selects the owner immediately. Typically most owners are only a few parents + // away so maybe it's not so bad. + const debugOwner = getUnfilteredOwner(fiber); + const ownerInstance = findNearestOwnerInstance( + parentInstance, + debugOwner, + ); + const ownerID = ownerInstance === null ? 0 : ownerInstance.id; const parentID = parentInstance ? parentInstance.id : 0; const displayNameStringID = getStringID(displayName); @@ -2231,11 +2220,15 @@ export function attach( displayName = env + '(' + displayName + ')'; } const elementType = ElementTypeVirtual; - // TODO: Support Virtual Owners. To do this we need to find a matching - // virtual instance which is not a super cheap parent traversal and so - // we should ideally only do that lazily. We should maybe change the - // frontend to get it lazily. - const ownerID: number = 0; + + // Finding the owner instance might require traversing the whole parent path which + // doesn't have great big O notation. Ideally we'd lazily fetch the owner when we + // need it but we have some synchronous operations in the front end like Alt+Left + // which selects the owner immediately. Typically most owners are only a few parents + // away so maybe it's not so bad. + const debugOwner = getUnfilteredOwner(componentInfo); + const ownerInstance = findNearestOwnerInstance(parentInstance, debugOwner); + const ownerID = ownerInstance === null ? 0 : ownerInstance.id; const parentID = parentInstance ? parentInstance.id : 0; const displayNameStringID = getStringID(displayName); @@ -3354,11 +3347,19 @@ export function attach( } function getUpdatersList(root: any): Array | null { - return root.memoizedUpdaters != null - ? Array.from(root.memoizedUpdaters) - .filter(fiber => getFiberIDUnsafe(fiber) !== null) - .map(fiberToSerializedElement) - : null; + const updaters = root.memoizedUpdaters; + if (updaters == null) { + return null; + } + const result = []; + // eslint-disable-next-line no-for-of-loops/no-for-of-loops + for (const updater of updaters) { + const inst = getFiberInstanceUnsafe(updater); + if (inst !== null) { + result.push(instanceToSerializedElement(inst)); + } + } + return result; } function handleCommitFiberUnmount(fiber: any) { @@ -3923,13 +3924,26 @@ export function attach( } } - function fiberToSerializedElement(fiber: Fiber): SerializedElement { - return { - displayName: getDisplayNameForFiber(fiber) || 'Anonymous', - id: getFiberIDThrows(fiber), - key: fiber.key, - type: getElementTypeForFiber(fiber), - }; + function instanceToSerializedElement( + instance: DevToolsInstance, + ): SerializedElement { + if (instance.kind === FIBER_INSTANCE) { + const fiber = instance.data; + return { + displayName: getDisplayNameForFiber(fiber) || 'Anonymous', + id: instance.id, + key: fiber.key, + type: getElementTypeForFiber(fiber), + }; + } else { + const componentInfo = instance.data; + return { + displayName: componentInfo.name || 'Anonymous', + id: instance.id, + key: componentInfo.key == null ? null : componentInfo.key, + type: ElementTypeVirtual, + }; + } } function getOwnersList(id: number): Array | null { @@ -3938,33 +3952,97 @@ export function attach( console.warn(`Could not find DevToolsInstance with id "${id}"`); return null; } - if (devtoolsInstance.kind !== FIBER_INSTANCE) { - // TODO: Handle VirtualInstance. - return null; + const self = instanceToSerializedElement(devtoolsInstance); + const owners = getOwnersListFromInstance(devtoolsInstance); + // This is particular API is prefixed with the current instance too for some reason. + if (owners === null) { + return [self]; } - const fiber = - findCurrentFiberUsingSlowPathByFiberInstance(devtoolsInstance); - if (fiber == null) { + owners.unshift(self); + owners.reverse(); + return owners; + } + + function getOwnersListFromInstance( + instance: DevToolsInstance, + ): Array | null { + let owner = getUnfilteredOwner(instance.data); + if (owner === null) { return null; } + const owners: Array = []; + let parentInstance: null | DevToolsInstance = instance.parent; + while (parentInstance !== null && owner !== null) { + const ownerInstance = findNearestOwnerInstance(parentInstance, owner); + if (ownerInstance !== null) { + owners.push(instanceToSerializedElement(ownerInstance)); + // Get the next owner and keep searching from the previous match. + owner = getUnfilteredOwner(owner); + parentInstance = ownerInstance.parent; + } else { + break; + } + } + return owners; + } - const owners: Array = [fiberToSerializedElement(fiber)]; - - let owner = fiber._debugOwner; - while (owner != null) { + function getUnfilteredOwner( + owner: ReactComponentInfo | Fiber | null | void, + ): ReactComponentInfo | Fiber | null { + if (owner == null) { + return null; + } + if (typeof owner.tag === 'number') { + const ownerFiber: Fiber = (owner: any); // Refined + owner = ownerFiber._debugOwner; + } else { + const ownerInfo: ReactComponentInfo = (owner: any); // Refined + owner = ownerInfo.owner; + } + while (owner) { if (typeof owner.tag === 'number') { const ownerFiber: Fiber = (owner: any); // Refined if (!shouldFilterFiber(ownerFiber)) { - owners.unshift(fiberToSerializedElement(ownerFiber)); + return ownerFiber; } owner = ownerFiber._debugOwner; } else { - // TODO: Track Server Component Owners. - break; + const ownerInfo: ReactComponentInfo = (owner: any); // Refined + if (!shouldFilterVirtual(ownerInfo)) { + return ownerInfo; + } + owner = ownerInfo.owner; } } + return null; + } - return owners; + function findNearestOwnerInstance( + parentInstance: null | DevToolsInstance, + owner: void | null | ReactComponentInfo | Fiber, + ): null | DevToolsInstance { + if (owner == null) { + return null; + } + // Search the parent path for any instance that matches this kind of owner. + while (parentInstance !== null) { + if ( + parentInstance.data === owner || + // Typically both owner and instance.data would refer to the current version of a Fiber + // but it is possible for memoization to ignore the owner on the JSX. Then the new Fiber + // isn't propagated down as the new owner. In that case we might match the alternate + // instead. This is a bit hacky but the fastest check since type casting owner to a Fiber + // needs a duck type check anyway. + parentInstance.data === (owner: any).alternate + ) { + return parentInstance; + } + parentInstance = parentInstance.parent; + } + // It is technically possible to create an element and render it in a different parent + // but this is a weird edge case and it is worth not having to scan the tree or keep + // a register for every fiber/component info. + return null; } // Fast path props lookup for React Native style editor. @@ -4047,7 +4125,6 @@ export function attach( } const { - _debugOwner: debugOwner, stateNode, key, memoizedProps, @@ -4174,21 +4251,8 @@ export function attach( context = {value: context}; } - let owners: null | Array = null; - let owner = debugOwner; - while (owner != null) { - if (typeof owner.tag === 'number') { - const ownerFiber: Fiber = (owner: any); // Refined - if (owners === null) { - owners = []; - } - owners.push(fiberToSerializedElement(ownerFiber)); - owner = ownerFiber._debugOwner; - } else { - // TODO: Track Server Component Owners. - break; - } - } + const owners: null | Array = + getOwnersListFromInstance(fiberInstance); const isTimedOutSuspense = tag === SuspenseComponent && memoizedState !== null; @@ -4352,8 +4416,8 @@ export function attach( displayName = env + '(' + displayName + ')'; } - // TODO: Support Virtual Owners. - const owners: null | Array = null; + const owners: null | Array = + getOwnersListFromInstance(virtualInstance); let rootType = null; let targetErrorBoundaryID = null; From 177b2419b2d8a3d14c3f3304bb7e300985d6f377 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 16 Aug 2024 16:45:03 -0700 Subject: [PATCH 2/2] [compiler] Validate environment config while parsing plugin opts Addresses a todo from a while back. We now validate environment options when parsing the plugin options, which means we can stop re-parsing/validating in later phases. ghstack-source-id: b19806e843e1254716705b33dcf86afb7223f6c7 Pull Request resolved: https://github.com/facebook/react/pull/30726 --- .../src/Entrypoint/Options.ts | 26 +++++++++++++++---- .../src/Entrypoint/Program.ts | 17 +----------- 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts index e97ececc2a137..e966497256511 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts @@ -7,8 +7,12 @@ import * as t from '@babel/types'; import {z} from 'zod'; -import {CompilerErrorDetailOptions} from '../CompilerError'; -import {ExternalFunction, PartialEnvironmentConfig} from '../HIR/Environment'; +import {CompilerError, CompilerErrorDetailOptions} from '../CompilerError'; +import { + EnvironmentConfig, + ExternalFunction, + parseEnvironmentConfig, +} from '../HIR/Environment'; import {hasOwnProperty} from '../Utils/utils'; const PanicThresholdOptionsSchema = z.enum([ @@ -32,7 +36,7 @@ const PanicThresholdOptionsSchema = z.enum([ export type PanicThresholdOptions = z.infer; export type PluginOptions = { - environment: PartialEnvironmentConfig | null; + environment: EnvironmentConfig; logger: Logger | null; @@ -194,7 +198,7 @@ export type Logger = { export const defaultOptions: PluginOptions = { compilationMode: 'infer', panicThreshold: 'none', - environment: {}, + environment: parseEnvironmentConfig({}).unwrap(), logger: null, gating: null, noEmit: false, @@ -218,7 +222,19 @@ export function parsePluginOptions(obj: unknown): PluginOptions { // normalize string configs to be case insensitive value = value.toLowerCase(); } - if (isCompilerFlag(key)) { + if (key === 'environment') { + const environmentResult = parseEnvironmentConfig(value); + if (environmentResult.isErr()) { + CompilerError.throwInvalidConfig({ + reason: + 'Error in validating environment config. This is an advanced setting and not meant to be used directly', + description: environmentResult.unwrapErr().toString(), + suggestions: null, + loc: null, + }); + } + parsedOptions[key] = environmentResult.unwrap(); + } else if (isCompilerFlag(key)) { parsedOptions[key] = value; } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts index 99ec1e04a65e4..979e9f88d1b57 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts @@ -16,7 +16,6 @@ import { EnvironmentConfig, ExternalFunction, ReactFunctionType, - parseEnvironmentConfig, tryParseExternalFunction, } from '../HIR/Environment'; import {CodegenFunction} from '../ReactiveScopes'; @@ -292,21 +291,7 @@ export function compileProgram( return; } - /* - * TODO(lauren): Remove pass.opts.environment nullcheck once PluginOptions - * is validated - */ - const environmentResult = parseEnvironmentConfig(pass.opts.environment ?? {}); - if (environmentResult.isErr()) { - CompilerError.throwInvalidConfig({ - reason: - 'Error in validating environment config. This is an advanced setting and not meant to be used directly', - description: environmentResult.unwrapErr().toString(), - suggestions: null, - loc: null, - }); - } - const environment = environmentResult.unwrap(); + const environment = pass.opts.environment; const restrictedImportsErr = validateRestrictedImports(program, environment); if (restrictedImportsErr) { handleError(restrictedImportsErr, pass, null);