From f800befa79727fc2950530143260327a86755f36 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sun, 28 Jul 2024 15:46:11 -0400 Subject: [PATCH] Implement "best renderer" by taking the inner most matched node Don't leak the "Fiber" type outside the renderer interface. --- .../__tests__/legacy/storeLegacy-v15-test.js | 2 +- .../src/__tests__/store-test.js | 2 +- .../src/backend/agent.js | 84 +++++++++++++++---- .../src/backend/fiber/renderer.js | 12 ++- .../src/backend/legacy/renderer.js | 17 +++- .../src/backend/types.js | 9 +- .../src/backend/views/Highlighter/Overlay.js | 16 +--- .../src/backend/views/Highlighter/index.js | 2 +- 8 files changed, 98 insertions(+), 46 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/legacy/storeLegacy-v15-test.js b/packages/react-devtools-shared/src/__tests__/legacy/storeLegacy-v15-test.js index f912638fb508e..27c841c2b75d0 100644 --- a/packages/react-devtools-shared/src/__tests__/legacy/storeLegacy-v15-test.js +++ b/packages/react-devtools-shared/src/__tests__/legacy/storeLegacy-v15-test.js @@ -905,7 +905,7 @@ describe('Store (legacy)', () => { [root] ▸ `); - const deepestedNodeID = global.agent.getIDForNode(ref); + const deepestedNodeID = global.agent.getIDForHostInstance(ref); act(() => store.toggleIsCollapsed(deepestedNodeID, false)); expect(store).toMatchInlineSnapshot(` [root] diff --git a/packages/react-devtools-shared/src/__tests__/store-test.js b/packages/react-devtools-shared/src/__tests__/store-test.js index 75435114975c0..dea60778d5884 100644 --- a/packages/react-devtools-shared/src/__tests__/store-test.js +++ b/packages/react-devtools-shared/src/__tests__/store-test.js @@ -1217,7 +1217,7 @@ describe('Store', () => { ▸ `); - const deepestedNodeID = agent.getIDForNode(ref.current); + const deepestedNodeID = agent.getIDForHostInstance(ref.current); act(() => store.toggleIsCollapsed(deepestedNodeID, false)); expect(store).toMatchInlineSnapshot(` diff --git a/packages/react-devtools-shared/src/backend/agent.js b/packages/react-devtools-shared/src/backend/agent.js index a3d579bd65d64..886435c289f61 100644 --- a/packages/react-devtools-shared/src/backend/agent.js +++ b/packages/react-devtools-shared/src/backend/agent.js @@ -43,7 +43,7 @@ import type { ComponentFilter, BrowserTheme, } from 'react-devtools-shared/src/frontend/types'; -import {isSynchronousXHRSupported} from './utils'; +import {isSynchronousXHRSupported, isReactNativeEnvironment} from './utils'; const debug = (methodName: string, ...args: Array) => { if (__DEBUG__) { @@ -343,31 +343,79 @@ export default class Agent extends EventEmitter<{ return renderer.getInstanceAndStyle(id); } - getBestMatchingRendererInterface(node: Object): RendererInterface | null { - let bestMatch = null; + getIDForHostInstance(target: HostInstance): number | null { + let bestMatch: null | HostInstance = null; + let bestRenderer: null | RendererInterface = null; + // Find the nearest ancestor which is mounted by a React. for (const rendererID in this._rendererInterfaces) { const renderer = ((this._rendererInterfaces[ (rendererID: any) ]: any): RendererInterface); - const fiber = renderer.getFiberForNative(node); - if (fiber !== null) { - // check if fiber.stateNode is matching the original hostInstance - if (fiber.stateNode === node) { - return renderer; - } else if (bestMatch === null) { - bestMatch = renderer; + const nearestNode: null = renderer.getNearestMountedHostInstance(target); + if (nearestNode !== null) { + if (nearestNode === target) { + // Exact match we can exit early. + bestMatch = nearestNode; + bestRenderer = renderer; + break; + } + if ( + bestMatch === null || + (!isReactNativeEnvironment() && bestMatch.contains(nearestNode)) + ) { + // If this is the first match or the previous match contains the new match, + // so the new match is a deeper and therefore better match. + bestMatch = nearestNode; + bestRenderer = renderer; } } } - // if an exact match is not found, return the first valid renderer as fallback - return bestMatch; + if (bestRenderer != null && bestMatch != null) { + try { + return bestRenderer.getElementIDForHostInstance(bestMatch, true); + } catch (error) { + // Some old React versions might throw if they can't find a match. + // If so we should ignore it... + } + } + return null; } - getIDForNode(node: Object): number | null { - const rendererInterface = this.getBestMatchingRendererInterface(node); - if (rendererInterface != null) { + getComponentNameForHostInstance(target: HostInstance): string | null { + // We duplicate this code from getIDForHostInstance to avoid an object allocation. + let bestMatch: null | HostInstance = null; + let bestRenderer: null | RendererInterface = null; + // Find the nearest ancestor which is mounted by a React. + for (const rendererID in this._rendererInterfaces) { + const renderer = ((this._rendererInterfaces[ + (rendererID: any) + ]: any): RendererInterface); + const nearestNode = renderer.getNearestMountedHostInstance(target); + if (nearestNode !== null) { + if (nearestNode === target) { + // Exact match we can exit early. + bestMatch = nearestNode; + bestRenderer = renderer; + break; + } + if ( + bestMatch === null || + (!isReactNativeEnvironment() && bestMatch.contains(nearestNode)) + ) { + // If this is the first match or the previous match contains the new match, + // so the new match is a deeper and therefore better match. + bestMatch = nearestNode; + bestRenderer = renderer; + } + } + } + + if (bestRenderer != null && bestMatch != null) { try { - return rendererInterface.getElementIDForHostInstance(node, true); + const id = bestRenderer.getElementIDForHostInstance(bestMatch, true); + if (id) { + return bestRenderer.getDisplayNameForElementID(id); + } } catch (error) { // Some old React versions might throw if they can't find a match. // If so we should ignore it... @@ -616,8 +664,8 @@ export default class Agent extends EventEmitter<{ } }; - selectNode(target: Object): void { - const id = this.getIDForNode(target); + selectNode(target: HostInstance): void { + const id = this.getIDForHostInstance(target); if (id !== null) { this._bridge.send('selectElement', id); } diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index 9b46d262f993d..0b48effe1902d 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -2882,8 +2882,14 @@ export function attach( return fiber != null ? getDisplayNameForFiber(fiber) : null; } - function getFiberForNative(hostInstance: HostInstance) { - return renderer.findFiberByHostInstance(hostInstance); + function getNearestMountedHostInstance( + hostInstance: HostInstance, + ): null | HostInstance { + const mountedHostInstance = renderer.findFiberByHostInstance(hostInstance); + if (mountedHostInstance != null) { + return mountedHostInstance.stateNode; + } + return null; } function getElementIDForHostInstance( @@ -4659,7 +4665,7 @@ export function attach( flushInitialOperations, getBestMatchForTrackedPath, getDisplayNameForElementID, - getFiberForNative, + getNearestMountedHostInstance, getElementIDForHostInstance, getInstanceAndStyle, getOwnersList, diff --git a/packages/react-devtools-shared/src/backend/legacy/renderer.js b/packages/react-devtools-shared/src/backend/legacy/renderer.js index 351ce64919e54..1955465607d2c 100644 --- a/packages/react-devtools-shared/src/backend/legacy/renderer.js +++ b/packages/react-devtools-shared/src/backend/legacy/renderer.js @@ -145,7 +145,9 @@ export function attach( let getElementIDForHostInstance: GetElementIDForHostInstance = ((null: any): GetElementIDForHostInstance); let findHostInstanceForInternalID: (id: number) => ?HostInstance; - let getFiberForNative = (node: HostInstance) => { + let getNearestMountedHostInstance = ( + node: HostInstance, + ): null | HostInstance => { // Not implemented. return null; }; @@ -160,8 +162,15 @@ export function attach( const internalInstance = idToInternalInstanceMap.get(id); return renderer.ComponentTree.getNodeFromInstance(internalInstance); }; - getFiberForNative = (node: HostInstance) => { - return renderer.ComponentTree.getClosestInstanceFromNode(node); + getNearestMountedHostInstance = ( + node: HostInstance, + ): null | HostInstance => { + const internalInstance = + renderer.ComponentTree.getClosestInstanceFromNode(node); + if (internalInstance != null) { + return renderer.ComponentTree.getNodeFromInstance(internalInstance); + } + return null; }; } else if (renderer.Mount.getID && renderer.Mount.getNode) { getElementIDForHostInstance = (node, findNearestUnfilteredAncestor) => { @@ -1111,7 +1120,7 @@ export function attach( flushInitialOperations, getBestMatchForTrackedPath, getDisplayNameForElementID, - getFiberForNative, + getNearestMountedHostInstance, getElementIDForHostInstance, getInstanceAndStyle, findHostInstancesForElementID: (id: number) => { diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index f08e53416dfbf..fdcfddea5fb6b 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -86,10 +86,7 @@ type SharedInternalsSubset = { }; export type CurrentDispatcherRef = SharedInternalsSubset; -export type GetDisplayNameForElementID = ( - id: number, - findNearestUnfilteredAncestor?: boolean, -) => string | null; +export type GetDisplayNameForElementID = (id: number) => string | null; export type GetElementIDForHostInstance = ( component: HostInstance, @@ -363,7 +360,9 @@ export type RendererInterface = { findHostInstancesForElementID: FindHostInstancesForElementID, flushInitialOperations: () => void, getBestMatchForTrackedPath: () => PathMatch | null, - getFiberForNative: (component: HostInstance) => Fiber | null, + getNearestMountedHostInstance: ( + component: HostInstance, + ) => HostInstance | null, getElementIDForHostInstance: GetElementIDForHostInstance, getDisplayNameForElementID: GetDisplayNameForElementID, getInstanceAndStyle(id: number): InstanceAndStyle, diff --git a/packages/react-devtools-shared/src/backend/views/Highlighter/Overlay.js b/packages/react-devtools-shared/src/backend/views/Highlighter/Overlay.js index 78ec72d084a0a..fe0a40d8e9c2d 100644 --- a/packages/react-devtools-shared/src/backend/views/Highlighter/Overlay.js +++ b/packages/react-devtools-shared/src/backend/views/Highlighter/Overlay.js @@ -233,19 +233,9 @@ export default class Overlay { name = elements[0].nodeName.toLowerCase(); const node = elements[0]; - const rendererInterface = - this.agent.getBestMatchingRendererInterface(node); - if (rendererInterface) { - const id = rendererInterface.getElementIDForHostInstance(node, true); - if (id) { - const ownerName = rendererInterface.getDisplayNameForElementID( - id, - true, - ); - if (ownerName) { - name += ' (in ' + ownerName + ')'; - } - } + const ownerName = this.agent.getComponentNameForHostInstance(node); + if (ownerName) { + name += ' (in ' + ownerName + ')'; } } diff --git a/packages/react-devtools-shared/src/backend/views/Highlighter/index.js b/packages/react-devtools-shared/src/backend/views/Highlighter/index.js index 3a5429ab93e01..dc711d7881bd4 100644 --- a/packages/react-devtools-shared/src/backend/views/Highlighter/index.js +++ b/packages/react-devtools-shared/src/backend/views/Highlighter/index.js @@ -193,7 +193,7 @@ export default function setupHighlighter( const selectElementForNode = throttle( memoize((node: HTMLElement) => { - const id = agent.getIDForNode(node); + const id = agent.getIDForHostInstance(node); if (id !== null) { bridge.send('selectElement', id); }