From 4a567f22c5a20d885b3a60b2736120577722a8ec Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 27 Aug 2024 16:54:08 -0400 Subject: [PATCH 1/6] Track all public HostInstances in a Map This lets us get from a HostInstance to the nearest DevToolsInstance without relying on findNearestUnfilteredElementID and fiberToDevToolsInstanceMap. --- .../src/backend/fiber/renderer.js | 186 ++++++++++-------- 1 file changed, 109 insertions(+), 77 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index d9a85984c8bcf..734d1cc047513 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -738,35 +738,85 @@ const fiberToFiberInstanceMap: Map = new Map(); // operations that should be the same whether the current and work-in-progress Fiber is used. const idToDevToolsInstanceMap: Map = new Map(); -// Map of resource DOM nodes to all the Fibers that depend on it. -const hostResourceToFiberMap: Map> = new Map(); +// Map of canonical HostInstances to the nearest parent DevToolsInstance. +const publicInstanceToDevToolsInstanceMap: Map = + new Map(); +// Map of resource DOM nodes to all the nearest DevToolsInstances that depend on it. +const hostResourceToDevToolsInstanceMap: Map< + HostInstance, + Set, +> = new Map(); + +function getPublicInstance(instance: HostInstance): HostInstance { + // Typically the PublicInstance and HostInstance is the same thing but not in Fabric. + // So we need to detect this and use that as the public instance. + return typeof instance === 'object' && + instance !== null && + typeof instance.canonical === 'object' + ? (instance.canonical: any) + : instance; +} + +function aquireHostInstance( + nearestInstance: DevToolsInstance, + hostInstance: HostInstance, +): void { + const publicInstance = getPublicInstance(hostInstance); + publicInstanceToDevToolsInstanceMap.set(publicInstance, nearestInstance); +} + +function releaseHostInstance( + nearestInstance: DevToolsInstance, + hostInstance: HostInstance, +): void { + const publicInstance = getPublicInstance(hostInstance); + if ( + publicInstanceToDevToolsInstanceMap.get(publicInstance) === nearestInstance + ) { + publicInstanceToDevToolsInstanceMap.delete(publicInstance); + } +} function aquireHostResource( - fiber: Fiber, + nearestInstance: DevToolsInstance, resource: ?{instance?: HostInstance}, ): void { const hostInstance = resource && resource.instance; if (hostInstance) { - let resourceFibers = hostResourceToFiberMap.get(hostInstance); - if (resourceFibers === undefined) { - resourceFibers = new Set(); - hostResourceToFiberMap.set(hostInstance, resourceFibers); + const publicInstance = getPublicInstance(hostInstance); + let resourceInstances = + hostResourceToDevToolsInstanceMap.get(publicInstance); + if (resourceInstances === undefined) { + resourceInstances = new Set(); + hostResourceToDevToolsInstanceMap.set(publicInstance, resourceInstances); + // Store the first match in the main map for quick access when selecting DOM node. + publicInstanceToDevToolsInstanceMap.set(publicInstance, nearestInstance); } - resourceFibers.add(fiber); + resourceInstances.add(nearestInstance); } } function releaseHostResource( - fiber: Fiber, + nearestInstance: DevToolsInstance, resource: ?{instance?: HostInstance}, ): void { const hostInstance = resource && resource.instance; if (hostInstance) { - const resourceFibers = hostResourceToFiberMap.get(hostInstance); - if (resourceFibers !== undefined) { - resourceFibers.delete(fiber); - if (resourceFibers.size === 0) { - hostResourceToFiberMap.delete(hostInstance); + const publicInstance = getPublicInstance(hostInstance); + const resourceInstances = + hostResourceToDevToolsInstanceMap.get(publicInstance); + if (resourceInstances !== undefined) { + resourceInstances.delete(nearestInstance); + if (resourceInstances.size === 0) { + hostResourceToDevToolsInstanceMap.delete(publicInstance); + publicInstanceToDevToolsInstanceMap.delete(publicInstance); + } else if (publicInstanceToDevToolsInstanceMap.get(publicInstance) === nearestInstance) { + // This was the first one. Store the next first one in the main map for easy access. + // eslint-disable-next-line no-for-of-loops/no-for-of-loops + for (const firstInstance of resourceInstances) { + publicInstanceToDevToolsInstanceMap.set(firstInstance, nearestInstance); + break; + } } } } @@ -1513,6 +1563,18 @@ export function attach( fiberToFiberInstanceMap.delete(alternate); } } + + // TODO: This is not enough if this Fiber was filtered since we don't end up + // untracking it. We could use a WeakMap but that doesn't work for Paper tags. + if (fiber.tag === HostHoistable) { + releaseHostResource(fiberInstance, fiber.memoizedState); + } else if ( + fiber.tag === HostComponent || + fiber.tag === HostText || + fiber.tag === HostSingleton + ) { + releaseHostInstance(fiberInstance, fiber.stateNode); + } } function getChangeDescription( @@ -2670,7 +2732,21 @@ export function attach( } if (fiber.tag === HostHoistable) { - aquireHostResource(fiber, fiber.memoizedState); + const nearestInstance = reconcilingParent; + if (nearestInstance === null) { + throw new Error('Did not expect a host hoistable to be the root'); + } + aquireHostResource(nearestInstance, fiber.memoizedState); + } else if ( + fiber.tag === HostComponent || + fiber.tag === HostText || + fiber.tag === HostSingleton + ) { + const nearestInstance = reconcilingParent; + if (nearestInstance === null) { + throw new Error('Did not expect a host hoistable to be the root'); + } + aquireHostInstance(nearestInstance, fiber.stateNode); } if (fiber.tag === SuspenseComponent) { @@ -3291,8 +3367,12 @@ export function attach( } try { if (nextFiber.tag === HostHoistable) { - releaseHostResource(prevFiber, prevFiber.memoizedState); - aquireHostResource(nextFiber, nextFiber.memoizedState); + const nearestInstance = reconcilingParent; + if (nearestInstance === null) { + throw new Error('Did not expect a host hoistable to be the root'); + } + releaseHostResource(nearestInstance, prevFiber.memoizedState); + aquireHostResource(nearestInstance, nextFiber.memoizedState); } const isSuspense = nextFiber.tag === SuspenseComponent; @@ -3781,81 +3861,33 @@ export function attach( } function getNearestMountedHostInstance( - hostInstance: HostInstance, + publicInstance: HostInstance, ): null | HostInstance { - const mountedFiber = renderer.findFiberByHostInstance(hostInstance); + // TODO: Remove dependency on findFiberByHostInstance. + const mountedFiber = renderer.findFiberByHostInstance(publicInstance); if (mountedFiber != null) { - if (mountedFiber.stateNode !== hostInstance) { + if (mountedFiber.stateNode !== publicInstance) { // If it's not a perfect match the specific one might be a resource. // We don't need to look at any parents because host resources don't have // children so it won't be in any parent if it's not this one. - if (hostResourceToFiberMap.has(hostInstance)) { - return hostInstance; + if (publicInstanceToDevToolsInstanceMap.has(publicInstance)) { + return publicInstance; } } return mountedFiber.stateNode; } - if (hostResourceToFiberMap.has(hostInstance)) { - return hostInstance; - } - return null; - } - - function findNearestUnfilteredElementID(searchFiber: Fiber) { - let fiber: null | Fiber = searchFiber; - while (fiber !== null) { - const fiberInstance = getFiberInstanceUnsafe(fiber); - if (fiberInstance !== null) { - // TODO: Ideally we would not have any filtered FiberInstances which - // would make this logic much simpler. Unfortunately, we sometimes - // eagerly add to the map and some times don't eagerly clean it up. - // TODO: If the fiber is filtered, the FiberInstance wouldn't really - // exist which would mean that we also don't have a way to get to the - // VirtualInstances. - if (!shouldFilterFiber(fiberInstance.data)) { - return fiberInstance.id; - } - // We couldn't use this Fiber but we might have a VirtualInstance - // that is the nearest unfiltered instance. - const parentInstance = fiberInstance.parent; - if ( - parentInstance !== null && - parentInstance.kind === VIRTUAL_INSTANCE - ) { - // Virtual Instances only exist if they're unfiltered. - return parentInstance.id; - } - // If we find a parent Fiber, it might not be the nearest parent - // so we break out and continue walking the Fiber tree instead. - } - fiber = fiber.return; + if (publicInstanceToDevToolsInstanceMap.has(publicInstance)) { + return publicInstance; } return null; } function getElementIDForHostInstance( - hostInstance: HostInstance, - findNearestUnfilteredAncestor: boolean = false, + publicInstance: HostInstance, ): number | null { - const resourceFibers = hostResourceToFiberMap.get(hostInstance); - if (resourceFibers !== undefined) { - // This is a resource. Find the first unfiltered instance. - // eslint-disable-next-line no-for-of-loops/no-for-of-loops - for (const resourceFiber of resourceFibers) { - const elementID = findNearestUnfilteredElementID(resourceFiber); - if (elementID !== null) { - return elementID; - } - } - // If we don't find one, fallthrough to select the parent instead. - } - const fiber = renderer.findFiberByHostInstance(hostInstance); - if (fiber != null) { - if (!findNearestUnfilteredAncestor) { - // TODO: Remove this option. It's not used. - return getFiberIDThrows(fiber); - } - return findNearestUnfilteredElementID(fiber); + const instance = publicInstanceToDevToolsInstanceMap.get(publicInstance); + if (instance !== undefined) { + return instance.id; } return null; } From 3843330ef22d2626d7d653f68220b580bf293cd7 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 3 Sep 2024 16:58:18 -0400 Subject: [PATCH 2/6] Handle React Native tags --- packages/react-devtools-shared/src/backend/fiber/renderer.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index 734d1cc047513..f145fc18f48cd 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -754,7 +754,9 @@ function getPublicInstance(instance: HostInstance): HostInstance { instance !== null && typeof instance.canonical === 'object' ? (instance.canonical: any) - : instance; + : typeof instance._nativeTag === 'number' + ? instance._nativeTag + : instance; } function aquireHostInstance( From 6473b9c05d29453c7ba1b886a07bed255798dd6f Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 27 Aug 2024 17:54:36 -0400 Subject: [PATCH 3/6] Rename getNearestMountedHostInstance to getNearestMountedDOMNode This conflict resolution is really only relevant to DOM renderers since there's generally multiple of them per tree and this resolution is only active for those anyway. Making this clear means we can change the implementation details. --- .../src/backend/agent.js | 165 +++++++++++------- .../src/backend/fiber/renderer.js | 16 +- .../src/backend/legacy/renderer.js | 14 +- .../src/backend/types.js | 5 +- 4 files changed, 118 insertions(+), 82 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/agent.js b/packages/react-devtools-shared/src/backend/agent.js index 9de4115b21d97..92db4c062d5a2 100644 --- a/packages/react-devtools-shared/src/backend/agent.js +++ b/packages/react-devtools-shared/src/backend/agent.js @@ -342,84 +342,123 @@ export default class Agent extends EventEmitter<{ } 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 nearestNode: null = renderer.getNearestMountedHostInstance(target); - if (nearestNode !== null) { - if (nearestNode === target) { - // Exact match we can exit early. - bestMatch = nearestNode; - bestRenderer = renderer; - break; + if (isReactNativeEnvironment() || typeof target.nodeType !== 'number') { + // In React Native or non-DOM we simply pick any renderer that has a match. + for (const rendererID in this._rendererInterfaces) { + const renderer = ((this._rendererInterfaces[ + (rendererID: any) + ]: any): RendererInterface); + try { + const match = renderer.getElementIDForHostInstance(target); + if (match != null) { + return match; + } + } catch (error) { + // Some old React versions might throw if they can't find a match. + // If so we should ignore it... } - 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; + } + return null; + } else { + // In the DOM we use a smarter mechanism to find the deepest a DOM node + // that is registered if there isn't an exact match. + let bestMatch: null | Element = 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: null | Element = renderer.getNearestMountedDOMNode( + (target: any), + ); + if (nearestNode !== null) { + if (nearestNode === target) { + // Exact match we can exit early. + bestMatch = nearestNode; + bestRenderer = renderer; + break; + } + if (bestMatch === null || 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 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... + if (bestRenderer != null && bestMatch != null) { + try { + return bestRenderer.getElementIDForHostInstance(bestMatch); + } catch (error) { + // Some old React versions might throw if they can't find a match. + // If so we should ignore it... + } } + return null; } - return 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 (isReactNativeEnvironment() || typeof target.nodeType !== 'number') { + // In React Native or non-DOM we simply pick any renderer that has a match. + for (const rendererID in this._rendererInterfaces) { + const renderer = ((this._rendererInterfaces[ + (rendererID: any) + ]: any): RendererInterface); + try { + const id = renderer.getElementIDForHostInstance(target); + if (id) { + return renderer.getDisplayNameForElementID(id); + } + } catch (error) { + // Some old React versions might throw if they can't find a match. + // If so we should ignore it... } - 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; + } + return null; + } else { + // In the DOM we use a smarter mechanism to find the deepest a DOM node + // that is registered if there isn't an exact match. + let bestMatch: null | Element = 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: null | Element = renderer.getNearestMountedDOMNode( + (target: any), + ); + if (nearestNode !== null) { + if (nearestNode === target) { + // Exact match we can exit early. + bestMatch = nearestNode; + bestRenderer = renderer; + break; + } + if (bestMatch === null || 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 { - const id = bestRenderer.getElementIDForHostInstance(bestMatch, true); - if (id) { - return bestRenderer.getDisplayNameForElementID(id); + if (bestRenderer != null && bestMatch != null) { + try { + const id = bestRenderer.getElementIDForHostInstance(bestMatch); + 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... } - } catch (error) { - // Some old React versions might throw if they can't find a match. - // If so we should ignore it... } + return null; } - return null; } getBackendVersion: () => void = () => { diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index f145fc18f48cd..1c62d177cd7dc 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -812,11 +812,17 @@ function releaseHostResource( if (resourceInstances.size === 0) { hostResourceToDevToolsInstanceMap.delete(publicInstance); publicInstanceToDevToolsInstanceMap.delete(publicInstance); - } else if (publicInstanceToDevToolsInstanceMap.get(publicInstance) === nearestInstance) { + } else if ( + publicInstanceToDevToolsInstanceMap.get(publicInstance) === + nearestInstance + ) { // This was the first one. Store the next first one in the main map for easy access. // eslint-disable-next-line no-for-of-loops/no-for-of-loops for (const firstInstance of resourceInstances) { - publicInstanceToDevToolsInstanceMap.set(firstInstance, nearestInstance); + publicInstanceToDevToolsInstanceMap.set( + firstInstance, + nearestInstance, + ); break; } } @@ -3862,9 +3868,7 @@ export function attach( } } - function getNearestMountedHostInstance( - publicInstance: HostInstance, - ): null | HostInstance { + function getNearestMountedDOMNode(publicInstance: Element): null | Element { // TODO: Remove dependency on findFiberByHostInstance. const mountedFiber = renderer.findFiberByHostInstance(publicInstance); if (mountedFiber != null) { @@ -5822,7 +5826,7 @@ export function attach( flushInitialOperations, getBestMatchForTrackedPath, getDisplayNameForElementID, - getNearestMountedHostInstance, + getNearestMountedDOMNode, 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 ccff5ef07a1b6..fa29d007a681d 100644 --- a/packages/react-devtools-shared/src/backend/legacy/renderer.js +++ b/packages/react-devtools-shared/src/backend/legacy/renderer.js @@ -145,15 +145,13 @@ export function attach( let getElementIDForHostInstance: GetElementIDForHostInstance = ((null: any): GetElementIDForHostInstance); let findHostInstanceForInternalID: (id: number) => ?HostInstance; - let getNearestMountedHostInstance = ( - node: HostInstance, - ): null | HostInstance => { + let getNearestMountedDOMNode = (node: Element): null | Element => { // Not implemented. return null; }; if (renderer.ComponentTree) { - getElementIDForHostInstance = (node, findNearestUnfilteredAncestor) => { + getElementIDForHostInstance = node => { const internalInstance = renderer.ComponentTree.getClosestInstanceFromNode(node); return internalInstanceToIDMap.get(internalInstance) || null; @@ -162,9 +160,7 @@ export function attach( const internalInstance = idToInternalInstanceMap.get(id); return renderer.ComponentTree.getNodeFromInstance(internalInstance); }; - getNearestMountedHostInstance = ( - node: HostInstance, - ): null | HostInstance => { + getNearestMountedDOMNode = (node: Element): null | Element => { const internalInstance = renderer.ComponentTree.getClosestInstanceFromNode(node); if (internalInstance != null) { @@ -173,7 +169,7 @@ export function attach( return null; }; } else if (renderer.Mount.getID && renderer.Mount.getNode) { - getElementIDForHostInstance = (node, findNearestUnfilteredAncestor) => { + getElementIDForHostInstance = node => { // Not implemented. return null; }; @@ -1126,7 +1122,7 @@ export function attach( flushInitialOperations, getBestMatchForTrackedPath, getDisplayNameForElementID, - getNearestMountedHostInstance, + getNearestMountedDOMNode, 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 2f1482fb1cbd2..fea635c50c4a7 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -90,7 +90,6 @@ export type GetDisplayNameForElementID = (id: number) => string | null; export type GetElementIDForHostInstance = ( component: HostInstance, - findNearestUnfilteredAncestor?: boolean, ) => number | null; export type FindHostInstancesForElementID = ( id: number, @@ -358,9 +357,7 @@ export type RendererInterface = { findHostInstancesForElementID: FindHostInstancesForElementID, flushInitialOperations: () => void, getBestMatchForTrackedPath: () => PathMatch | null, - getNearestMountedHostInstance: ( - component: HostInstance, - ) => HostInstance | null, + getNearestMountedDOMNode: (component: Element) => Element | null, getElementIDForHostInstance: GetElementIDForHostInstance, getDisplayNameForElementID: GetDisplayNameForElementID, getInstanceAndStyle(id: number): InstanceAndStyle, From 0fb4a9477ba425f5a9a57398ff133ce767afcdf4 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 27 Aug 2024 20:52:28 -0400 Subject: [PATCH 4/6] Simplify getNearestMountedDOMNode to just walk to path and check the map We don't need the dependency on findFiberByHostInstance now that we maintain our own map of public instances. This relies on the assumption that this is a DOM renderer but we could always add support for more renderer types that need conflict resolution if they happen. --- .../src/backend/fiber/renderer.js | 21 +++++-------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index 1c62d177cd7dc..e121b4a982f6b 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -3869,23 +3869,12 @@ export function attach( } function getNearestMountedDOMNode(publicInstance: Element): null | Element { - // TODO: Remove dependency on findFiberByHostInstance. - const mountedFiber = renderer.findFiberByHostInstance(publicInstance); - if (mountedFiber != null) { - if (mountedFiber.stateNode !== publicInstance) { - // If it's not a perfect match the specific one might be a resource. - // We don't need to look at any parents because host resources don't have - // children so it won't be in any parent if it's not this one. - if (publicInstanceToDevToolsInstanceMap.has(publicInstance)) { - return publicInstance; - } - } - return mountedFiber.stateNode; + let domNode: null | Element = publicInstance; + while (domNode && !publicInstanceToDevToolsInstanceMap.has(domNode)) { + // $FlowFixMe: In practice this is either null or Element. + domNode = domNode.parentNode; } - if (publicInstanceToDevToolsInstanceMap.has(publicInstance)) { - return publicInstance; - } - return null; + return domNode; } function getElementIDForHostInstance( From 47a1fa88b15a6f5960458789d82fb77bf1696cd3 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 27 Aug 2024 22:03:25 -0400 Subject: [PATCH 5/6] Make injecting findFiberByHostInstance optional so future versions can drop it --- .../react-devtools-shared/src/backend/console.js | 12 +----------- packages/react-devtools-shared/src/backend/index.js | 7 ++++++- packages/react-devtools-shared/src/backend/types.js | 3 ++- 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/console.js b/packages/react-devtools-shared/src/backend/console.js index 93725c4428269..05d9055d0b021 100644 --- a/packages/react-devtools-shared/src/backend/console.js +++ b/packages/react-devtools-shared/src/backend/console.js @@ -135,17 +135,7 @@ export function registerRenderer( renderer: ReactRenderer, onErrorOrWarning?: OnErrorOrWarning, ): void { - const { - currentDispatcherRef, - getCurrentFiber, - findFiberByHostInstance, - version, - } = renderer; - - // Ignore React v15 and older because they don't expose a component stack anyway. - if (typeof findFiberByHostInstance !== 'function') { - return; - } + const {currentDispatcherRef, getCurrentFiber, version} = renderer; // currentDispatcherRef gets injected for v16.8+ to support hooks inspection. // getCurrentFiber gets injected for v16.9+. diff --git a/packages/react-devtools-shared/src/backend/index.js b/packages/react-devtools-shared/src/backend/index.js index 5c398ebf75869..f264134b4bb53 100644 --- a/packages/react-devtools-shared/src/backend/index.js +++ b/packages/react-devtools-shared/src/backend/index.js @@ -73,7 +73,12 @@ export function initBackend( // Inject any not-yet-injected renderers (if we didn't reload-and-profile) if (rendererInterface == null) { - if (typeof renderer.findFiberByHostInstance === 'function') { + if ( + // v16-19 + typeof renderer.findFiberByHostInstance === 'function' || + // v16.8+ + renderer.currentDispatcherRef != null + ) { // react-reconciler v16+ rendererInterface = attach(hook, id, renderer, global); } else if (renderer.ComponentTree) { diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index fea635c50c4a7..982426f2ffb19 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -105,10 +105,11 @@ export type Lane = number; export type Lanes = number; export type ReactRenderer = { - findFiberByHostInstance: (hostInstance: HostInstance) => Fiber | null, version: string, rendererPackageName: string, bundleType: BundleType, + // 16.0+ - To be removed in future versions. + findFiberByHostInstance?: (hostInstance: HostInstance) => Fiber | null, // 16.9+ overrideHookState?: ?( fiber: Object, From a4ae4d6eefbd311f5c6938003be28ebc4ad1c270 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 3 Sep 2024 16:56:43 -0400 Subject: [PATCH 6/6] Untrack Fiber by walking the Fiber tree If we have any filtered children, we need to untrack those as well. --- .../src/backend/fiber/renderer.js | 105 ++++++++++-------- 1 file changed, 56 insertions(+), 49 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index e121b4a982f6b..7cd7554c14c84 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -1525,63 +1525,30 @@ export function attach( // Removes a Fiber (and its alternate) from the Maps used to track their id. // This method should always be called when a Fiber is unmounting. - function untrackFiber(fiberInstance: FiberInstance) { + function untrackFiber(nearestInstance: DevToolsInstance, fiber: Fiber) { if (__DEBUG__) { - debug('untrackFiber()', fiberInstance.data, null); + debug('untrackFiber()', fiber, null); } + // TODO: Consider using a WeakMap instead. The only thing where that doesn't work + // is React Native Paper which tracks tags but that support is eventually going away + // and can use the old findFiberByHostInstance strategy. - idToDevToolsInstanceMap.delete(fiberInstance.id); - - const fiber = fiberInstance.data; - - // Restore any errors/warnings associated with this fiber to the pending - // map. I.e. treat it as before we tracked the instances. This lets us - // restore them if we remount the same Fibers later. Otherwise we rely - // on the GC of the Fibers to clean them up. - if (fiberInstance.errors !== null) { - pendingFiberToErrorsMap.set(fiber, fiberInstance.errors); - fiberInstance.errors = null; - } - if (fiberInstance.warnings !== null) { - pendingFiberToWarningsMap.set(fiber, fiberInstance.warnings); - fiberInstance.warnings = null; - } - - if (fiberInstance.flags & FORCE_ERROR) { - fiberInstance.flags &= ~FORCE_ERROR; - forceErrorCount--; - if (forceErrorCount === 0 && setErrorHandler != null) { - setErrorHandler(shouldErrorFiberAlwaysNull); - } - } - if (fiberInstance.flags & FORCE_SUSPENSE_FALLBACK) { - fiberInstance.flags &= ~FORCE_SUSPENSE_FALLBACK; - forceFallbackCount--; - if (forceFallbackCount === 0 && setSuspenseHandler != null) { - setSuspenseHandler(shouldSuspendFiberAlwaysFalse); - } - } - - if (fiberToFiberInstanceMap.get(fiber) === fiberInstance) { - fiberToFiberInstanceMap.delete(fiber); - } - const {alternate} = fiber; - if (alternate !== null) { - if (fiberToFiberInstanceMap.get(alternate) === fiberInstance) { - fiberToFiberInstanceMap.delete(alternate); - } - } - - // TODO: This is not enough if this Fiber was filtered since we don't end up - // untracking it. We could use a WeakMap but that doesn't work for Paper tags. if (fiber.tag === HostHoistable) { - releaseHostResource(fiberInstance, fiber.memoizedState); + releaseHostResource(nearestInstance, fiber.memoizedState); } else if ( fiber.tag === HostComponent || fiber.tag === HostText || fiber.tag === HostSingleton ) { - releaseHostInstance(fiberInstance, fiber.stateNode); + releaseHostInstance(nearestInstance, fiber.stateNode); + } + + // Recursively clean up any filtered Fibers below this one as well since + // we won't recordUnmount on those. + for (let child = fiber.child; child !== null; child = child.sibling) { + if (shouldFilterFiber(child)) { + untrackFiber(nearestInstance, child); + } } } @@ -2425,7 +2392,47 @@ export function attach( pendingRealUnmountedIDs.push(id); } - untrackFiber(fiberInstance); + idToDevToolsInstanceMap.delete(fiberInstance.id); + + // Restore any errors/warnings associated with this fiber to the pending + // map. I.e. treat it as before we tracked the instances. This lets us + // restore them if we remount the same Fibers later. Otherwise we rely + // on the GC of the Fibers to clean them up. + if (fiberInstance.errors !== null) { + pendingFiberToErrorsMap.set(fiber, fiberInstance.errors); + fiberInstance.errors = null; + } + if (fiberInstance.warnings !== null) { + pendingFiberToWarningsMap.set(fiber, fiberInstance.warnings); + fiberInstance.warnings = null; + } + + if (fiberInstance.flags & FORCE_ERROR) { + fiberInstance.flags &= ~FORCE_ERROR; + forceErrorCount--; + if (forceErrorCount === 0 && setErrorHandler != null) { + setErrorHandler(shouldErrorFiberAlwaysNull); + } + } + if (fiberInstance.flags & FORCE_SUSPENSE_FALLBACK) { + fiberInstance.flags &= ~FORCE_SUSPENSE_FALLBACK; + forceFallbackCount--; + if (forceFallbackCount === 0 && setSuspenseHandler != null) { + setSuspenseHandler(shouldSuspendFiberAlwaysFalse); + } + } + + if (fiberToFiberInstanceMap.get(fiber) === fiberInstance) { + fiberToFiberInstanceMap.delete(fiber); + } + const {alternate} = fiber; + if (alternate !== null) { + if (fiberToFiberInstanceMap.get(alternate) === fiberInstance) { + fiberToFiberInstanceMap.delete(alternate); + } + } + + untrackFiber(fiberInstance, fiber); } // Running state of the remaining children from the previous version of this parent that