Skip to content

Commit 59226f0

Browse files
javachefacebook-github-bot
javache
authored and
facebook-github-bot
committedJan 30, 2017
Merge ReactMethod and ReactSyncHook
Reviewed By: astreet Differential Revision: D4409726 fbshipit-source-id: 7a0091da754b114680772aa9c0a898b1aa721ba5
·
v0.80.00.60.2
1 parent 412acd2 commit 59226f0

File tree

7 files changed

+85
-133
lines changed

7 files changed

+85
-133
lines changed
 

‎ReactAndroid/src/main/java/com/facebook/react/bridge/BaseJavaModule.java

Lines changed: 30 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -176,12 +176,18 @@ public class JavaMethod implements NativeMethod {
176176
private final int mJSArgumentsNeeded;
177177
private final String mTraceName;
178178

179-
public JavaMethod(Method method) {
179+
public JavaMethod(Method method, boolean isSync) {
180180
mMethod = method;
181181
mMethod.setAccessible(true);
182+
183+
if (isSync) {
184+
mType = METHOD_TYPE_SYNC;
185+
}
186+
187+
// TODO: create these lazily
182188
Class[] parameterTypes = method.getParameterTypes();
183189
mArgumentExtractors = buildArgumentExtractors(parameterTypes);
184-
mSignature = buildSignature(parameterTypes);
190+
mSignature = buildSignature(mMethod, parameterTypes, isSync);
185191
// Since native methods are invoked from a message queue executed on a single thread, it is
186192
// save to allocate only one arguments object per method that can be reused across calls
187193
mArguments = new Object[parameterTypes.length];
@@ -197,9 +203,16 @@ public String getSignature() {
197203
return mSignature;
198204
}
199205

200-
private String buildSignature(Class[] paramTypes) {
201-
StringBuilder builder = new StringBuilder(paramTypes.length);
202-
builder.append("v.");
206+
private String buildSignature(Method method, Class[] paramTypes, boolean isSync) {
207+
StringBuilder builder = new StringBuilder(paramTypes.length + 2);
208+
209+
if (isSync) {
210+
builder.append(returnTypeToChar(method.getReturnType()));
211+
builder.append('.');
212+
} else {
213+
builder.append("v.");
214+
}
215+
203216
for (int i = 0; i < paramTypes.length; i++) {
204217
Class paramClass = paramTypes[i];
205218
if (paramClass == ExecutorToken.class) {
@@ -212,7 +225,9 @@ private String buildSignature(Class[] paramTypes) {
212225
} else if (paramClass == Promise.class) {
213226
Assertions.assertCondition(
214227
i == paramTypes.length - 1, "Promise must be used as last parameter only");
215-
mType = METHOD_TYPE_PROMISE;
228+
if (!isSync) {
229+
mType = METHOD_TYPE_PROMISE;
230+
}
216231
}
217232
builder.append(paramTypeToChar(paramClass));
218233
}
@@ -352,89 +367,36 @@ public void invoke(CatalystInstance catalystInstance, ExecutorToken executorToke
352367
* Determines how the method is exported in JavaScript:
353368
* METHOD_TYPE_ASYNC for regular methods
354369
* METHOD_TYPE_PROMISE for methods that return a promise object to the caller.
370+
* METHOD_TYPE_SYNC for sync methods
355371
*/
356372
@Override
357373
public String getType() {
358374
return mType;
359375
}
360376
}
361377

362-
public class SyncJavaHook implements SyncNativeHook {
363-
364-
private Method mMethod;
365-
private final String mSignature;
366-
367-
public SyncJavaHook(Method method) {
368-
mMethod = method;
369-
mMethod.setAccessible(true);
370-
mSignature = buildSignature(method);
371-
}
372-
373-
public Method getMethod() {
374-
return mMethod;
375-
}
376-
377-
public String getSignature() {
378-
return mSignature;
379-
}
380-
381-
private String buildSignature(Method method) {
382-
Class[] paramTypes = method.getParameterTypes();
383-
StringBuilder builder = new StringBuilder(paramTypes.length + 2);
384-
385-
builder.append(returnTypeToChar(method.getReturnType()));
386-
builder.append('.');
387-
388-
for (int i = 0; i < paramTypes.length; i++) {
389-
Class paramClass = paramTypes[i];
390-
if (paramClass == ExecutorToken.class) {
391-
if (!BaseJavaModule.this.supportsWebWorkers()) {
392-
throw new RuntimeException(
393-
"Module " + BaseJavaModule.this + " doesn't support web workers, but " +
394-
mMethod.getName() +
395-
" takes an ExecutorToken.");
396-
}
397-
} else if (paramClass == Promise.class) {
398-
Assertions.assertCondition(
399-
i == paramTypes.length - 1, "Promise must be used as last parameter only");
400-
}
401-
builder.append(paramTypeToChar(paramClass));
402-
}
403-
404-
return builder.toString();
405-
}
406-
}
407-
408378
private @Nullable Map<String, NativeMethod> mMethods;
409-
private @Nullable Map<String, SyncNativeHook> mHooks;
410379

411380
private void findMethods() {
412381
if (mMethods == null) {
413382
Systrace.beginSection(TRACE_TAG_REACT_JAVA_BRIDGE, "findMethods");
414383
mMethods = new HashMap<>();
415-
mHooks = new HashMap<>();
416384

417385
Method[] targetMethods = getClass().getDeclaredMethods();
418386
for (Method targetMethod : targetMethods) {
419-
if (targetMethod.getAnnotation(ReactMethod.class) != null) {
420-
String methodName = targetMethod.getName();
421-
if (mHooks.containsKey(methodName) || mMethods.containsKey(methodName)) {
422-
// We do not support method overloading since js sees a function as an object regardless
423-
// of number of params.
424-
throw new IllegalArgumentException(
425-
"Java Module " + getName() + " sync method name already registered: " + methodName);
426-
}
427-
mMethods.put(methodName, new JavaMethod(targetMethod));
428-
}
429-
if (targetMethod.getAnnotation(ReactSyncHook.class) != null) {
387+
ReactMethod annotation = targetMethod.getAnnotation(ReactMethod.class);
388+
if (annotation != null) {
430389
String methodName = targetMethod.getName();
431-
if (mHooks.containsKey(methodName) || mMethods.containsKey(methodName)) {
390+
if (mMethods.containsKey(methodName)) {
432391
// We do not support method overloading since js sees a function as an object regardless
433392
// of number of params.
434393
throw new IllegalArgumentException(
435-
"Java Module " + getName() + " sync method name already registered: " + methodName);
394+
"Java Module " + getName() + " method name already registered: " + methodName);
436395
}
437-
mHooks.put(methodName, new SyncJavaHook(targetMethod));
396+
mMethods.put(
397+
methodName,
398+
new JavaMethod(targetMethod,
399+
annotation.isBlockingSynchronousMethod()));
438400
}
439401
}
440402
Systrace.endSection(TRACE_TAG_REACT_JAVA_BRIDGE);
@@ -454,11 +416,6 @@ public final Map<String, NativeMethod> getMethods() {
454416
return null;
455417
}
456418

457-
public final Map<String, SyncNativeHook> getSyncHooks() {
458-
findMethods();
459-
return assertNotNull(mHooks);
460-
}
461-
462419
@Override
463420
public void initialize() {
464421
// do nothing

‎ReactAndroid/src/main/java/com/facebook/react/bridge/NativeModule.java

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -24,13 +24,6 @@ interface NativeMethod {
2424
String getType();
2525
}
2626

27-
/**
28-
* A method that can be called from JS synchronously on the JS thread and return a result.
29-
* @see ReactSyncHook
30-
*/
31-
interface SyncNativeHook {
32-
}
33-
3427
/**
3528
* @return the name of this module. This will be the name used to {@code require()} this module
3629
* from javascript.

‎ReactAndroid/src/main/java/com/facebook/react/bridge/ReactMethod.java

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,28 @@
1414
import static java.lang.annotation.RetentionPolicy.RUNTIME;
1515

1616
/**
17-
* Annotation which is used to mark methods that are exposed to
18-
* Catalyst. This applies to derived classes of {@link
19-
* BaseJavaModule}, which will generate a list of exported methods by
20-
* searching for those which are annotated with this annotation and
21-
* adding a JS callback for each.
17+
* Annotation which is used to mark methods that are exposed to React Native.
18+
*
19+
* This applies to derived classes of {@link BaseJavaModule}, which will generate a list
20+
* of exported methods by searching for those which are annotated with this annotation
21+
* and adding a JS callback for each.
2222
*/
2323
@Retention(RUNTIME)
2424
public @interface ReactMethod {
25-
25+
/**
26+
* Whether the method can be called from JS synchronously **on the JS thread**,
27+
* possibly returning a result.
28+
*
29+
* WARNING: in the vast majority of cases, you should leave this to false which allows
30+
* your native module methods to be called asynchronously: calling methods
31+
* synchronously can have strong performance penalties and introduce threading-related
32+
* bugs to your native modules.
33+
*
34+
* In order to support remote debugging, both the method args and return type must be
35+
* serializable to JSON: this means that we only support the same args as
36+
* {@link ReactMethod}, and the hook can only be void or return JSON values (e.g. bool,
37+
* number, String, {@link WritableMap}, or {@link WritableArray}). Calling these
38+
* methods when running under the websocket executor is currently not supported.
39+
*/
40+
boolean isBlockingSynchronousMethod() default false;
2641
}

‎ReactAndroid/src/main/java/com/facebook/react/bridge/ReactSyncHook.java

Lines changed: 0 additions & 31 deletions
This file was deleted.

‎ReactAndroid/src/main/java/com/facebook/react/cxxbridge/JavaModuleWrapper.java

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
package com.facebook.react.cxxbridge;
1111

12+
import java.lang.reflect.Method;
1213
import java.util.ArrayList;
1314
import java.util.List;
1415
import java.util.Map;
@@ -19,6 +20,7 @@
1920
import com.facebook.react.bridge.ExecutorToken;
2021
import com.facebook.react.bridge.NativeArray;
2122
import com.facebook.react.bridge.NativeModuleLogger;
23+
import com.facebook.react.bridge.NativeModule;
2224
import com.facebook.react.bridge.ReadableNativeArray;
2325
import com.facebook.react.bridge.WritableNativeArray;
2426
import com.facebook.react.bridge.WritableNativeMap;
@@ -38,6 +40,10 @@
3840
/* package */ class JavaModuleWrapper {
3941
@DoNotStrip
4042
public class MethodDescriptor {
43+
@DoNotStrip
44+
Method method;
45+
@DoNotStrip
46+
String signature;
4147
@DoNotStrip
4248
String name;
4349
@DoNotStrip
@@ -46,7 +52,7 @@ public class MethodDescriptor {
4652

4753
private final CatalystInstance mCatalystInstance;
4854
private final ModuleHolder mModuleHolder;
49-
private final ArrayList<BaseJavaModule.JavaMethod> mMethods;
55+
private final ArrayList<NativeModule.NativeMethod> mMethods;
5056

5157
public JavaModuleWrapper(CatalystInstance catalystinstance, ModuleHolder moduleHolder) {
5258
mCatalystInstance = catalystinstance;
@@ -67,19 +73,21 @@ public String getName() {
6773
@DoNotStrip
6874
public List<MethodDescriptor> getMethodDescriptors() {
6975
ArrayList<MethodDescriptor> descs = new ArrayList<>();
70-
71-
for (Map.Entry<String, BaseJavaModule.NativeMethod> entry :
72-
getModule().getMethods().entrySet()) {
76+
for (Map.Entry<String, NativeModule.NativeMethod> entry :
77+
getModule().getMethods().entrySet()) {
7378
MethodDescriptor md = new MethodDescriptor();
7479
md.name = entry.getKey();
7580
md.type = entry.getValue().getType();
7681

7782
BaseJavaModule.JavaMethod method = (BaseJavaModule.JavaMethod) entry.getValue();
83+
if (md.type == BaseJavaModule.METHOD_TYPE_SYNC) {
84+
md.signature = method.getSignature();
85+
md.method = method.getMethod();
86+
}
7887
mMethods.add(method);
7988

8089
descs.add(md);
8190
}
82-
8391
return descs;
8492
}
8593

‎ReactAndroid/src/main/jni/xreact/jni/ModuleRegistryHolder.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,9 @@
22

33
#pragma once
44

5+
#include <cxxreact/ModuleRegistry.h>
56
#include <fb/fbjni.h>
67

7-
#include <cxxreact/ModuleRegistry.h>
88
#include "CxxModuleWrapper.h"
99

1010
namespace facebook {

‎ReactAndroid/src/test/java/com/facebook/react/bridge/BaseJavaModuleTest.java

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111

1212
import java.util.Map;
1313

14-
import com.facebook.react.bridge.Arguments;
1514
import com.facebook.react.bridge.ReadableNativeArray;
1615

1716
import org.junit.Before;
@@ -55,32 +54,43 @@ public void testCallMethodWithoutEnoughArgs() throws Exception {
5554
regularMethod.invoke(null, null, mArguments);
5655
}
5756

58-
@Test(expected = NativeArgumentsParseException.class)
59-
public void testCallAsyncMethodWithoutEnoughArgs() throws Exception {
60-
BaseJavaModule.NativeMethod asyncMethod = mMethods.get("asyncMethod");
57+
@Test
58+
public void testCallMethodWithEnoughArgs() {
59+
BaseJavaModule.NativeMethod regularMethod = mMethods.get("regularMethod");
6160
Mockito.stub(mArguments.size()).toReturn(2);
62-
asyncMethod.invoke(null, null, mArguments);
61+
regularMethod.invoke(null, null, mArguments);
6362
}
6463

65-
@Test()
66-
public void testCallAsyncMethodWithEnoughArgs() throws Exception {
64+
@Test
65+
public void testCallAsyncMethodWithEnoughArgs() {
66+
// Promise block evaluates to 2 args needing to be passed from JS
6767
BaseJavaModule.NativeMethod asyncMethod = mMethods.get("asyncMethod");
6868
Mockito.stub(mArguments.size()).toReturn(3);
6969
asyncMethod.invoke(null, null, mArguments);
7070
}
7171

72+
@Test
73+
public void testCallSyncMethod() {
74+
BaseJavaModule.NativeMethod syncMethod = mMethods.get("syncMethod");
75+
Mockito.stub(mArguments.size()).toReturn(2);
76+
syncMethod.invoke(null, null, mArguments);
77+
}
78+
7279
private static class MethodsModule extends BaseJavaModule {
7380
@Override
7481
public String getName() {
7582
return "Methods";
7683
}
7784

7885
@ReactMethod
79-
public void regularMethod(String a, int b) {
80-
}
86+
public void regularMethod(String a, int b) {}
8187

8288
@ReactMethod
83-
public void asyncMethod(int a, Promise p) {
89+
public void asyncMethod(int a, Promise p) {}
90+
91+
@ReactMethod(isBlockingSynchronousMethod = true)
92+
public int syncMethod(int a, int b) {
93+
return a + b;
8494
}
8595
}
8696
}

0 commit comments

Comments
 (0)
Please sign in to comment.