Skip to content

[DevTools] find best renderer when inspecting #24665

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

Merged
merged 9 commits into from
Jun 8, 2022
Merged

[DevTools] find best renderer when inspecting #24665

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
051a179
[DevTools] find best renderer when inspecting
mondaychen Jun 2, 2022
29841c9
fix lint
mondaychen Jun 2, 2022
276fc88
fix test
mondaychen Jun 6, 2022
b7d225a
fix lint
mondaychen Jun 6, 2022
7a6b59f
move logic to agent
mondaychen Jun 7, 2022
975601b
fix lint
mondaychen Jun 7, 2022
48bba78
style improvements per review comments
mondaychen Jun 7, 2022
2e8c6c5
fix lint & flow
mondaychen Jun 7, 2022
27b6756
re-add try catch for safety
mondaychen Jun 8, 2022
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
21 changes: 10 additions & 11 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 @@ -39,7 +39,10 @@ import type {
RendererInterface,
} from './types';
import type {ComponentFilter} from '../types';
import {isSynchronousXHRSupported} from './utils';
import {
isSynchronousXHRSupported,
getBestMatchingRendererInterface,
} from './utils';
import type {BrowserTheme} from 'react-devtools-shared/src/devtools/views/DevTools';

const debug = (methodName, ...args) => {
Expand Down Expand Up @@ -310,20 +313,16 @@ export default class Agent extends EventEmitter<{|
}

getIDForNode(node: Object): number | null {
const renderers = [];
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;
}
} catch (error) {
// Some old React versions might throw if they can't find a match.
// If so we should ignore it...
}
renderers.push(renderer);
}
const rendererInterface = getBestMatchingRendererInterface(renderers, node);
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably should be in the backend/agent file instead of the utils because you're using two for loops right now and if you put the code from getBestMatchingRendererInterface into the agent you would only need one

ex.

let renderer = this._rendererInterfaces[0];
for (const rendererID in this._rendererInterfaces) {
  const currentRenderer = ...;
  const fiber = currentRenderer.getFiberForNative(node, false);
  if (fiber !== null && fiber.stateNode === node) {
   renderer = currentRenderer;
    break;
  }
  ...
}
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first for-loop is only there for proper typing. Maybe I should refactor the types of this._rendererInterfaces?
It's indeed easier to do everything agent, but how about the use of the util function in Overlay?

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 updated the file with better typing d0346ff so that I can use values() instead of for-loop 9a7e0be

Does it make sense?

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;
}
Expand Down
6 changes: 6 additions & 0 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 @@ -1084,6 +1084,11 @@ export function attach(

function unpatchConsoleForStrictMode() {}

function getFiberForNative() {
// Not implemented
return null;
}

return {
clearErrorsAndWarnings,
clearErrorsForFiberID,
Expand All @@ -1094,6 +1099,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
View file
Open in desktop
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
1 change: 1 addition & 0 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 @@ -350,6 +350,7 @@ export type RendererInterface = {
findNativeNodesForFiberID: FindNativeNodesForFiberID,
flushInitialOperations: () => void,
getBestMatchForTrackedPath: () => PathMatch | null,
getFiberForNative: (hostInstance: NativeType) => ?Fiber,
getFiberIDForNative: GetFiberIDForNative,
getDisplayNameForFiberID: GetDisplayNameForFiberID,
getInstanceAndStyle(id: number): InstanceAndStyle,
Expand Down
22 changes: 22 additions & 0 deletions packages/react-devtools-shared/src/backend/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import {copy} from 'clipboard-js';
import {dehydrate} from '../hydration';
import isArray from 'shared/isArray';
import type {RendererInterface} from './types';

import type {DehydratedData} from 'react-devtools-shared/src/devtools/views/Components/types';

Expand Down Expand Up @@ -273,3 +274,24 @@ export function isSynchronousXHRSupported(): boolean {
window.document.featurePolicy.allowsFeature('sync-xhr')
);
}

export function getBestMatchingRendererInterface(
rendererInterfaces: Iterable<RendererInterface>,
node: Object,
): RendererInterface | null {
let bestMatch = null;
// eslint-disable-next-line no-for-of-loops/no-for-of-loops
for (const renderer of rendererInterfaces) {
const fiber = renderer.getFiberForNative(node);
if (fiber != null) {
// check if fiber.stateNode is matching the original hostInstance
if (fiber.stateNode === node) {
return renderer;
} else {
bestMatch = renderer;
}
}
}
// if an exact match is not found, return the best match as fallback
return bestMatch;
}
9 changes: 6 additions & 3 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 @@ -8,6 +8,7 @@
*/

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

const assign = Object.assign;

Expand Down Expand Up @@ -233,13 +234,15 @@ export default class Overlay {
const hook: DevToolsHook =
node.ownerDocument.defaultView.__REACT_DEVTOOLS_GLOBAL_HOOK__;
if (hook != null && hook.rendererInterfaces != null) {
const rendererInterface = getBestMatchingRendererInterface(
hook.rendererInterfaces.values(),
node,
);
let ownerName = null;
// eslint-disable-next-line no-for-of-loops/no-for-of-loops
for (const rendererInterface of hook.rendererInterfaces.values()) {
if (rendererInterface !== null) {
const id = rendererInterface.getFiberIDForNative(node, true);
if (id !== null) {
ownerName = rendererInterface.getDisplayNameForFiberID(id, true);
break;
}
}

Expand Down