Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DevTools] Implement "best renderer" by taking the inner most matched node #30494

Merged
merged 1 commit into from
Jul 30, 2024
Merged

[DevTools] Implement "best renderer" by taking the inner most matched node #30494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/react-devtools-shared/src/__tests__/legacy/storeLegacy-v15-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -905,7 +905,7 @@ describe('Store (legacy)', () => {
[root]
▸ <Wrapper>
`);
const deepestedNodeID = global.agent.getIDForNode(ref);
const deepestedNodeID = global.agent.getIDForHostInstance(ref);
act(() => store.toggleIsCollapsed(deepestedNodeID, false));
expect(store).toMatchInlineSnapshot(`
[root]
Expand Down
2 changes: 1 addition & 1 deletion packages/react-devtools-shared/src/__tests__/store-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1217,7 +1217,7 @@ describe('Store', () => {
▸ <Wrapper>
`);

const deepestedNodeID = agent.getIDForNode(ref.current);
const deepestedNodeID = agent.getIDForHostInstance(ref.current);

act(() => store.toggleIsCollapsed(deepestedNodeID, false));
expect(store).toMatchInlineSnapshot(`
Expand Down
84 changes: 66 additions & 18 deletions packages/react-devtools-shared/src/backend/agent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>) => {
if (__DEBUG__) {
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by that? Which object is going to be allocated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like if you try to extract this into a helper you end up with the problem that JS can't return two values. It'd need to return the ID and the renderer. So you'd have to wrap that in an object and that would end up being an unnecessary 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...
Expand Down Expand Up @@ -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);
}
Expand Down
12 changes: 9 additions & 3 deletions packages/react-devtools-shared/src/backend/fiber/renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -4659,7 +4665,7 @@ export function attach(
flushInitialOperations,
getBestMatchForTrackedPath,
getDisplayNameForElementID,
getFiberForNative,
getNearestMountedHostInstance,
getElementIDForHostInstance,
getInstanceAndStyle,
getOwnersList,
Expand Down
17 changes: 13 additions & 4 deletions packages/react-devtools-shared/src/backend/legacy/renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Expand All @@ -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) => {
Expand Down Expand Up @@ -1111,7 +1120,7 @@ export function attach(
flushInitialOperations,
getBestMatchForTrackedPath,
getDisplayNameForElementID,
getFiberForNative,
getNearestMountedHostInstance,
getElementIDForHostInstance,
getInstanceAndStyle,
findHostInstancesForElementID: (id: number) => {
Expand Down
9 changes: 4 additions & 5 deletions packages/react-devtools-shared/src/backend/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
16 changes: 3 additions & 13 deletions packages/react-devtools-shared/src/backend/views/Highlighter/Overlay.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 + ')';
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/react-devtools-shared/src/backend/views/Highlighter/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
Loading