-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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
Changes from 8 commits
051a179
29841c9
276fc88
b7d225a
7a6b59f
975601b
48bba78
2e8c6c5
27b6756
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -309,22 +309,31 @@ 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) { | ||
// check if fiber.stateNode is matching the original hostInstance | ||
if (fiber.stateNode === node) { | ||
return renderer; | ||
} else if (bestMatch === null) { | ||
bestMatch = 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 first valid renderer as fallback | ||
return bestMatch; | ||
} | ||
|
||
getIDForNode(node: Object): number | null { | ||
const rendererInterface = this.getBestMatchingRendererInterface(node); | ||
if (rendererInterface != null) { | ||
return rendererInterface.getFiberIDForNative(node, true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we wrap this around a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
|
There was a problem hiding this comment.
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 wherefiber
doesn't equalnull
. What do you think about keeping the original behavior if the exact fiber isn't found (ie. returning the first matching renderer)?