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
Adds a ReactNativeInspector API to ReactNativeRenderer #9691
Conversation
const fiber = ReactNativeComponentTree.findFiberByHostInstance(viewTag); | ||
const hierarchy = getOwnerHierarchy(fiber); | ||
const instance = lastNotNativeInstance(hierarchy); | ||
const props = instance.memoizedProps || {}; |
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.
This is not guaranteed to yield the current props. This could've been the previous or next props if the fiber we're getting is not the current one.
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.
getFiberCurrentPropsFromNode
is also not guaranteed to have the current props, because it should only be used for event listeners (atm) and therefore only updated if event listeners change.
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.
So what implementation is right here then? This was ported from an old PR, so it would be good to know what the ideal would be here.
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.
I'm not sure. :) The way we solved it in the devtools is that we track everything that could've been updated but it seems like overkill to do here.
We could use ReactFiberTreeReflection.findCurrentFiberUsingSlowPath
. That one will guarantee to yield the current Fiber and therefore the correct props for this particular Fiber but won't yield the correct props for every Fiber on the way up to the root owner.
We could call it repeatedly for every Fiber on the way to the root. Maybe that's not too bad for this use case?
|
||
var getInspectorDataForViewTag = function(viewTag: any): Object { | ||
const fiber = ReactNativeComponentTree.findFiberByHostInstance(viewTag); | ||
const hierarchy = getOwnerHierarchy(fiber); |
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.
This yields a stack of Fibers, right? What do you do with them on the other side? Can we expose something less leaky, like an object such as the one returned by this function.
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.
This is needed by inspector to show the current position in the tree relative to the root. It expects an array of fibers here.
@sebmarkbage can you take another look on this PR please? |
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.
Some tiny changes that we talked about in person. Deferring to @sebmarkbage for final say though.
hostNode: getHostNode(fiber, findNodeHandle), | ||
props: fiber.stateNode | ||
? getFiberCurrentPropsFromNode(fiber.stateNode) | ||
: {}, |
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.
Should we use emptyObject
here?
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.
This is not guaranteed to have the current props. It's misnamed. It's only guaranteed to have the current event handlers. ReactFiberTreeReflection.findCurrentHostFiber(fiber).memoizedProps
is better for inspection.
const componentHierarchy = getOwnerHierarchy(component); | ||
const instance = lastNotNativeInstance(componentHierarchy); | ||
const hierarchy = createHierarchy(componentHierarchy); | ||
const props = (instance._instance || {}).props || {}; |
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.
empty object :D
var lastNotNativeInstance = function(hierarchy) { | ||
for (let i = hierarchy.length - 1; i > 1; i--) { | ||
const instance = hierarchy[i]; | ||
if (!instance.viewConfig) { |
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.
I don't think this check would work with Fiber. Would it? hierarchy
contains fibers which wouldn't have the viewConfig
property.
// look for children first for the hostNode | ||
// as composite fibers do not have a hostNode | ||
while (fiber) { | ||
hostNode = findNodeHandle(fiber.stateNode); |
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.
Can we first check that fiber type is either ClassComponent
or HostComponent
? I don't think it's universally safe to take .stateNode
and expect it to be either a class or a node. For example coroutines also use .stateNode
for a different purpose, and findNodeHandle()
would blow up on unexpected inputs.
I would imagine something like:
if (fiber.stateNode !== null && (fiber.type === HostComponent || fiber.type === ClassComponent)) {
hostNode = findNodeHandle(fiber.stateNode);
}
I’m actually not sure if we even need to check for ClassComponent
there because we’d drill down anyway. So maybe it’s enough to check fiber.type === HostComponent && fiber.stateNode !== null
.
WDYT?
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.
We shouldn't even expose stateNode of HostComponent. That data structure will need to change and likely turn into something native. In fact, it may just be a number.
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.
I'm confused with the check on fiber.type
, when changing it to either HostComponent or ClassComponent, it fails as it never matches these, it seems they are functions not numbers.
Do you have an equivalent PR coming for React Native so that we can see how you intend to use this API? I don't think you should need to expose any Fibers or the stateNode from a HostComponent. All of that can have wrapper objects that you expose from here to the UI part and let the UI part just a dumb view that presents that in |
I don't think Fibers are exposed anymore: https://github.com/facebook/react/pull/9691/files#diff-a34363327f035a330f6a579ba383b2a4R59 They're mapped to opaque objects. I guess putting |
* LICENSE file in the root directory of this source tree. An additional grant | ||
* of patent rights can be found in the PATENTS file in the same directory. | ||
* | ||
* @providesModule ReactNativeFiberInspector |
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.
I think the value added by this UI is that you can inspect a quick breadcrumb - on the device. Otherwise you can just use the full DevTools.
There's nothing inherently ReactNative specific about this type of UI. Mobile web could use the same. We should build the same thing for web if it is useful to have a UI that just quickly shows the breadcrumb.
As a start, can we move this to "shared" and call it something more generic like ReactFiberBreadCrumbInspector.
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.
Is this something that is going to add a lot of value to do now rather than later? I get the reasoning for doing this, but I feel having a working Fiber inspector merged soon along with RN flat bundles is going to be of more value. What do you think?
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.
I agree I’d prefer we do another pass at this later, but unblock RN Fiber rollout now.
(After fixing the other issues where we seem very close)
Fixes a flow error
})); | ||
}; | ||
|
||
const {getClosestInstanceFromNode} = ReactNativeComponentTree; |
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.
Let's move imports to the top.
|
||
const {findCurrentFiberUsingSlowPath} = ReactFiberTreeReflection; | ||
|
||
var getInspectorDataForViewTag = function(viewTag: number): Object { |
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.
Let's make sure it's still defined in PROD mode (but is throwing).
})); | ||
}; | ||
|
||
var getInspectorDataForViewTag = function(viewTag: any): Object { |
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.
Let's make sure this is defined in PROD too.
|
||
var getHostProps = function(fiber) { | ||
return (ReactFiberTreeReflection.findCurrentHostFiber(fiber) || emptyObject) | ||
.memoizedProps; |
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.
Nit: let's make the intent more explicit by doing a check in a ternary or an if statement instead of reading emptyObject.memoizedProps
.
getInspectorData: () => ({ | ||
measure: callback => | ||
UIManager.measure(component.getHostNode(), callback), | ||
props: (component._instance || emptyObject).props || emptyObject, |
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.
Nit: let's make the intent more explicit by doing a check in a ternary or an if statement instead of reading emptyObject.props
.
const componentHierarchy = getOwnerHierarchy(component); | ||
const instance = lastNotNativeInstance(componentHierarchy); | ||
const hierarchy = createHierarchy(componentHierarchy); | ||
const props = (instance._instance || emptyObject).props || emptyObject; |
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.
Nit: let's make the intent more explicit by doing a check in a ternary or an if statement instead of reading emptyObject.props
.
Summary: This PR aims to update the Inspector tool in React Native to use the new inspection APIs that have been added to the ReactNative renderer: facebook/react#9691 This PR also cleans up the code in `Inspector.js` so there's no usage of React's internals. Closes #14160 Reviewed By: bvaughn Differential Revision: D5129280 Pulled By: trueadm fbshipit-source-id: b1b077c04f46b0f52cdea0e19b4154441558f77a
Currently, React Native has internal implementation details of how Fiber and Stack work in order to make the built-in React Native Inspector tool. This PR aims at adding an API to ReactNativeRenderer so React Native can access the internals via an externally made public API. This API is DEV only, so should not affect file size in production.
An upstream React Native PR will follow too, adding support for React Native to use this new API.