Add HostComponent type to ReactNative #16898

elicwhite · 2019-09-25T22:24:42Z

Note: This change was already landed to React Native in facebook/react-native@69c38e5 so this commit just updates the types in React core to make the next sync not have conflicts.

In React Native there are three types of "Native" components.

createReactClass with NativeMethodsMixin

class MyComponent extends ReactNative.NativeComponent

requireNativeComponent('RCTView')

The implementation for how to handle all three of these exists in the React Native Renderer. Refs attached to components created via these methods provide a set of functions such as

.measure
.measureInWindow
.measureLayout
.setNativeProps

These methods have been used for our core components in the repo to provide a consistent API. Many of the APIs in React Native require a reactTag to a host component. This is acquired by calling findNodeHandle with any component. findNodeHandle works with the first two approaches.

For a lot of our new Fabric APIs, we will require passing a ref to a HostComponent directly instead of relying on findNodeHandle to tunnel through the component tree as that behavior isn't safe with React concurrent mode.

The goal of this change is to enable us to differentiate between components created with requireNativeComponent and the other types. This will be needed to be able to safely type the new APIs.

For existing components that should support being a host component but need to use some JS behavior in a wrapper, they should use forwardRef. The majority of React Native's core components were migrated to use forwardRef last year. Components that can't use forwardRef will need to have a method like getNativeRef() to get access to the underlying host component ref.

Note, we will need follow up changes as well as changes to the React Renderer in the React repo to fully utilize this new type.

sizebot · 2019-09-25T22:29:01Z

No significant bundle size changes to report.

Generated by 🚫 dangerJS against db60405

elicwhite · 2019-09-25T22:26:39Z

packages/react-native-renderer/src/NativeMethodsMixin.js

