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.

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] find best renderer when inspecting #24665

Merged
merged 9 commits into from
Jun 8, 2022
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
35 changes: 26 additions & 9 deletions packages/react-devtools-shared/src/backend/agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -309,22 +309,39 @@ export default class Agent extends EventEmitter<{|
return renderer.getInstanceAndStyle(id);
}

getIDForNode(node: Object): number | null {
getBestMatchingRendererInterface(node: Object): RendererInterface | null {
let bestMatch = null;
for (const rendererID in this._rendererInterfaces) {
const renderer = ((this._rendererInterfaces[
(rendererID: any)
]: any): RendererInterface);

try {
const id = renderer.getFiberIDForNative(node, true);
if (id !== null) {
return id;
const fiber = renderer.getFiberForNative(node);
if (fiber != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

fiber !== null

// check if fiber.stateNode is matching the original hostInstance
if (fiber.stateNode === node) {
return renderer;
} else {
bestMatch = renderer;
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now you're setting bestMatch every renderer where fiber doesn't equal null. What do you think about keeping the original behavior if the exact fiber isn't found (ie. returning the first matching renderer)?

}
} catch (error) {
// Some old React versions might throw if they can't find a match.
// If so we should ignore it...
}
}
// if an exact match is not found, return the best match as fallback
return bestMatch;
}

getIDForNode(node: Object): number | null {
const rendererInterface = this.getBestMatchingRendererInterface(node);
if (rendererInterface != null) {
return rendererInterface.getFiberIDForNative(node, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we wrap this around a try/catch like before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add it back for safety, but it looks pretty vague what exactly it's trying to catch. The comment says it's for old react, and commit itself is pretty old too. Do we know in which version this might cause an error, so we can get rid of it one day?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good question. I'm not sure. We're currently supporting React 16, which was released 5 years ago, so we're supporting pretty far back. I'm not sure exactly which version causes an error, but if you want to find out and add a comment that'd be great 😊

}
return null;
}

getDisplayNameForNode(node: Object): string | null {
const rendererInterface = this.getBestMatchingRendererInterface(node);
if (rendererInterface != null) {
return rendererInterface.getDisplayNameForFiberID(node, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this not require an ID?

}
return null;
}

Expand Down
9 changes: 9 additions & 0 deletions packages/react-devtools-shared/src/backend/legacy/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import {decorateMany, forceUpdate, restoreMany} from './utils';
import type {
DevToolsHook,
GetFiberIDForNative,
GetFiberForNative,
InspectedElementPayload,
InstanceAndStyle,
NativeType,
Expand Down Expand Up @@ -148,6 +149,10 @@ export function attach(

let getInternalIDForNative: GetFiberIDForNative = ((null: any): GetFiberIDForNative);
let findNativeNodeForInternalID: (id: number) => ?NativeType;
let getFiberForNative: GetFiberForNative = node => {
// Not implemented.
return null;
};

if (renderer.ComponentTree) {
getInternalIDForNative = (node, findNearestUnfilteredAncestor) => {
Expand All @@ -160,6 +165,9 @@ export function attach(
const internalInstance = idToInternalInstanceMap.get(id);
return renderer.ComponentTree.getNodeFromInstance(internalInstance);
};
getFiberForNative = node => {
return renderer.ComponentTree.getClosestInstanceFromNode(node);
};
} else if (renderer.Mount.getID && renderer.Mount.getNode) {
getInternalIDForNative = (node, findNearestUnfilteredAncestor) => {
// Not implemented.
Expand Down Expand Up @@ -1094,6 +1102,7 @@ export function attach(
flushInitialOperations,
getBestMatchForTrackedPath,
getDisplayNameForFiberID,
getFiberForNative,
getFiberIDForNative: getInternalIDForNative,
getInstanceAndStyle,
findNativeNodesForFiberID: (id: number) => {
Expand Down
5 changes: 5 additions & 0 deletions packages/react-devtools-shared/src/backend/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -2818,6 +2818,10 @@ export function attach(
return fiber != null ? getDisplayNameForFiber(((fiber: any): Fiber)) : null;
}

function getFiberForNative(hostInstance) {
return renderer.findFiberByHostInstance(hostInstance);
}

function getFiberIDForNative(
hostInstance,
findNearestUnfilteredAncestor = false,
Expand Down Expand Up @@ -4490,6 +4494,7 @@ export function attach(
flushInitialOperations,
getBestMatchForTrackedPath,
getDisplayNameForFiberID,
getFiberForNative,
getFiberIDForNative,
getInstanceAndStyle,
getOwnersList,
Expand Down
4 changes: 3 additions & 1 deletion packages/react-devtools-shared/src/backend/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ export type GetFiberIDForNative = (
component: NativeType,
findNearestUnfilteredAncestor?: boolean,
) => number | null;
export type GetFiberForNative = (component: NativeType) => Fiber | null;
export type FindNativeNodesForFiberID = (id: number) => ?Array<NativeType>;

export type ReactProviderType<T> = {
Expand All @@ -93,7 +94,7 @@ export type Lane = number;
export type Lanes = number;

export type ReactRenderer = {
findFiberByHostInstance: (hostInstance: NativeType) => ?Fiber,
findFiberByHostInstance: (hostInstance: NativeType) => Fiber | null,
version: string,
rendererPackageName: string,
bundleType: BundleType,
Expand Down Expand Up @@ -350,6 +351,7 @@ export type RendererInterface = {
findNativeNodesForFiberID: FindNativeNodesForFiberID,
flushInitialOperations: () => void,
getBestMatchForTrackedPath: () => PathMatch | null,
getFiberForNative: GetFiberForNative,
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand why you're doing this (following prior precedent) but I personally think this would be more readable if GetFiberForNative was inlined. What do you think?

getFiberIDForNative: GetFiberIDForNative,
getDisplayNameForFiberID: GetDisplayNameForFiberID,
getInstanceAndStyle(id: number): InstanceAndStyle,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
* @flow
*/

import type Agent from 'react-devtools-shared/src/backend/agent';

import Overlay from './Overlay';

const SHOW_DURATION = 2000;
Expand All @@ -26,6 +28,7 @@ export function hideOverlay() {
export function showOverlay(
elements: Array<HTMLElement> | null,
componentName: string | null,
agent: Agent,
hideAfterTimeout: boolean,
) {
// TODO (npm-packages) Detect RN and support it somehow
Expand All @@ -42,7 +45,7 @@ export function showOverlay(
}

if (overlay === null) {
overlay = new Overlay();
overlay = new Overlay(agent);
}

overlay.inspect(elements, componentName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@

import {getElementDimensions, getNestedBoundingClientRect} from '../utils';

const assign = Object.assign;

import type {DevToolsHook} from 'react-devtools-shared/src/backend/types';
import type {Rect} from '../utils';
import type Agent from 'react-devtools-shared/src/backend/agent';

type Box = {|top: number, left: number, width: number, height: number|};

const assign = Object.assign;

// Note that the Overlay components are not affected by the active Theme,
// because they highlight elements in the main Chrome window (outside of devtools).
// The colors below were chosen to roughly match those used by Chrome devtools.
Expand Down Expand Up @@ -153,8 +153,9 @@ export default class Overlay {
container: HTMLElement;
tip: OverlayTip;
rects: Array<OverlayRect>;
agent: Agent;

constructor() {
constructor(agent: Agent) {
// Find the root window, because overlays are positioned relative to it.
const currentWindow = window.__REACT_DEVTOOLS_TARGET_WINDOW__ || window;
this.window = currentWindow;
Expand All @@ -170,6 +171,8 @@ export default class Overlay {
this.tip = new OverlayTip(doc, this.container);
this.rects = [];

this.agent = agent;

doc.body.appendChild(this.container);
}

Expand Down Expand Up @@ -230,22 +233,10 @@ export default class Overlay {
name = elements[0].nodeName.toLowerCase();

const node = elements[0];
const hook: DevToolsHook =
node.ownerDocument.defaultView.__REACT_DEVTOOLS_GLOBAL_HOOK__;
if (hook != null && hook.rendererInterfaces != null) {
let ownerName = null;
// eslint-disable-next-line no-for-of-loops/no-for-of-loops
for (const rendererInterface of hook.rendererInterfaces.values()) {
const id = rendererInterface.getFiberIDForNative(node, true);
if (id !== null) {
ownerName = rendererInterface.getDisplayNameForFiberID(id, true);
break;
}
}

if (ownerName) {
name += ' (in ' + ownerName + ')';
}
const ownerName = this.agent.getDisplayNameForNode(node);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe instead of making getDisplayNameForNode you can use getBestMatchingRendererInterface, getFiberIDForNative, and gwetDisplayNameForFiberID?


if (ownerName) {
name += ' (in ' + ownerName + ')';
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ export default function setupHighlighter(
node.scrollIntoView({block: 'nearest', inline: 'nearest'});
}

showOverlay(nodes, displayName, hideAfterTimeout);
showOverlay(nodes, displayName, agent, hideAfterTimeout);

if (openNativeElementsPanel) {
window.__REACT_DEVTOOLS_GLOBAL_HOOK__.$0 = node;
Expand Down Expand Up @@ -171,7 +171,7 @@ export default function setupHighlighter(

// Don't pass the name explicitly.
// It will be inferred from DOM tag and Fiber owner.
showOverlay([target], null, false);
showOverlay([target], null, agent, false);

selectFiberForNode(target);
}
Expand Down