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

[React Native] Add getInspectorDataForViewAtPoint #18233

Merged
merged 11 commits into from
Mar 11, 2020

Conversation

rickhanlonii
Copy link
Member

@rickhanlonii rickhanlonii commented Mar 5, 2020

Summary

This diff adds getInspectorDataForViewAtPoint, accessible through the DevTools hook, which provides an interface to return inspector data about the view touched at a given point. This allows the React Native inspector to ask React for information about a view at an x,y touch point without any knowledge of how each renderer gets the information.

Implementation

Paper
For Paper we've just moved the logic from the InspectorOverlay into React Core. This will allow us to delete getInspectorDataForViewTag called here which is necessary since Fabric does not support React tags.

Fabric
For Fabric we add a new strategy which queries native for a node using findNodeAtPoint, passes the shadow node to measure, and then asks React for the fiber information.

Test Plan

  • Manually tested in Fabric and Paper for React Native.

@sizebot
Copy link

sizebot commented Mar 5, 2020

Details of bundled changes.

Comparing: 024a764...aa3719a

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactFabric-dev.js +0.7% +0.5% 615.72 KB 619.82 KB 133.15 KB 133.8 KB RN_OSS_DEV
ReactNativeRenderer-dev.js +0.7% +0.5% 633.61 KB 637.87 KB 137.47 KB 138.14 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 🔺+0.1% 🔺+0.2% 259.8 KB 260.13 KB 45.2 KB 45.27 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +0.1% +0.1% 271.59 KB 271.92 KB 47.44 KB 47.5 KB RN_OSS_PROFILING
ReactFabric-prod.js 🔺+0.1% 🔺+0.1% 251.76 KB 251.96 KB 43.71 KB 43.75 KB RN_OSS_PROD
ReactFabric-profiling.js +0.1% +0.1% 263.56 KB 263.76 KB 45.98 KB 46.01 KB RN_OSS_PROFILING

Size changes (stable)

Generated by 🚫 dangerJS against aa3719a

@sizebot
Copy link

sizebot commented Mar 5, 2020

Details of bundled changes.

Comparing: 024a764...aa3719a

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-prod.js 🔺+0.1% 🔺+0.2% 259.81 KB 260.14 KB 45.21 KB 45.28 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +0.1% +0.1% 271.61 KB 271.94 KB 47.45 KB 47.51 KB RN_OSS_PROFILING
ReactFabric-prod.js 🔺+0.1% 🔺+0.1% 251.77 KB 251.97 KB 43.72 KB 43.75 KB RN_OSS_PROD
ReactFabric-profiling.js +0.1% +0.1% 263.57 KB 263.77 KB 45.99 KB 46.02 KB RN_OSS_PROFILING
ReactFabric-dev.js +0.7% +0.5% 618.36 KB 622.47 KB 133.49 KB 134.14 KB RN_FB_DEV
ReactFabric-prod.js 🔺+0.1% 🔺+0.1% 251.93 KB 252.13 KB 43.75 KB 43.78 KB RN_FB_PROD
ReactNativeRenderer-dev.js +0.7% +0.5% 633.63 KB 637.88 KB 137.47 KB 138.15 KB RN_OSS_DEV
ReactFabric-profiling.js +0.1% +0.1% 263.72 KB 263.92 KB 46.02 KB 46.06 KB RN_FB_PROFILING
ReactNativeRenderer-dev.js +0.7% +0.5% 636.25 KB 640.51 KB 137.81 KB 138.48 KB RN_FB_DEV
ReactNativeRenderer-prod.js 🔺+0.1% 🔺+0.2% 259.96 KB 260.28 KB 45.23 KB 45.3 KB RN_FB_PROD
ReactNativeRenderer-profiling.js +0.1% +0.1% 271.75 KB 272.08 KB 47.48 KB 47.54 KB RN_FB_PROFILING
ReactFabric-dev.js +0.7% +0.5% 615.73 KB 619.84 KB 133.16 KB 133.81 KB RN_OSS_DEV

Size changes (experimental)

Generated by 🚫 dangerJS against aa3719a

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 5, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit aa3719a:

Sandbox Source
trusting-chaum-vgmil Configuration

@rickhanlonii rickhanlonii marked this pull request as ready for review March 8, 2020 00:14
Copy link
Member

@elicwhite elicwhite left a comment

Choose a reason for hiding this comment

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

Approving

scripts/error-codes/codes.json Outdated Show resolved Hide resolved
@rickhanlonii rickhanlonii merged commit bf35108 into facebook:master Mar 11, 2020
@rickhanlonii rickhanlonii deleted the rh-inspector-fabric branch March 11, 2020 16:12
@@ -10,6 +10,7 @@
import type {Fiber} from './ReactFiber';
import type {FiberRoot} from './ReactFiberRoot';
import type {RootTag} from 'shared/ReactRootTags';
import type {TouchedViewDataAtPoint} from 'react-native-renderer/src/ReactNativeTypes';
Copy link
Contributor

Choose a reason for hiding this comment

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

This import is causing CI to fail.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can't have dependencies on RN from the reconciler bundle. The folders are checked independently.

Also from a layering perspective it's probably not right.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused that some of the commits seemed to pass CI


export type TouchedViewDataAtPoint = $ReadOnly<{|
pointerY: number,
touchedViewTag?: number,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this used for? I thought we're getting rid of all these tags so seems odd to add new users of it and expose more implementation details about to change to future work. Shouldn't there be another mechanism?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is to support the current devtools behavior in Paper where tapping in the app will open the element in the tree. I was thinking we would add this now to prevent the regression, while we figure out the correct solution for Fabric and Paper moving forward.

https://github.com/facebook/react-native/blob/84b7b8d07645b528c784e0bacecf1293797e0ec7/Libraries/Inspector/Inspector.js#L209-L212

What do you think?

sebmarkbage added a commit that referenced this pull request Mar 11, 2020
@sebmarkbage
Copy link
Collaborator

I reverted this to unblock the sync but it's probably an easy fix. Feel free to reland after fixing the Flow errors etc.

@@ -109,6 +110,13 @@ type DevToolsConfig = {|
// This API is unfortunately RN-specific.
// TODO: Change it to accept Fiber instead and type it properly.
getInspectorDataForViewTag?: (tag: number) => Object,
// Used by RN in-app inspector.
getInspectorDataForViewAtPoint?: (
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 either of these two should be part of this type because they're not actually used by the core DevTools protocol. It's kind of an adhoc RN specific addition.

Instead we should make this a renderer specific object.

rendererConfig: RendererInspectionConfig

Then you can define the type for RendererInspectionConfig through the HostConfig mechanism. So that each renderer gets their own type. That can then be safely referenced in the react-native folder.

So Fabric and RN host configs gets this type:

export type RendererInspectionConfig = {
  getInspectorDataForViewTag?: (tag: number) => Object,
  getInspectorDataForViewAtPoint?: (...) => ...;
};

You then import that from here using:

import type {RendererInspectionConfig} from './ReactFiberHostConfig';

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants