Merge ReactMethod and ReactSyncHook

javache · facebook-github-bot · commit 59226f022cf9 · 2017-01-30T06:43:48.000-08:00
Reviewed By: astreet

Differential Revision: D4409726

fbshipit-source-id: 7a0091da754b114680772aa9c0a898b1aa721ba5
diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/BaseJavaModule.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/BaseJavaModule.java
@@ -176,12 +176,18 @@ public class JavaMethod implements NativeMethod {
     private final int mJSArgumentsNeeded;
     private final String mTraceName;
 
-    public JavaMethod(Method method) {
+    public JavaMethod(Method method, boolean isSync) {
       mMethod = method;
       mMethod.setAccessible(true);
+
+      if (isSync) {
+        mType = METHOD_TYPE_SYNC;
+      }
+
+      // TODO: create these lazily
       Class[] parameterTypes = method.getParameterTypes();
       mArgumentExtractors = buildArgumentExtractors(parameterTypes);
-      mSignature = buildSignature(parameterTypes);
+      mSignature = buildSignature(mMethod, parameterTypes, isSync);
       // Since native methods are invoked from a message queue executed on a single thread, it is
       // save to allocate only one arguments object per method that can be reused across calls
       mArguments = new Object[parameterTypes.length];
@@ -197,9 +203,16 @@ public String getSignature() {
       return mSignature;
     }
 
-    private String buildSignature(Class[] paramTypes) {
-      StringBuilder builder = new StringBuilder(paramTypes.length);
-      builder.append("v.");
+    private String buildSignature(Method method, Class[] paramTypes, boolean isSync) {
+      StringBuilder builder = new StringBuilder(paramTypes.length + 2);
+
+      if (isSync) {
+        builder.append(returnTypeToChar(method.getReturnType()));
+        builder.append('.');
+      } else {
+        builder.append("v.");
+      }
+
       for (int i = 0; i < paramTypes.length; i++) {
         Class paramClass = paramTypes[i];
         if (paramClass == ExecutorToken.class) {
@@ -212,7 +225,9 @@ private String buildSignature(Class[] paramTypes) {
         } else if (paramClass == Promise.class) {
           Assertions.assertCondition(
             i == paramTypes.length - 1, "Promise must be used as last parameter only");
-          mType = METHOD_TYPE_PROMISE;
+          if (!isSync) {
+            mType = METHOD_TYPE_PROMISE;
+          }
         }
         builder.append(paramTypeToChar(paramClass));
       }
@@ -352,89 +367,36 @@ public void invoke(CatalystInstance catalystInstance, ExecutorToken executorToke
      * Determines how the method is exported in JavaScript:
      * METHOD_TYPE_ASYNC for regular methods
      * METHOD_TYPE_PROMISE for methods that return a promise object to the caller.
+     * METHOD_TYPE_SYNC for sync methods
      */
     @Override
     public String getType() {
       return mType;
     }
   }
 
-  public class SyncJavaHook implements SyncNativeHook {
-
-    private Method mMethod;
-    private final String mSignature;
-
-    public SyncJavaHook(Method method) {
-      mMethod = method;
-      mMethod.setAccessible(true);
-      mSignature = buildSignature(method);
-    }
-
-    public Method getMethod() {
-      return mMethod;
-    }
-
-    public String getSignature() {
-      return mSignature;
-    }
-
-    private String buildSignature(Method method) {
-      Class[] paramTypes = method.getParameterTypes();
-      StringBuilder builder = new StringBuilder(paramTypes.length + 2);
-
-      builder.append(returnTypeToChar(method.getReturnType()));
-      builder.append('.');
-
-      for (int i = 0; i < paramTypes.length; i++) {
-        Class paramClass = paramTypes[i];
-        if (paramClass == ExecutorToken.class) {
-          if (!BaseJavaModule.this.supportsWebWorkers()) {
-            throw new RuntimeException(
-              "Module " + BaseJavaModule.this + " doesn't support web workers, but " +
-                mMethod.getName() +
-                " takes an ExecutorToken.");
-          }
-        } else if (paramClass == Promise.class) {
-          Assertions.assertCondition(
-            i == paramTypes.length - 1, "Promise must be used as last parameter only");
-        }
-        builder.append(paramTypeToChar(paramClass));
-      }
-
-      return builder.toString();
-    }
-  }
-
   private @Nullable Map<String, NativeMethod> mMethods;
-  private @Nullable Map<String, SyncNativeHook> mHooks;
 
   private void findMethods() {
     if (mMethods == null) {
       Systrace.beginSection(TRACE_TAG_REACT_JAVA_BRIDGE, "findMethods");
       mMethods = new HashMap<>();
-      mHooks = new HashMap<>();
 
       Method[] targetMethods = getClass().getDeclaredMethods();
       for (Method targetMethod : targetMethods) {
-        if (targetMethod.getAnnotation(ReactMethod.class) != null) {
-          String methodName = targetMethod.getName();
-          if (mHooks.containsKey(methodName) || mMethods.containsKey(methodName)) {
-            // We do not support method overloading since js sees a function as an object regardless
-            // of number of params.
-            throw new IllegalArgumentException(
-              "Java Module " + getName() + " sync method name already registered: " + methodName);
-          }
-          mMethods.put(methodName, new JavaMethod(targetMethod));
-        }
-        if (targetMethod.getAnnotation(ReactSyncHook.class) != null) {
+        ReactMethod annotation = targetMethod.getAnnotation(ReactMethod.class);
+        if (annotation != null) {
           String methodName = targetMethod.getName();
-          if (mHooks.containsKey(methodName) || mMethods.containsKey(methodName)) {
+          if (mMethods.containsKey(methodName)) {
             // We do not support method overloading since js sees a function as an object regardless
             // of number of params.
             throw new IllegalArgumentException(
-              "Java Module " + getName() + " sync method name already registered: " + methodName);
+              "Java Module " + getName() + " method name already registered: " + methodName);
           }
-          mHooks.put(methodName, new SyncJavaHook(targetMethod));
+          mMethods.put(
+              methodName,
+              new JavaMethod(targetMethod,
+            annotation.isBlockingSynchronousMethod()));
         }
       }
       Systrace.endSection(TRACE_TAG_REACT_JAVA_BRIDGE);
@@ -454,11 +416,6 @@ public final Map<String, NativeMethod> getMethods() {
     return null;
   }
 
-  public final Map<String, SyncNativeHook> getSyncHooks() {
-    findMethods();
-    return assertNotNull(mHooks);
-  }
-
   @Override
   public void initialize() {
     // do nothing
diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/NativeModule.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/NativeModule.java
@@ -24,13 +24,6 @@ interface NativeMethod {
     String getType();
   }
 
-  /**
-   * A method that can be called from JS synchronously on the JS thread and return a result.
-   * @see ReactSyncHook
-   */
-  interface SyncNativeHook {
-  }
-
   /**
    * @return the name of this module. This will be the name used to {@code require()} this module
    * from javascript.
diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactMethod.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactMethod.java
@@ -14,13 +14,28 @@
 import static java.lang.annotation.RetentionPolicy.RUNTIME;
 
 /**
- * Annotation which is used to mark methods that are exposed to
- * Catalyst. This applies to derived classes of {@link
- * BaseJavaModule}, which will generate a list of exported methods by
- * searching for those which are annotated with this annotation and
- * adding a JS callback for each.
+ * Annotation which is used to mark methods that are exposed to React Native.
+ *
+ * This applies to derived classes of {@link BaseJavaModule}, which will generate a list
+ * of exported methods by searching for those which are annotated with this annotation
+ * and adding a JS callback for each.
  */
 @Retention(RUNTIME)
 public @interface ReactMethod {
-
+  /**
+   * Whether the method can be called from JS synchronously **on the JS thread**,
+   * possibly returning a result.
+   *
+   * WARNING: in the vast majority of cases, you should leave this to false which allows
+   * your native module methods to be called asynchronously: calling methods
+   * synchronously can have strong performance penalties and introduce threading-related
+   * bugs to your native modules.
+   *
+   * In order to support remote debugging, both the method args and return type must be
+   * serializable to JSON: this means that we only support the same args as
+   * {@link ReactMethod}, and the hook can only be void or return JSON values (e.g. bool,
+   * number, String, {@link WritableMap}, or {@link WritableArray}). Calling these
+   * methods when running under the websocket executor is currently not supported.
+   */
+  boolean isBlockingSynchronousMethod() default false;
 }
diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactSyncHook.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactSyncHook.java
diff --git a/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/JavaModuleWrapper.java b/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/JavaModuleWrapper.java
@@ -9,6 +9,7 @@
 
 package com.facebook.react.cxxbridge;
 
+import java.lang.reflect.Method;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
@@ -19,6 +20,7 @@
 import com.facebook.react.bridge.ExecutorToken;
 import com.facebook.react.bridge.NativeArray;
 import com.facebook.react.bridge.NativeModuleLogger;
+import com.facebook.react.bridge.NativeModule;
 import com.facebook.react.bridge.ReadableNativeArray;
 import com.facebook.react.bridge.WritableNativeArray;
 import com.facebook.react.bridge.WritableNativeMap;
@@ -38,6 +40,10 @@
 /* package */ class JavaModuleWrapper {
   @DoNotStrip
   public class MethodDescriptor {
+    @DoNotStrip
+    Method method;
+    @DoNotStrip
+    String signature;
     @DoNotStrip
     String name;
     @DoNotStrip
@@ -46,7 +52,7 @@ public class MethodDescriptor {
 
   private final CatalystInstance mCatalystInstance;
   private final ModuleHolder mModuleHolder;
-  private final ArrayList<BaseJavaModule.JavaMethod> mMethods;
+  private final ArrayList<NativeModule.NativeMethod> mMethods;
 
   public JavaModuleWrapper(CatalystInstance catalystinstance, ModuleHolder moduleHolder) {
     mCatalystInstance = catalystinstance;
@@ -67,19 +73,21 @@ public String getName() {
   @DoNotStrip
   public List<MethodDescriptor> getMethodDescriptors() {
     ArrayList<MethodDescriptor> descs = new ArrayList<>();
-
-    for (Map.Entry<String, BaseJavaModule.NativeMethod> entry :
-           getModule().getMethods().entrySet()) {
+    for (Map.Entry<String, NativeModule.NativeMethod> entry :
+          getModule().getMethods().entrySet()) {
       MethodDescriptor md = new MethodDescriptor();
       md.name = entry.getKey();
       md.type = entry.getValue().getType();
 
       BaseJavaModule.JavaMethod method = (BaseJavaModule.JavaMethod) entry.getValue();
+      if (md.type == BaseJavaModule.METHOD_TYPE_SYNC) {
+        md.signature = method.getSignature();
+        md.method = method.getMethod();
+      }
       mMethods.add(method);
 
       descs.add(md);
     }
-
     return descs;
   }
 
diff --git a/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryHolder.h b/ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryHolder.h
@@ -2,9 +2,9 @@
 
 #pragma once
 
+#include <cxxreact/ModuleRegistry.h>
 #include <fb/fbjni.h>
 
-#include <cxxreact/ModuleRegistry.h>
 #include "CxxModuleWrapper.h"
 
 namespace facebook {
diff --git a/ReactAndroid/src/test/java/com/facebook/react/bridge/BaseJavaModuleTest.java b/ReactAndroid/src/test/java/com/facebook/react/bridge/BaseJavaModuleTest.java
@@ -11,7 +11,6 @@
 
 import java.util.Map;
 
-import com.facebook.react.bridge.Arguments;
 import com.facebook.react.bridge.ReadableNativeArray;
 
 import org.junit.Before;
@@ -55,32 +54,43 @@ public void testCallMethodWithoutEnoughArgs() throws Exception {
     regularMethod.invoke(null, null, mArguments);
   }
 
-  @Test(expected = NativeArgumentsParseException.class)
-  public void testCallAsyncMethodWithoutEnoughArgs() throws Exception {
-    BaseJavaModule.NativeMethod asyncMethod = mMethods.get("asyncMethod");
+  @Test
+  public void testCallMethodWithEnoughArgs() {
+    BaseJavaModule.NativeMethod regularMethod = mMethods.get("regularMethod");
     Mockito.stub(mArguments.size()).toReturn(2);
-    asyncMethod.invoke(null, null, mArguments);
+    regularMethod.invoke(null, null, mArguments);
   }
 
-  @Test()
-  public void testCallAsyncMethodWithEnoughArgs() throws Exception {
+  @Test
+  public void testCallAsyncMethodWithEnoughArgs() {
+    // Promise block evaluates to 2 args needing to be passed from JS
     BaseJavaModule.NativeMethod asyncMethod = mMethods.get("asyncMethod");
     Mockito.stub(mArguments.size()).toReturn(3);
     asyncMethod.invoke(null, null, mArguments);
   }
 
+  @Test
+  public void testCallSyncMethod() {
+    BaseJavaModule.NativeMethod syncMethod = mMethods.get("syncMethod");
+    Mockito.stub(mArguments.size()).toReturn(2);
+    syncMethod.invoke(null, null, mArguments);
+  }
+
   private static class MethodsModule extends BaseJavaModule {
     @Override
     public String getName() {
       return "Methods";
     }
 
     @ReactMethod
-    public void regularMethod(String a, int b) {
-    }
+    public void regularMethod(String a, int b) {}
 
     @ReactMethod
-    public void asyncMethod(int a, Promise p) {
+    public void asyncMethod(int a, Promise p) {}
+
+    @ReactMethod(isBlockingSynchronousMethod = true)
+    public int syncMethod(int a, int b) {
+      return a + b;
     }
   }
 }