@@ -161,7 +161,7 @@ export default function(
    measureLayout: function(
      relativeToNativeNode: number | Object,
      onSuccess: MeasureLayoutOnSuccessCallback,
-      onFail: () => void /* currently unused */,
+      onFail?: () => void /* currently unused */,


This was always optional in the types used in React Native, but because the two types are duplicated in ReactNativeTypes, they must have fallen out of sync.

elicwhite · 2019-09-25T22:26:47Z

packages/react-native-renderer/src/ReactFabricHostConfig.js

@@ -151,9 +151,9 @@ class ReactFabricHostComponent {
  }

  measureLayout(
-    relativeToNativeNode: number | Object,


I realize it's not part of this change, but should the warningWithoutStack inside of this function (and the one in packages/react-native-renderer/src/ReactNativeFiberHostComponent.js) be __DEV__ gated?

warningWithoutStack internally has a dev check and is a no-op in dev.

elicwhite · 2019-09-25T22:29:02Z

packages/react-native-renderer/src/ReactNativeTypes.js

@@ -8,7 +8,7 @@
 * @flow
 */

-import React from 'react';
+import * as React from 'react';


This is needed because we are now importing types from React. ElementRef, and AbstractComponent.

elicwhite · 2019-09-25T22:29:22Z

packages/react-native-renderer/src/ReactNativeTypes.js

  ): void,
  setNativeProps(nativeProps: Object): void,
 };

+export type NativeMethodsMixinType = NativeMethods;
+export type HostComponent<T> = React.AbstractComponent<


So simple. So clean.

Summary: I upstreamed the changes to this file from previous commits to React in this (unlanded) PR: facebook/react#16898 (comment) I had to make some additional changes to be able to make Flow pass there. Bringing those changes back to FBSource as well. Having this change made here will make the next sync easier as we won't have to deal with conflicts then. Changelog: [Internal] Reviewed By: cpojer Differential Revision: D17586781 fbshipit-source-id: 4be8376d0af4fb5d63410afaaf5bb0005d992981

bvaughn · 2019-09-26T21:25:36Z

packages/react-native-renderer/src/ReactFabricHostConfig.js

@@ -151,9 +151,9 @@ class ReactFabricHostComponent {
  }

  measureLayout(
-    relativeToNativeNode: number | Object,


I realize it's not part of this change, but should the warningWithoutStack inside of this function (and the one in packages/react-native-renderer/src/ReactNativeFiberHostComponent.js) be __DEV__ gated?

bvaughn · 2019-09-26T21:27:54Z

packages/react-native-renderer/src/ReactNativeFiberHostComponent.js

      relativeToNativeNode.canonical &&
      relativeToNativeNode.canonical._nativeTag
    ) {
+      /* $FlowFixMe canonical doesn't exist on the node.
+       I think this branch is dead and will remove it in a followup */


I was wondering if the change from Object -> ReactNativeFiberHostComponent made sense given relativeToNativeNode.canonical but I assume it's okay?

Yeah.

When I added this option I wrote tests that called this with createClass, ReactNativeNativeComponent and a host component. Nothing triggers this branch. That's why I think this is dead.

Worst case, nothing except some of our Fabric screens are using this API yet and we'll get quick signal if I'm wrong once we delete it. But it's unrelated to this type change.

bvaughn · 2019-09-26T21:29:21Z

packages/react-native-renderer/src/ReactNativeTypes.js

@@ -8,7 +8,7 @@
 * @flow
 */

-import React from 'react';
+import React, {type ElementRef, type AbstractComponent} from 'react';


I didn't know you could mix ES imports and type imports this way 😮

bvaughn · 2019-09-26T21:31:52Z

packages/react-native-renderer/src/ReactNativeTypes.js

+export type NativeMethodsMixinType = NativeMethods;
+export type HostComponent<T> = AbstractComponent<
+  T,
+  $ReadOnly<$Exact<NativeMethods>>,


I don't think I understand the purpose of $ReadOnly or $Exact here.

From @jbrown215:

Definitely consider making the instance type of HostComponent read-only-- all of those function names seem like things you don't want people to overwrite. If the object really has no other properties, you can also make it exact. If it might, don't, otherwise spreading that object would be unsound (not that I can see a reason why someone would want to spread it?)

* Add HostComponent type to ReactNative * Use type alias imports instead of wildcard * Fix forgotten Object in measureLayout type

Add HostComponent type to ReactNative

afea710

facebook-github-bot added the CLA Signed label Sep 25, 2019

elicwhite commented Sep 25, 2019

View reviewed changes

elicwhite added 2 commits September 25, 2019 15:33

Use type alias imports instead of wildcard

e99175b

Fix forgotten Object in measureLayout type

db60405

elicwhite requested review from bvaughn and sebmarkbage September 26, 2019 20:34

bvaughn approved these changes Sep 26, 2019

View reviewed changes

elicwhite merged commit db8afe4 into facebook:master Sep 26, 2019

elicwhite deleted the host-component branch September 26, 2019 21:42

rickhanlonii pushed a commit to rickhanlonii/react that referenced this pull request Oct 9, 2019

Add HostComponent type to ReactNative (facebook#16898)

0663a5e

* Add HostComponent type to ReactNative * Use type alias imports instead of wildcard * Fix forgotten Object in measureLayout type

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HostComponent type to ReactNative #16898

Add HostComponent type to ReactNative #16898

elicwhite commented Sep 25, 2019 •

edited

Loading

sizebot commented Sep 25, 2019 •

edited

Loading

elicwhite Sep 25, 2019

elicwhite Sep 25, 2019

bvaughn Sep 26, 2019

elicwhite Sep 26, 2019

elicwhite Sep 25, 2019

elicwhite Sep 25, 2019

bvaughn Sep 26, 2019

bvaughn Sep 26, 2019

elicwhite Sep 26, 2019

bvaughn Sep 26, 2019

bvaughn Sep 26, 2019

elicwhite Sep 26, 2019

Add HostComponent type to ReactNative #16898

Add HostComponent type to ReactNative #16898

Conversation

elicwhite commented Sep 25, 2019 • edited Loading

sizebot commented Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elicwhite commented Sep 25, 2019 •

edited

Loading

sizebot commented Sep 25, 2019 •

edited

Loading