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

Adds a ReactNativeInspector API to ReactNativeRenderer #9691

Merged
merged 23 commits into from May 23, 2017
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
ddfd05f
fixed conflicts with master
trueadm May 15, 2017
12e8367
splits fiber and stack implementations
trueadm May 15, 2017
9cb93e5
prettier run
trueadm May 15, 2017
87d5fa3
Merge remote-tracking branch 'upstream/master' into reactnativeinspec…
trueadm May 15, 2017
179926e
updated fiber implementation based on feedback
trueadm May 16, 2017
6edea82
updated fiber implementation for selecting inspector hierarchy
trueadm May 16, 2017
416c207
run of prettier
trueadm May 22, 2017
532912a
fixed flow issues
trueadm May 22, 2017
159efdb
updated stack implementation
trueadm May 22, 2017
3a3d8c3
fixes an implementation difference where before it wasnt properly und…
trueadm May 22, 2017
ba9c3fa
addresses comments in PR feedback
trueadm May 23, 2017
28bef25
updated stack inspector to use emptyObject
trueadm May 23, 2017
0f88fa5
Update ReactNativeFiberInspector.js
trueadm May 23, 2017
450da60
fixes last flow error
trueadm May 23, 2017
6215bf9
prettier
trueadm May 23, 2017
d378e9c
applied changes to how viewConfig works for fibers and extracted meas…
trueadm May 23, 2017
76b721a
fixed bad paste
trueadm May 23, 2017
4ce3fc8
fixes flow errors
trueadm May 23, 2017
fc0c137
prettyify
trueadm May 23, 2017
a6f8e6b
revmoed getComponentName changes
trueadm May 23, 2017
3e84ec9
updated logic for getHostProps and addressed other PR feedback
trueadm May 23, 2017
e0e443e
improved throwing to invariant and update stack implemenation based o…
trueadm May 23, 2017
b05ef94
prettier
trueadm May 23, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/renderers/native/ReactNativeFiber.js
Expand Up @@ -23,6 +23,7 @@ const ReactNativeInjection = require('ReactNativeInjection');
const ReactNativeTagHandles = require('ReactNativeTagHandles');
const ReactNativeViewConfigRegistry = require('ReactNativeViewConfigRegistry');
const ReactPortal = require('ReactPortal');
const ReactNativeFiberInspector = require('ReactNativeFiberInspector');
const ReactVersion = require('ReactVersion');
const UIManager = require('UIManager');

