Support callbacks in synchronous native functions

Summary:
We need to move animated native module calls to synchronous so we can properly thread them in with mounting instructions in Fabric. Some of them take callbacks so we need to add support for that when switching to synchronous.

A side benefit is that we can unify codepaths a little more with async callbacks.

Reviewed By: shergin

Differential Revision: D14790898

fbshipit-source-id: dc222b9e74375e046e8a9b1b19d72f652dc6722c

IntegrationTests/SyncMethodTest.js

@@ -21,12 +21,26 @@ class SyncMethodTest extends React.Component<{}> {
     if (
       RNTesterTestModule.echoString('test string value') !== 'test string value'
     ) {
-      throw new Error('Something wrong with sync method export');
+      throw new Error('Something wrong with echoString sync method');
     }
     if (RNTesterTestModule.methodThatReturnsNil() != null) {
-      throw new Error('Something wrong with sync method export');
+      throw new Error('Something wrong with methodThatReturnsNil sync method');
     }
-    TestModule.markTestCompleted();
+    let response;
+    RNTesterTestModule.methodThatCallsCallbackWithString('test', echo => {
+      response = echo;
+    });
+    requestAnimationFrame(() => {
+      if (response === 'test') {
+        TestModule.markTestCompleted();
+      } else {
+        throw new Error(
+          'Something wrong with methodThatCallsCallbackWithString sync method, ' +
+            'got response ' +
+            JSON.stringify(response),
+        );
+      }
+    });
   }
 
   render(): React.Node {

Libraries/BatchedBridge/MessageQueue.js

@@ -165,7 +165,27 @@ class MessageQueue {
     return getValue ? getValue() : null;
   }
 
-  enqueueNativeCall(
+  callNativeSyncHook(
+    moduleID: number,
+    methodID: number,
+    params: any[],
+    onFail: ?Function,
+    onSucc: ?Function,
+  ) {
+    if (__DEV__) {
+      invariant(
+        global.nativeCallSyncHook,
+        'Calling synchronous methods on native ' +
+          'modules is not supported in Chrome.\n\n Consider providing alternative ' +
+          'methods to expose this method in debug mode, e.g. by exposing constants ' +
+          'ahead-of-time.',
+      );
+    }
+    this.processCallbacks(moduleID, methodID, params, onFail, onSucc);
+    return global.nativeCallSyncHook(moduleID, methodID, params);
+  }
+
+  processCallbacks(
     moduleID: number,
     methodID: number,
     params: any[],
@@ -188,7 +208,6 @@ class MessageQueue {
       this._successCallbacks[this._callID] = onSucc;
       this._failureCallbacks[this._callID] = onFail;
     }
-
     if (__DEV__) {
       global.nativeTraceBeginAsyncFlow &&
         global.nativeTraceBeginAsyncFlow(
@@ -198,6 +217,16 @@ class MessageQueue {
         );
     }
     this._callID++;
+  }
+
+  enqueueNativeCall(
+    moduleID: number,
+    methodID: number,
+    params: any[],
+    onFail: ?Function,
+    onSucc: ?Function,
+  ) {
+    this.processCallbacks(moduleID, methodID, params, onFail, onSucc);
 
     this._queue[MODULE_IDS].push(moduleID);
     this._queue[METHOD_IDS].push(methodID);
@@ -385,15 +414,14 @@ class MessageQueue {
       const debug = this._debugInfo[callID];
       const module = debug && this._remoteModuleTable[debug[0]];
       const method = debug && this._remoteMethodTable[debug[0]][debug[1]];
-      if (!callback) {
-        let errorMessage = `Callback with id ${cbID}: ${module}.${method}() not found`;
-        if (method) {
-          errorMessage =
-            `The callback ${method}() exists in module ${module}, ` +
-            'but only one callback may be registered to a function in a native module.';
-        }
-        invariant(callback, errorMessage);
-      }
+      invariant(
+        callback,
+        `No callback found with cbID ${cbID} and callID ${callID} for ` +
+          (method
+            ? ` ${module}.${method} - most likely the callback was already invoked`
+            : `module ${module || '<unknown>'}`) +
+          `. Args: '${stringifySafe(args)}'`,
+      );
       const profileName = debug
         ? '<callback for ' + module + '.' + method + '>'
         : cbID;

Libraries/BatchedBridge/NativeModules.js

@@ -105,19 +105,6 @@ function genMethod(moduleID: number, methodID: number, type: MethodType) {
         );
       });
     };
-  } else if (type === 'sync') {
-    fn = function(...args: Array<any>) {
-      if (__DEV__) {
-        invariant(
-          global.nativeCallSyncHook,
-          'Calling synchronous methods on native ' +
-            'modules is not supported in Chrome.\n\n Consider providing alternative ' +
-            'methods to expose this method in debug mode, e.g. by exposing constants ' +
-            'ahead-of-time.',
-        );
-      }
-      return global.nativeCallSyncHook(moduleID, methodID, args);
-    };
   } else {
     fn = function(...args: Array<any>) {
       const lastArg = args.length > 0 ? args[args.length - 1] : null;
@@ -133,13 +120,23 @@ function genMethod(moduleID: number, methodID: number, type: MethodType) {
       const onFail = hasErrorCallback ? secondLastArg : null;
       const callbackCount = hasSuccessCallback + hasErrorCallback;
       args = args.slice(0, args.length - callbackCount);
-      BatchedBridge.enqueueNativeCall(
-        moduleID,
-        methodID,
-        args,
-        onFail,
-        onSuccess,
-      );
+      if (type === 'sync') {
+        return BatchedBridge.callNativeSyncHook(
+          moduleID,
+          methodID,
+          args,
+          onFail,
+          onSuccess,
+        );
+      } else {
+        BatchedBridge.enqueueNativeCall(
+          moduleID,
+          methodID,
+          args,
+          onFail,
+          onSuccess,
+        );
+      }
     };
   }
   fn.type = type;

RNTester/RNTesterIntegrationTests/RNTesterTestModule.m

@@ -27,4 +27,10 @@ RCT_EXPORT_BLOCKING_SYNCHRONOUS_METHOD(methodThatReturnsNil)
   return nil;
 }
 
+RCT_EXPORT_BLOCKING_SYNCHRONOUS_METHOD(methodThatCallsCallbackWithString:(NSString *)input callback:(RCTResponseSenderBlock)callback)
+{
+  callback(@[input]);
+  return nil;
+}
+
 @end