From 041185edfdeac67e4476c6d4ea1a07e413d1f83f Mon Sep 17 00:00:00 2001 From: Alexander Blom Date: Wed, 4 May 2016 10:28:59 -0700 Subject: [PATCH] Let JS modules use strings instead of ids on Android Reviewed By: nicklockwood Differential Revision: D3229626 fb-gh-sync-id: f8b2e8c9fc0d25d67f623cdbbe9930a02a40d1de fbshipit-source-id: f8b2e8c9fc0d25d67f623cdbbe9930a02a40d1de --- Libraries/Utilities/MessageQueue.js | 11 +--- .../Utilities/__tests__/MessageQueue-test.js | 6 -- .../react/bridge/CatalystInstance.java | 4 +- .../react/bridge/CatalystInstanceImpl.java | 8 ++- .../bridge/JavaScriptModuleRegistration.java | 65 +++++++------------ .../bridge/JavaScriptModuleRegistry.java | 4 +- .../react/bridge/JavaScriptModulesConfig.java | 9 +-- .../facebook/react/bridge/ReactBridge.java | 2 +- ReactAndroid/src/main/jni/react/Bridge.h | 4 +- .../src/main/jni/react/jni/OnLoad.cpp | 6 +- .../bridge/JavaScriptModuleConfigTest.java | 8 +-- 11 files changed, 45 insertions(+), 82 deletions(-) diff --git a/Libraries/Utilities/MessageQueue.js b/Libraries/Utilities/MessageQueue.js index 54a641db1..2bbd718f9 100644 --- a/Libraries/Utilities/MessageQueue.js +++ b/Libraries/Utilities/MessageQueue.js @@ -53,8 +53,6 @@ class MessageQueue { this._callableModules = {}; this._queue = [[], [], [], 0]; - this._moduleTable = {}; - this._methodTable = {}; this._callbacks = []; this._callbackID = 0; this._callID = 0; @@ -69,9 +67,6 @@ class MessageQueue { let modulesConfig = this._genModulesConfig(remoteModules); this._genModules(modulesConfig); - localModules && this._genLookupTables( - this._genModulesConfig(localModules),this._moduleTable, this._methodTable - ); this._debugInfo = {}; this._remoteModuleTable = {}; @@ -165,13 +160,9 @@ class MessageQueue { } } - __callFunction(module, method, args) { + __callFunction(module: string, method: string, args: any) { this._lastFlush = new Date().getTime(); this._eventLoopStartTime = this._lastFlush; - if (isFinite(module)) { - method = this._methodTable[module][method]; - module = this._moduleTable[module]; - } Systrace.beginEvent(`${module}.${method}()`); if (__DEV__ && SPY_MODE) { console.log('N->JS : ' + module + '.' + method + '(' + JSON.stringify(args) + ')'); diff --git a/Libraries/Utilities/__tests__/MessageQueue-test.js b/Libraries/Utilities/__tests__/MessageQueue-test.js index bc7005d90..f1865d3fa 100644 --- a/Libraries/Utilities/__tests__/MessageQueue-test.js +++ b/Libraries/Utilities/__tests__/MessageQueue-test.js @@ -48,12 +48,6 @@ describe('MessageQueue', () => { assertQueue(flushedQueue, 0, 0, 1, [2]); }); - it('should call a local function with id', () => { - expect(TestModule.testHook1.calls.count()).toEqual(0); - queue.__callFunction(0, 0, [1]); - expect(TestModule.testHook1.calls.count()).toEqual(1); - }); - it('should call a local function with the function name', () => { expect(TestModule.testHook2.calls.count()).toEqual(0); queue.__callFunction('one', 'testHook2', [2]); diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstance.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstance.java index f85b9cf69..2987512ef 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstance.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstance.java @@ -30,8 +30,8 @@ public interface CatalystInstance extends MemoryPressureListener { @DoNotStrip void callFunction( ExecutorToken executorToken, - int moduleId, - int methodId, + String module, + String method, NativeArray arguments, String tracingName); /** diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java index 702b7759d..784f8a6cd 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java @@ -172,8 +172,8 @@ public class CatalystInstanceImpl implements CatalystInstance { @Override public void callFunction( ExecutorToken executorToken, - int moduleId, - int methodId, + String module, + String method, NativeArray arguments, String tracingName) { synchronized (mJavaToJSCallsTeardownLock) { @@ -184,7 +184,9 @@ public class CatalystInstanceImpl implements CatalystInstance { incrementPendingJSCalls(); - Assertions.assertNotNull(mBridge).callFunction(executorToken, moduleId, methodId, arguments, tracingName); + Assertions.assertNotNull(mBridge).callFunction(executorToken, + module, + method, arguments, tracingName); } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaScriptModuleRegistration.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaScriptModuleRegistration.java index e049833a4..42b518e47 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaScriptModuleRegistration.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaScriptModuleRegistration.java @@ -13,12 +13,13 @@ import javax.annotation.concurrent.Immutable; import java.lang.reflect.Method; import java.util.Arrays; -import java.util.Comparator; +import java.util.HashMap; +import java.util.LinkedHashSet; +import java.util.List; import java.util.Map; import java.util.Set; -import com.facebook.react.common.MapBuilder; -import com.facebook.infer.annotation.Assertions; +import com.facebook.react.common.build.ReactBuildConfig; /** * Registration info for a {@link JavaScriptModule}. Maps its methods to method ids. @@ -26,54 +27,32 @@ import com.facebook.infer.annotation.Assertions; @Immutable public class JavaScriptModuleRegistration { - private final int mModuleId; private final Class mModuleInterface; - private final Map mMethodsToIds; private final Map mMethodsToTracingNames; - public JavaScriptModuleRegistration(int moduleId, Class moduleInterface) { - mModuleId = moduleId; + public JavaScriptModuleRegistration(Class moduleInterface) { mModuleInterface = moduleInterface; + mMethodsToTracingNames = new HashMap<>(); - mMethodsToIds = MapBuilder.newHashMap(); - mMethodsToTracingNames = MapBuilder.newHashMap(); - final Method[] declaredMethods = mModuleInterface.getDeclaredMethods(); - Arrays.sort(declaredMethods, new Comparator() { - @Override - public int compare(Method lhs, Method rhs) { - return lhs.getName().compareTo(rhs.getName()); + if (ReactBuildConfig.DEBUG) { + Set methodNames = new LinkedHashSet<>(); + for (Method method : mModuleInterface.getDeclaredMethods()) { + if (!methodNames.add(method.getName())) { + throw new AssertionError( + "Method overloading is unsupported: " + mModuleInterface.getName() + "#" + + method.getName()); + } } - }); - - // Methods are sorted by name so we can dupe check and have obvious ordering - String previousName = null; - for (int i = 0; i < declaredMethods.length; i++) { - Method method = declaredMethods[i]; - String name = method.getName(); - Assertions.assertCondition( - !name.equals(previousName), - "Method overloading is unsupported: " + mModuleInterface.getName() + "#" + name); - previousName = name; - - mMethodsToIds.put(method, i); - mMethodsToTracingNames.put(method, "JSCall__" + getName() + "_" + method.getName()); } } - public int getModuleId() { - return mModuleId; - } - - public int getMethodId(Method method) { - final Integer id = mMethodsToIds.get(method); - if (id == null) { - Assertions.assertUnreachable("Unknown method: " + method.getName()); - } - return id.intValue(); - } - public String getTracingName(Method method) { - return Assertions.assertNotNull(mMethodsToTracingNames.get(method)); + String name = mMethodsToTracingNames.get(method); + if (name == null) { + name = "JSCall__" + getName() + "_" + method.getName(); + mMethodsToTracingNames.put(method, name); + } + return name; } public Class getModuleInterface() { @@ -92,7 +71,7 @@ public class JavaScriptModuleRegistration { return name; } - public Set getMethods() { - return mMethodsToIds.keySet(); + public List getMethods() { + return Arrays.asList(mModuleInterface.getDeclaredMethods()); } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaScriptModuleRegistry.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaScriptModuleRegistry.java index ad4f68b85..77e972ca3 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaScriptModuleRegistry.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaScriptModuleRegistry.java @@ -96,8 +96,8 @@ public class JavaScriptModuleRegistry { NativeArray jsArgs = args != null ? Arguments.fromJavaArgs(args) : new WritableNativeArray(); mCatalystInstance.callFunction( executorToken, - mModuleRegistration.getModuleId(), - mModuleRegistration.getMethodId(method), + mModuleRegistration.getName(), + method.getName(), jsArgs, tracingName); return null; diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaScriptModulesConfig.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaScriptModulesConfig.java index fb50c0102..42e5f37ad 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaScriptModulesConfig.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaScriptModulesConfig.java @@ -42,11 +42,11 @@ public class JavaScriptModulesConfig { private void appendJSModuleToJSONObject( JsonWriter writer, JavaScriptModuleRegistration registration) throws IOException { - writer.name("moduleID").value(registration.getModuleId()); + writer.name("moduleID").value(registration.getName()); writer.name("methods").beginObject(); for (Method method : registration.getMethods()) { writer.name(method.getName()).beginObject(); - writer.name("methodID").value(registration.getMethodId(method)); + writer.name("methodID").value(method.getName()); writer.endObject(); } writer.endObject(); @@ -56,14 +56,11 @@ public class JavaScriptModulesConfig { } public static class Builder { - - private int mLastJSModuleId = 0; private List mModules = new ArrayList(); public Builder add(Class moduleInterfaceClass) { - int moduleId = mLastJSModuleId++; - mModules.add(new JavaScriptModuleRegistration(moduleId, moduleInterfaceClass)); + mModules.add(new JavaScriptModuleRegistration(moduleInterfaceClass)); return this; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactBridge.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactBridge.java index 3c8128244..cbdebb4f3 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactBridge.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactBridge.java @@ -79,7 +79,7 @@ public class ReactBridge extends Countable { */ public native void loadScriptFromAssets(AssetManager assetManager, String assetName); public native void loadScriptFromFile(@Nullable String fileName, @Nullable String sourceURL); - public native void callFunction(ExecutorToken executorToken, int moduleId, int methodId, NativeArray arguments, String tracingName); + public native void callFunction(ExecutorToken executorToken, String module, String method, NativeArray arguments, String tracingName); public native void invokeCallback(ExecutorToken executorToken, int callbackID, NativeArray arguments); public native void setGlobalVariable(String propertyName, String jsonEncodedArgument); public native boolean supportsProfiling(); diff --git a/ReactAndroid/src/main/jni/react/Bridge.h b/ReactAndroid/src/main/jni/react/Bridge.h index 9c08da7a6..9fa3413ec 100644 --- a/ReactAndroid/src/main/jni/react/Bridge.h +++ b/ReactAndroid/src/main/jni/react/Bridge.h @@ -66,8 +66,8 @@ public: */ void callFunction( ExecutorToken executorToken, - const std::string& moduleId, - const std::string& methodId, + const std::string& module, + const std::string& method, const folly::dynamic& args, const std::string& tracingName); diff --git a/ReactAndroid/src/main/jni/react/jni/OnLoad.cpp b/ReactAndroid/src/main/jni/react/jni/OnLoad.cpp index 478eda666..ee9aed7f4 100644 --- a/ReactAndroid/src/main/jni/react/jni/OnLoad.cpp +++ b/ReactAndroid/src/main/jni/react/jni/OnLoad.cpp @@ -776,15 +776,15 @@ static void loadScriptFromFile(JNIEnv* env, jobject obj, jstring fileName, jstri env->CallStaticVoidMethod(markerClass, gLogMarkerMethod, env->NewStringUTF("loadScriptFromFile_exec")); } -static void callFunction(JNIEnv* env, jobject obj, JExecutorToken::jhybridobject jExecutorToken, jint moduleId, jint methodId, +static void callFunction(JNIEnv* env, jobject obj, JExecutorToken::jhybridobject jExecutorToken, jstring module, jstring method, NativeArray::jhybridobject args, jstring tracingName) { auto bridge = extractRefPtr(env, obj); auto arguments = cthis(wrap_alias(args)); try { bridge->callFunction( cthis(wrap_alias(jExecutorToken))->getExecutorToken(wrap_alias(jExecutorToken)), - folly::to(moduleId), - folly::to(methodId), + fromJString(env, module), + fromJString(env, method), std::move(arguments->array), fromJString(env, tracingName) ); diff --git a/ReactAndroid/src/test/java/com/facebook/react/bridge/JavaScriptModuleConfigTest.java b/ReactAndroid/src/test/java/com/facebook/react/bridge/JavaScriptModuleConfigTest.java index e87ad92f1..616a240d6 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/bridge/JavaScriptModuleConfigTest.java +++ b/ReactAndroid/src/test/java/com/facebook/react/bridge/JavaScriptModuleConfigTest.java @@ -49,11 +49,11 @@ public class JavaScriptModuleConfigTest { JsonNode intMethodNode = methods.get("intMethod"); assertThat(intMethodNode).isNotNull(); - assertThat(intMethodNode.get("methodID").asInt()).isEqualTo(0); + assertThat(intMethodNode.get("methodID").asText()).isEqualTo("intMethod"); JsonNode stringMethod = methods.get("stringMethod"); assertThat(stringMethod).isNotNull(); - assertThat(stringMethod.get("methodID").asInt()).isEqualTo(1); + assertThat(stringMethod.get("methodID").asText()).isEqualTo("stringMethod"); } @Test @@ -69,11 +69,11 @@ public class JavaScriptModuleConfigTest { JsonNode someModuleNode = node.get("SomeModule"); assertThat(someModuleNode).isNotNull(); - int someModuleID = someModuleNode.get("moduleID").asInt(); + String someModuleID = someModuleNode.get("moduleID").asText(); JsonNode otherModuleNode = node.get("OtherModule"); assertThat(otherModuleNode).isNotNull(); - int otherModuleID = otherModuleNode.get("moduleID").asInt(); + String otherModuleID = otherModuleNode.get("moduleID").asText(); assertThat(otherModuleID) .isNotEqualTo(someModuleID); }