Cleanup and document native module configuration

Summary: Get rid of the old behaviour of JSON encoding in `nativeRequireModuleConfig` and consistently use the same names for function types "async/promise/sync"

Reviewed By: lexs

Differential Revision: D3819348

fbshipit-source-id: fc798a5abcaf6a3ef9d95bd8654afa7825c83967

Libraries/BatchedBridge/BatchedBridgedModules/NativeModules.js

@@ -20,6 +20,7 @@ function normalizePrefix(moduleName: string): string {
 
 /**
  * Dirty hack to support old (RK) and new (RCT) native module name conventions.
+ * TODO 10487027: kill this behaviour
  */
 Object.keys(RemoteModules).forEach((moduleName) => {
   const strippedName = normalizePrefix(moduleName);
@@ -46,11 +47,7 @@ Object.keys(RemoteModules).forEach((moduleName) => {
     get: () => {
       let module = RemoteModules[moduleName];
       if (module && typeof module.moduleID === 'number' && global.nativeRequireModuleConfig) {
-        // The old bridge (still used by iOS) will send the config as
+        const config = global.nativeRequireModuleConfig(moduleName);
-        //  a JSON string that needs parsing, so we set config according
-        //  to the type of response we got.
-        const rawConfig = global.nativeRequireModuleConfig(moduleName);
-        const config = typeof rawConfig === 'string' ? JSON.parse(rawConfig) : rawConfig;
         module = config && BatchedBridge.processModuleConfig(config, module.moduleID);
         RemoteModules[moduleName] = module;
       }

Libraries/Utilities/MessageQueue.js

@@ -34,9 +34,9 @@ const TRACE_TAG_REACT_APPS = 1 << 17;
 const DEBUG_INFO_LIMIT = 32;
 
 const MethodTypes = keyMirror({
-  remote: null,
+  async: null,
-  remoteAsync: null,
+  promise: null,
-  syncHook: null,
+  sync: null,
 });
 
 const guard = (fn) => {
@@ -78,15 +78,7 @@ class MessageQueue {
     lazyProperty(this, 'RemoteModules', () => {
       const {remoteModuleConfig} = configProvider();
       const modulesConfig = remoteModuleConfig;
-      const modules = this._genModules(modulesConfig);
+      return this._genModules(modulesConfig);
-
-      if (__DEV__) {
-        this._genLookupTables(
-          modulesConfig, this._remoteModuleTable, this._remoteMethodTable
-        );
-      }
-
-      return modules;
     });
   }
 
@@ -148,7 +140,7 @@ class MessageQueue {
     const info = this._genModule(config, moduleID);
     this.RemoteModules[info.name] = info.module;
     if (__DEV__) {
-      this._genLookup(config, moduleID, this._remoteModuleTable, this._remoteMethodTable);
+      this._createDebugLookup(config, moduleID, this._remoteModuleTable, this._remoteMethodTable);
     }
     return info.module;
   }
@@ -157,10 +149,13 @@ class MessageQueue {
     return new Date().getTime() - this._eventLoopStartTime;
   }
 
+  registerCallableModule(name, methods) {
+    this._callableModules[name] = methods;
+  }
+
   /**
    * "Private" methods
    */
-
   __callImmediates() {
     Systrace.beginEvent('JSTimersExecution.callImmediates()');
     guard(() => JSTimersExecution.callImmediates());
@@ -181,7 +176,6 @@ class MessageQueue {
       onSucc && params.push(this._callbackID);
       this._callbacks[this._callbackID++] = onSucc;
     }
-    var preparedParams = this._serializeNativeParams ? JSON.stringify(params) : params;
 
     if (__DEV__) {
       global.nativeTraceBeginAsyncFlow &&
@@ -191,6 +185,8 @@ class MessageQueue {
 
     this._queue[MODULE_IDS].push(module);
     this._queue[METHOD_IDS].push(method);
+
+    const preparedParams = this._serializeNativeParams ? JSON.stringify(params) : params;
     this._queue[PARAMS].push(preparedParams);
 
     const now = new Date().getTime();
@@ -279,13 +275,91 @@ class MessageQueue {
    * Private helper methods
    */
 
-  _genLookupTables(modulesConfig, moduleTable, methodTable) {
+  _genModules(remoteModules) {
-    modulesConfig.forEach((config, moduleID) => {
+    const modules = {};
-      this._genLookup(config, moduleID, moduleTable, methodTable);
+    remoteModules.forEach((config, moduleID) => {
+      // Initially this config will only contain the module name when running in JSC. The actual
+      // configuration of the module will be lazily loaded (see NativeModules.js) and updated
+      // through processModuleConfig.
+      const info = this._genModule(config, moduleID);
+      if (info) {
+        modules[info.name] = info.module;
+      }
+
+      if (__DEV__) {
+        this._createDebugLookup(config, moduleID, this._remoteModuleTable, this._remoteMethodTable);
+      }
     });
+    return modules;
   }
 
-  _genLookup(config, moduleID, moduleTable, methodTable) {
+  _genModule(config, moduleID): ?Object {
+    if (!config) {
+      return null;
+    }
+
+    let moduleName, constants, methods, promiseMethods, syncMethods;
+    if (moduleHasConstants(config)) {
+      [moduleName, constants, methods, promiseMethods, syncMethods] = config;
+    } else {
+      [moduleName, methods, promiseMethods, syncMethods] = config;
+    }
+
+    const module = {};
+    methods && methods.forEach((methodName, methodID) => {
+      const isPromise = promiseMethods && arrayContains(promiseMethods, methodID);
+      const isSync = syncMethods && arrayContains(syncMethods, methodID);
+      invariant(!isPromise || !isSync, 'Cannot have a method that is both async and a sync hook');
+      const methodType = isPromise ? MethodTypes.promise :
+        isSync ? MethodTypes.sync : MethodTypes.async;
+      module[methodName] = this._genMethod(moduleID, methodID, methodType);
+    });
+    Object.assign(module, constants);
+
+    if (!constants && !methods && !promiseMethods) {
+      module.moduleID = moduleID;
+    }
+
+    return { name: moduleName, module };
+  }
+
+  _genMethod(module, method, type) {
+    let fn = null;
+    const self = this;
+    if (type === MethodTypes.promise) {
+      fn = function(...args) {
+        return new Promise((resolve, reject) => {
+          self.__nativeCall(module, method, args,
+            (data) => resolve(data),
+            (errorData) => reject(createErrorFromErrorData(errorData)));
+        });
+      };
+    } else if (type === MethodTypes.sync) {
+      fn = function(...args) {
+        return global.nativeCallSyncHook(module, method, args);
+      };
+    } else {
+      fn = function(...args) {
+        const lastArg = args.length > 0 ? args[args.length - 1] : null;
+        const secondLastArg = args.length > 1 ? args[args.length - 2] : null;
+        const hasSuccessCallback = typeof lastArg === 'function';
+        const hasErrorCallback = typeof secondLastArg === 'function';
+        hasErrorCallback && invariant(
+          hasSuccessCallback,
+          'Cannot have a non-function arg after a function arg.'
+        );
+        const onSuccess = hasSuccessCallback ? lastArg : null;
+        const onFail = hasErrorCallback ? secondLastArg : null;
+        const callbackCount = hasSuccessCallback + hasErrorCallback;
+        args = args.slice(0, args.length - callbackCount);
+        return self.__nativeCall(module, method, args, onFail, onSuccess);
+      };
+    }
+    fn.type = type;
+    return fn;
+  }
+
+  _createDebugLookup(config, moduleID, moduleTable, methodTable) {
     if (!config) {
       return;
     }
@@ -300,99 +374,6 @@ class MessageQueue {
     moduleTable[moduleID] = moduleName;
     methodTable[moduleID] = Object.assign({}, methods);
   }
-
-  _genModules(remoteModules) {
-    const modules = {};
-
-    remoteModules.forEach((config, moduleID) => {
-      const info = this._genModule(config, moduleID);
-      if (info) {
-        modules[info.name] = info.module;
-      }
-    });
-
-    return modules;
-  }
-
-  _genModule(config, moduleID): ?Object {
-    if (!config) {
-      return null;
-    }
-
-    let moduleName, constants, methods, asyncMethods, syncHooks;
-    if (moduleHasConstants(config)) {
-      [moduleName, constants, methods, asyncMethods, syncHooks] = config;
-    } else {
-      [moduleName, methods, asyncMethods, syncHooks] = config;
-    }
-
-    const module = {};
-    methods && methods.forEach((methodName, methodID) => {
-      const isAsync = asyncMethods && arrayContains(asyncMethods, methodID);
-      const isSyncHook = syncHooks && arrayContains(syncHooks, methodID);
-      invariant(!isAsync || !isSyncHook, 'Cannot have a method that is both async and a sync hook');
-      const methodType = isAsync ? MethodTypes.remoteAsync :
-          isSyncHook ? MethodTypes.syncHook :
-          MethodTypes.remote;
-      module[methodName] = this._genMethod(moduleID, methodID, methodType);
-    });
-    Object.assign(module, constants);
-
-    if (!constants && !methods && !asyncMethods) {
-      module.moduleID = moduleID;
-    }
-
-    return { name: moduleName, module };
-  }
-
-  _genMethod(module, method, type) {
-    let fn = null;
-    const self = this;
-    if (type === MethodTypes.remoteAsync) {
-      fn = function(...args) {
-        return new Promise((resolve, reject) => {
-          self.__nativeCall(
-            module,
-            method,
-            args,
-            (data) => {
-              resolve(data);
-            },
-            (errorData) => {
-              var error = createErrorFromErrorData(errorData);
-              reject(error);
-            });
-        });
-      };
-    } else if (type === MethodTypes.syncHook) {
-      return function(...args) {
-        return global.nativeCallSyncHook(module, method, args);
-      };
-    } else {
-      fn = function(...args) {
-        const lastArg = args.length > 0 ? args[args.length - 1] : null;
-        const secondLastArg = args.length > 1 ? args[args.length - 2] : null;
-        const hasSuccCB = typeof lastArg === 'function';
-        const hasErrorCB = typeof secondLastArg === 'function';
-        hasErrorCB && invariant(
-          hasSuccCB,
-          'Cannot have a non-function arg after a function arg.'
-        );
-        const numCBs = hasSuccCB + hasErrorCB;
-        const onSucc = hasSuccCB ? lastArg : null;
-        const onFail = hasErrorCB ? secondLastArg : null;
-        args = args.slice(0, args.length - numCBs);
-        return self.__nativeCall(module, method, args, onFail, onSucc);
-      };
-    }
-    fn.type = type;
-    return fn;
-  }
-
-  registerCallableModule(name, methods) {
-    this._callableModules[name] = methods;
-  }
-
 }
 
 function moduleHasConstants(moduleArray: Array<Object|Array<>>): boolean {

React/Executors/RCTJSCExecutor.mm

@@ -423,15 +423,14 @@ static NSThread *newJavaScriptThread(void)
 
     __weak RCTJSCExecutor *weakSelf = self;
 
-    context[@"nativeRequireModuleConfig"] = ^NSString *(NSString *moduleName) {
+    context[@"nativeRequireModuleConfig"] = ^NSArray *(NSString *moduleName) {
       RCTJSCExecutor *strongSelf = weakSelf;
       if (!strongSelf.valid) {
         return nil;
       }
 
       RCT_PROFILE_BEGIN_EVENT(RCTProfileTagAlways, @"nativeRequireModuleConfig", @{ @"moduleName": moduleName });
-      NSArray *config = [strongSelf->_bridge configForModuleName:moduleName];
+      NSArray *result = [strongSelf->_bridge configForModuleName:moduleName];
-      NSString *result = config ? RCTJSONStringify(config, NULL) : nil;
       RCT_PROFILE_END_EVENT(RCTProfileTagAlways, @"js_call,config");
       return result;
     };

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

@@ -50,9 +50,9 @@ import static com.facebook.systrace.Systrace.TRACE_TAG_REACT_JAVA_BRIDGE;
  */
 public abstract class BaseJavaModule implements NativeModule {
   // taken from Libraries/Utilities/MessageQueue.js
-  static final public String METHOD_TYPE_REMOTE = "remote";
+  static final public String METHOD_TYPE_ASYNC = "async";
-  static final public String METHOD_TYPE_REMOTE_ASYNC = "remoteAsync";
+  static final public String METHOD_TYPE_PROMISE= "promise";
-  static final public String METHOD_TYPE_SYNC_HOOK = "syncHook";
+  static final public String METHOD_TYPE_SYNC = "sync";
 
   private static abstract class ArgumentExtractor<T> {
     public int getJSArgumentsNeeded() {
@@ -164,7 +164,7 @@ public abstract class BaseJavaModule implements NativeModule {
     private final ArgumentExtractor[] mArgumentExtractors;
     private final String mSignature;
     private final Object[] mArguments;
-    private String mType = METHOD_TYPE_REMOTE;
+    private String mType = METHOD_TYPE_ASYNC;
     private final int mJSArgumentsNeeded;
     private final String mTraceName;
 
@@ -203,7 +203,7 @@ public abstract class BaseJavaModule implements NativeModule {
         } else if (paramClass == Promise.class) {
           Assertions.assertCondition(
             i == paramTypes.length - 1, "Promise must be used as last parameter only");
-          mType = METHOD_TYPE_REMOTE_ASYNC;
+          mType = METHOD_TYPE_PROMISE;
         }
         builder.append(paramTypeToChar(paramClass));
       }
@@ -254,7 +254,7 @@ public abstract class BaseJavaModule implements NativeModule {
           argumentExtractors[i] = ARGUMENT_EXTRACTOR_PROMISE;
           Assertions.assertCondition(
               paramIndex == paramTypes.length - 1, "Promise must be used as last parameter only");
-          mType = METHOD_TYPE_REMOTE_ASYNC;
+          mType = METHOD_TYPE_PROMISE;
         } else if (argumentClass == ReadableMap.class) {
           argumentExtractors[i] = ARGUMENT_EXTRACTOR_MAP;
         } else if (argumentClass == ReadableArray.class) {
@@ -339,8 +339,8 @@ public abstract class BaseJavaModule implements NativeModule {
 
     /**
      * Determines how the method is exported in JavaScript:
-     * METHOD_TYPE_REMOTE for regular methods
+     * METHOD_TYPE_ASYNC for regular methods
-     * METHOD_TYPE_REMOTE_ASYNC for methods that return a promise object to the caller.
+     * METHOD_TYPE_PROMISE for methods that return a promise object to the caller.
      */
     @Override
     public String getType() {

ReactAndroid/src/main/java/com/facebook/react/bridge/Promise.java

@@ -16,7 +16,7 @@ import javax.annotation.Nullable;
  * method parameter.
  *
  * Methods annotated with {@link ReactMethod} that use {@link Promise} as type of the last parameter
- * will be marked as "remoteAsync" and will return a promise when invoked from JavaScript.
+ * will be marked as "promise" and will return a promise when invoked from JavaScript.
  */
 public interface Promise {
 

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

@@ -108,7 +108,7 @@ import static com.facebook.systrace.Systrace.TRACE_TAG_REACT_JAVA_BRIDGE;
         mModule.getSyncHooks().entrySet()) {
       MethodDescriptor md = new MethodDescriptor();
       md.name = entry.getKey();
-      md.type = BaseJavaModule.METHOD_TYPE_SYNC_HOOK;
+      md.type = BaseJavaModule.METHOD_TYPE_SYNC;
 
       BaseJavaModule.SyncJavaHook method = (BaseJavaModule.SyncJavaHook) entry.getValue();
       md.method = method.getMethod();

ReactAndroid/src/main/jni/xreact/jni/CxxModuleWrapper.cpp

@@ -53,9 +53,9 @@ public:
 
   std::string getType() {
     if (method_->func) {
-      return "remote";
+      return "async";
     } else {
-      return "syncHook";
+      return "sync";
     }
   }
 

ReactCommon/cxxreact/CxxNativeModule.cpp

@@ -37,14 +37,10 @@ std::string CxxNativeModule::getName() {
 }
 
 std::vector<MethodDescriptor> CxxNativeModule::getMethods() {
-  // Same as MessageQueue.MethodTypes.remote
-  static const auto kMethodTypeRemote = "remote";
-  static const auto kMethodTypeSyncHook = "syncHook";
-
   std::vector<MethodDescriptor> descs;
   for (auto& method : methods_) {
     assert(method.func || method.syncFunc);
-    descs.emplace_back(method.name, method.func ? kMethodTypeRemote : kMethodTypeSyncHook);
+    descs.emplace_back(method.name, method.func ? "async" : "sync");
   }
   return descs;
 }

ReactCommon/cxxreact/ModuleRegistry.cpp

@@ -48,7 +48,7 @@ folly::dynamic ModuleRegistry::getConfig(const std::string& name) {
 
   NativeModule* module = modules_[it->second].get();
 
-  // string name, [object constants,] array methodNames (methodId is index), [array asyncMethodIds]
+  // string name, [object constants,] array methodNames (methodId is index), [array promiseMethodIds], [array syncMethodIds]
   folly::dynamic config = folly::dynamic::array(name);
 
   {
@@ -64,23 +64,26 @@ folly::dynamic ModuleRegistry::getConfig(const std::string& name) {
     std::vector<MethodDescriptor> methods = module->getMethods();
 
     folly::dynamic methodNames = folly::dynamic::array;
-    folly::dynamic asyncMethodIds = folly::dynamic::array;
+    folly::dynamic promiseMethodIds = folly::dynamic::array;
-    folly::dynamic syncHookIds = folly::dynamic::array;
+    folly::dynamic syncMethodIds = folly::dynamic::array;
 
     for (auto& descriptor : methods) {
+      // TODO: #10487027 compare tags instead of doing string comparison?
       methodNames.push_back(std::move(descriptor.name));
-      if (descriptor.type == "remoteAsync") {
+      if (descriptor.type == "promise") {
-        asyncMethodIds.push_back(methodNames.size() - 1);
+        promiseMethodIds.push_back(methodNames.size() - 1);
-      } else if (descriptor.type == "syncHook") {
+      } else if (descriptor.type == "sync") {
-        syncHookIds.push_back(methodNames.size() - 1);
+        syncMethodIds.push_back(methodNames.size() - 1);
       }
     }
 
     if (!methodNames.empty()) {
       config.push_back(std::move(methodNames));
-      config.push_back(std::move(asyncMethodIds));
+      if (!promiseMethodIds.empty() || !syncMethodIds.empty()) {
-      if (!syncHookIds.empty()) {
+        config.push_back(std::move(promiseMethodIds));
-        config.push_back(std::move(syncHookIds));
+        if (!syncMethodIds.empty()) {
+          config.push_back(std::move(syncMethodIds));
+        }
       }
     }
   }