Fix TurboModule binding improvements for TurboCxxModule

Summary:
TurboCxxModule doesn't use the the default JSI getter that TurboModule offers, and has custom behaviour for `getConstants` which broke the I18nAssets module in the binding experiment.

Changelog: [Internal]

Reviewed By: ryancat

Differential Revision: D37030917

fbshipit-source-id: 187f159abc76f792ad2c7045dc2852d216ea978d

ReactCommon/react/nativemodule/core/ReactCommon/TurboCxxModule.cpp

@@ -60,7 +60,7 @@ CxxModule::Callback makeTurboCxxModuleCallback(
 TurboCxxModule::TurboCxxModule(
     std::unique_ptr<CxxModule> cxxModule,
     std::shared_ptr<CallInvoker> jsInvoker)
-    : TurboModule(cxxModule->getName(), jsInvoker),
+    : TurboModule(cxxModule->getName(), std::move(jsInvoker)),
       cxxMethods_(cxxModule->getMethods()),
       cxxModule_(std::move(cxxModule)) {}
 
@@ -69,10 +69,12 @@ jsi::Value TurboCxxModule::get(
     const jsi::PropNameID &propName) {
   std::string propNameUtf8 = propName.utf8(runtime);
 
+  auto result = jsi::Value::undefined();
+
   if (propNameUtf8 == "getConstants") {
     // This is special cased because `getConstants()` is already a part of
     // CxxModule.
-    return jsi::Function::createFromHostFunction(
+    result = jsi::Function::createFromHostFunction(
         runtime,
         propName,
         0,
@@ -89,30 +91,45 @@ jsi::Value TurboCxxModule::get(
           }
           return result;
         });
+  } else {
+    for (auto &method : cxxMethods_) {
+      if (method.name == propNameUtf8) {
+        result = jsi::Function::createFromHostFunction(
+            runtime,
+            propName,
+            0,
+            [this, propNameUtf8](
+                jsi::Runtime &rt,
+                const jsi::Value &thisVal,
+                const jsi::Value *args,
+                size_t count) {
+              return invokeMethod(rt, propNameUtf8, args, count);
+            });
+      }
+    }
   }
 
-  for (auto &method : cxxMethods_) {
-    if (method.name == propNameUtf8) {
-      return jsi::Function::createFromHostFunction(
-          runtime,
-          propName,
-          0,
-          [this, propNameUtf8](
-              jsi::Runtime &rt,
-              const jsi::Value &thisVal,
-              const jsi::Value *args,
-              size_t count) {
-            return invokeMethod(rt, VoidKind, propNameUtf8, args, count);
-          });
-    }
+  // If we have a JS wrapper, cache the result of this lookup
+  if (jsRepresentation_) {
+    jsRepresentation_->setProperty(runtime, propName, result);
   }
 
-  return jsi::Value::undefined();
+  return result;
+}
+
+std::vector<jsi::PropNameID> TurboCxxModule::getPropertyNames(
+    jsi::Runtime &runtime) {
+  std::vector<jsi::PropNameID> result;
+  result.reserve(cxxMethods_.size() + 1);
+  result.push_back(jsi::PropNameID::forUtf8(runtime, "getConstants"));
+  for (auto it = cxxMethods_.begin(); it != cxxMethods_.end(); it++) {
+    result.push_back(jsi::PropNameID::forUtf8(runtime, it->name));
+  }
+  return result;
 }
 
 jsi::Value TurboCxxModule::invokeMethod(
     jsi::Runtime &runtime,
-    TurboModuleMethodValueKind valueKind,
     const std::string &methodName,
     const jsi::Value *args,
     size_t count) {

ReactCommon/react/nativemodule/core/ReactCommon/TurboCxxModule.h

@@ -29,13 +29,15 @@ class JSI_EXPORT TurboCxxModule : public TurboModule {
       std::unique_ptr<facebook::xplat::module::CxxModule> cxxModule,
       std::shared_ptr<CallInvoker> jsInvoker);
 
-  virtual facebook::jsi::Value get(
+  facebook::jsi::Value get(
       facebook::jsi::Runtime &runtime,
       const facebook::jsi::PropNameID &propName) override;
 
+  std::vector<facebook::jsi::PropNameID> getPropertyNames(
+      facebook::jsi::Runtime &runtime) override;
+
   jsi::Value invokeMethod(
       jsi::Runtime &runtime,
-      TurboModuleMethodValueKind valueKind,
       const std::string &methodName,
       const jsi::Value *args,
       size_t count);

ReactCommon/react/nativemodule/core/ReactCommon/TurboModule.h

@@ -32,6 +32,7 @@ enum TurboModuleMethodValueKind {
   PromiseKind,
 };
 
+class TurboCxxModule;
 class TurboModuleBinding;
 
 /**
@@ -58,10 +59,20 @@ class JSI_EXPORT TurboModule : public facebook::jsi::HostObject {
     }
   }
 
+  std::vector<facebook::jsi::PropNameID> getPropertyNames(
+      facebook::jsi::Runtime &runtime) override {
+    std::vector<jsi::PropNameID> result;
+    result.reserve(methodMap_.size());
+    for (auto it = methodMap_.cbegin(); it != methodMap_.cend(); ++it) {
+      result.push_back(jsi::PropNameID::forUtf8(runtime, it->first));
+    }
+    return result;
+  }
+
+ protected:
   const std::string name_;
   std::shared_ptr<CallInvoker> jsInvoker_;
 
- protected:
   struct MethodMetadata {
     size_t argCount;
     facebook::jsi::Value (*invoker)(
@@ -70,17 +81,17 @@ class JSI_EXPORT TurboModule : public facebook::jsi::HostObject {
         const facebook::jsi::Value *args,
         size_t count);
   };
-
-  facebook::jsi::Value get(
-      facebook::jsi::Runtime &runtime,
-      const facebook::jsi::PropNameID &propName,
-      const MethodMetadata &meta);
-
   std::unordered_map<std::string, MethodMetadata> methodMap_;
 
  private:
+  friend class TurboCxxModule;
   friend class TurboModuleBinding;
   std::unique_ptr<jsi::Object> jsRepresentation_;
+
+  facebook::jsi::Value get(
+      facebook::jsi::Runtime &runtime,
+      const facebook::jsi::PropNameID &propName,
+      const MethodMetadata &meta);
 };
 
 /**

ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleBinding.cpp

@@ -98,11 +98,8 @@ jsi::Value TurboModuleBinding::getModule(
       } else {
         // Option 2: eagerly install all hostfunctions at this point, avoids
         // prototype
-        for (auto it = module->methodMap_.cbegin();
-             it != module->methodMap_.cend();
-             ++it) {
-          auto propName = jsi::PropNameID::forUtf8(runtime, it->first);
-          module->get(runtime, propName, it->second);
+        for (auto &propName : module->getPropertyNames(runtime)) {
+          module->get(runtime, propName);
         }
       }
     }