Expand Down Expand Up @@ -465,6 +466,7 @@ if (typeof injectInternals === 'function') {
injectInternals({
findFiberByHostInstance: ReactNativeComponentTree.getClosestInstanceFromNode,
findHostInstanceByFiber: NativeRenderer.findHostInstance,
getInspectorDataForViewTag: ReactNativeFiberInspector.getInspectorDataForViewTag,
// This is an enum because we may add more (e.g. profiler build)
bundleType: __DEV__ ? 1 : 0,
version: ReactVersion,
Expand Down
101 changes: 101 additions & 0 deletions src/renderers/native/ReactNativeFiberInspector.js
@@ -0,0 +1,101 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* 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
Copy link
Collaborator

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.

Copy link
Contributor Author

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?

Copy link
Collaborator

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)

* @flow
*/
'use strict';

const ReactNativeComponentTree = require('ReactNativeComponentTree');
const ReactFiberTreeReflection = require('ReactFiberTreeReflection');
const getComponentName = require('getComponentName');

import type {Fiber} from 'ReactFiber';

if (__DEV__) {
var traverseOwnerTreeUp = function(hierarchy, instance: any) {
if (instance) {
hierarchy.unshift(instance);
traverseOwnerTreeUp(hierarchy, instance._debugOwner);
}
};

var getOwnerHierarchy = function(instance: any) {
var hierarchy = [];
traverseOwnerTreeUp(hierarchy, instance);
return hierarchy;
};

var lastNotNativeInstance = function(hierarchy) {
for (let i = hierarchy.length - 1; i > 1; i--) {
const instance = hierarchy[i];
if (!instance.viewConfig) {
Copy link
Collaborator

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.

return instance;
}
}
return hierarchy[0];
};

var getHostNode = function(fiber: Fiber, findNodeHandle) {
let hostNode;
// look for children first for the hostNode
// as composite fibers do not have a hostNode
while (fiber) {
hostNode = findNodeHandle(fiber.stateNode);
Copy link
Collaborator

@gaearon gaearon May 22, 2017

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?

Copy link
Collaborator

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.

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'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.

if (hostNode) {
return hostNode;
}
fiber = fiber.child;
}
return null;
};

var createHierarchy = function(fiberHierarchy) {
return fiberHierarchy.map(fiber => ({
name: getComponentName(fiber),
getInspectorData: findNodeHandle => ({
hostNode: getHostNode(fiber, findNodeHandle),
props: fiber.stateNode
? getFiberCurrentPropsFromNode(fiber.stateNode)
: {},
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 use emptyObject here?

Copy link
Collaborator

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.

source: fiber._debugSource,
}),
}));
};

const {
getClosestInstanceFromNode,
getFiberCurrentPropsFromNode,
} = ReactNativeComponentTree;

const {findCurrentFiberUsingSlowPath} = ReactFiberTreeReflection;

var getInspectorDataForViewTag = function(viewTag: number): Object {
Copy link
Collaborator

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).

const fiber = findCurrentFiberUsingSlowPath(
getClosestInstanceFromNode(viewTag),
);
const fiberHierarchy = getOwnerHierarchy(fiber);
const instance = lastNotNativeInstance(fiberHierarchy);
const hierarchy = createHierarchy(fiberHierarchy);
const props = getFiberCurrentPropsFromNode(instance.stateNode) || {};
const source = instance._debugSource;
const selection = fiberHierarchy.indexOf(instance);

return {
hierarchy,
instance,
props,
selection,
source,
};
};
}

module.exports = {
getInspectorDataForViewTag,
};
2 changes: 2 additions & 0 deletions src/renderers/native/ReactNativeStack.js
Expand Up @@ -16,6 +16,7 @@ var ReactNativeInjection = require('ReactNativeInjection');
var ReactNativeMount = require('ReactNativeMount');
var ReactNativeStackInjection = require('ReactNativeStackInjection');
var ReactUpdates = require('ReactUpdates');
var ReactNativeStackInspector = require('ReactNativeStackInspector');

var findNodeHandle = require('findNodeHandle');

Expand Down Expand Up @@ -81,6 +82,7 @@ if (
},
Mount: ReactNativeMount,
Reconciler: require('ReactReconciler'),
getInspectorDataForViewTag: ReactNativeStackInspector.getInspectorDataForViewTag,
});
}

Expand Down
75 changes: 75 additions & 0 deletions src/renderers/native/ReactNativeStackInspector.js
@@ -0,0 +1,75 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* 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 ReactNativeStackInspector
* @flow
*/
'use strict';

const ReactNativeComponentTree = require('ReactNativeComponentTree');
const getComponentName = require('getComponentName');

if (__DEV__) {
var traverseOwnerTreeUp = function(hierarchy, instance) {
if (instance) {
hierarchy.unshift(instance);
traverseOwnerTreeUp(hierarchy, instance._currentElement._owner);
}
};

var getOwnerHierarchy = function(instance) {
var hierarchy = [];
traverseOwnerTreeUp(hierarchy, instance);
return hierarchy;
};

var lastNotNativeInstance = function(hierarchy) {
for (let i = hierarchy.length - 1; i > 1; i--) {
const instance = hierarchy[i];
if (!instance.viewConfig) {
return instance;
}
}
return hierarchy[0];
};

var createHierarchy = function(componentHierarchy) {
return componentHierarchy.map(component => ({
name: getComponentName(component),
getInspectorData: () => ({
hostNode: component.getHostNode(),
props: (component._instance || {}).props || {},
source: component._currentElement && component._currentElement._source,
}),
}));
};

var getInspectorDataForViewTag = function(viewTag: any): Object {
Copy link
Collaborator

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.

const component = ReactNativeComponentTree.getClosestInstanceFromNode(
viewTag,
);
const componentHierarchy = getOwnerHierarchy(component);
const instance = lastNotNativeInstance(componentHierarchy);
const hierarchy = createHierarchy(componentHierarchy);
const props = (instance._instance || {}).props || {};
Copy link
Contributor

Choose a reason for hiding this comment

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

empty object :D

const source = instance._currentElement && instance._currentElement._source;
const selection = componentHierarchy.indexOf(instance);

return {
hierarchy,
instance,
props,
selection,
source,
};
};
}

module.exports = {
getInspectorDataForViewTag,
};