Create ReactNativeComponent Abstract Class

[yungsters]

Summary:
This creates ReactNativeComponent, the modern replacement for NativeMethodsMixin.

This class is going to allow us to start migrating React Native off React.createClass(...) into a typesafe, class-based brave new world.

This also introduces IReactNativeComponent, an interface that enforces consistency between ReactNativeComponent and ReactNativeFiberHostComponent.

[gaearon]

Should this be public API of RN? I was surprised FB code uses NativeMethodsMixin but we don't seem to expose it.

[yungsters]

Sounds good. Any objections to exporting it as ReactNative.Component?

[gaearon]

I think this would confuse people because it's easy to mistake imports. Maybe ReactNative.NativeComponent? In which case would one use it?

[bvaughn]

The ReactNative imports were causing the failure, I'd guess because of circular references. Try patching in:

diff --git a/src/renderers/native/ReactNativeComponent.js b/src/renderers/native/ReactNativeComponent.js
index 2671d79e3..0565bde50 100644
--- a/src/renderers/native/ReactNativeComponent.js
+++ b/src/renderers/native/ReactNativeComponent.js
@@ -13,8 +13,7 @@
 
 'use strict';
 
-const React = require('React');
-const ReactNative = require('ReactNative');
+const React = require('react');
 const ReactNativeAttributePayload = require('ReactNativeAttributePayload');
 const ReactNativeFeatureFlags = require('ReactNativeFeatureFlags');
 const TextInputState = require('TextInputState');
@@ -34,6 +33,10 @@ import type {
   ReactNativeBaseComponentViewConfig,
 } from 'ReactNativeViewConfigRegistry';
 
+const findNumericNodeHandle = ReactNativeFeatureFlags.useFiber
+  ? require('findNumericNodeHandleFiber')
+  : require('findNumericNodeHandleStack');
+
 /**
  * Superclass that provides methods to access the underlying native component.
  * This can be useful when you want to focus a view or measure its dimensions.
@@ -56,14 +59,14 @@ class ReactNativeComponent<DefaultProps, Props, State>
    * Removes focus. This is the opposite of `focus()`.
    */
   blur(): void {
-    TextInputState.blurTextInput(ReactNative.findNodeHandle(this));
+    TextInputState.blurTextInput(findNumericNodeHandle(this));
   }
 
   /**
    * Requests focus. The exact behavior depends on the platform and view.
    */
   focus(): void {
-    TextInputState.focusTextInput(ReactNative.findNodeHandle(this));
+    TextInputState.focusTextInput(findNumericNodeHandle(this));
   }
 
   /**
@@ -83,7 +86,7 @@ class ReactNativeComponent<DefaultProps, Props, State>
    */
   measure(callback: MeasureOnSuccessCallback): void {
     UIManager.measure(
-      ReactNative.findNodeHandle(this),
+      findNumericNodeHandle(this),
       mountSafeCallback(this, callback),
     );
   }
@@ -103,7 +106,7 @@ class ReactNativeComponent<DefaultProps, Props, State>
    */
   measureInWindow(callback: MeasureInWindowOnSuccessCallback): void {
     UIManager.measureInWindow(
-      ReactNative.findNodeHandle(this),
+      findNumericNodeHandle(this),
       mountSafeCallback(this, callback),
     );
   }
@@ -120,7 +123,7 @@ class ReactNativeComponent<DefaultProps, Props, State>
     onFail: () => void /* currently unused */,
   ): void {
     UIManager.measureLayout(
-      ReactNative.findNodeHandle(this),
+      findNumericNodeHandle(this),
       relativeToNativeNode,
       mountSafeCallback(this, onFail),
       mountSafeCallback(this, onSuccess),
@@ -134,14 +137,6 @@ class ReactNativeComponent<DefaultProps, Props, State>
    * Manipulation](docs/direct-manipulation.html)).
    */
   setNativeProps(nativeProps: Object): void {
-    // Ensure ReactNative factory function has configured findNodeHandle.
-    // Requiring it won't execute the factory function until first referenced.
-    // It's possible for tests that use ReactTestRenderer to reach this point,
-    // Without having executed ReactNative.
-    // Defer the factory function until now to avoid a cycle with UIManager.
-    // TODO (bvaughn) Remove this once ReactNativeStack is dropped.
-    require('ReactNative');
-
     injectedSetNativeProps(this, nativeProps);
   }
 }

[bvaughn]

I'd prefer to use NativeMethodsMixinType and add a "TODO" comment to replace it with an interface once NativeMethodsMixin is gone. This works (I tested it a few days ago) and avoids having to maintain mirror types.

[yungsters]

I'll change ReactNativeComponent to use NativeMethodsMixinType like you did in ReactNativeFiberHostComponent, but disclaimer... the same trick does not actually work for ReactNativeComponent:

(ReactNativeComponent.prototype: NativeMethodsMixinType);

Unlike for ReactNativeFiberHostComponent, Flow does not complain if any of the methods are missing. I suspect this is because ReactBaseClasses (and therefore React.Component) is not Flow-typed. Getting rid of extends React.Component fixes this, but we obviously cannot do that.

[bvaughn]

Oh, rats! That's unfortunate. 😦

In that case, then I guess the duplicate type is a reasonable workaround. If we use it, we should just remove the (ReactNativeComponent.prototype: NativeMethodsMixinType) cast too I guess.

[bvaughn]

This is redundant with the NativeMethodsMixinType type already being used.

[yungsters]

Try patching in: [...]

@bvaughn Thanks for that suggestion. However, I still run into the following build error:

'React' is imported by src/renderers/native/ReactNativeComponent.js, but could not be resolved – treating it as an external dependency

Although ReactNative might have also been an eventual cause of build breakage, I think React is for some reason also not permitted or a circular dependency.

[bvaughn]

Yeah, my diff also replaced "React" with "react":

-const React = require('React');
-const ReactNative = require('ReactNative');
+const React = require('react');

[yungsters]

Fixed the build and replaced the shim with ReactNative.NativeComponent.

[bvaughn]

If we're exposing ReactNativeComponent via ReactNative.NativeComponent then we can ditch the shim and the accessor on __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.ReactNativeComponent

[yungsters]

Oh, what... I thought I removed that already.

[bvaughn]

This can be removed too 😁

[bvaughn]

And this 😁

[yungsters]

Hmm... I think that I screwed something up with Git.

[yungsters]

Thank you for the thorough review. 👍

[bvaughn]

No problem! 👍