From c75f062a7732e5d5edc81df7d55470bfc0d8522a Mon Sep 17 00:00:00 2001 From: Emily Janzer Date: Mon, 13 May 2019 11:58:51 -0700 Subject: [PATCH] Change JSCallInvoker to be an abstract class Summary: The existing implementation of JSCallInvoker holds a reference to Instance (aka the bridge). This diff makes JSCallInvoker an abstract base class that's extended by BridgeJSCallInvoker, which is what's returned by CatalystInstance to initialize TurboModules. This will allow us to add another class that derives from JSCallInvoker that doesn't rely on the bridge. Reviewed By: shergin Differential Revision: D15252375 fbshipit-source-id: 75eee2ca149235a63e2a2cd8cdd361b163d1d1ab --- .../jni/react/jni/CatalystInstanceImpl.cpp | 2 +- .../main/jni/react/jni/CatalystInstanceImpl.h | 3 +- ReactCommon/jscallinvoker/BUCK | 3 +- ...allInvoker.cpp => BridgeJSCallInvoker.cpp} | 6 +-- .../jsireact/BridgeJSCallInvoker.h | 42 +++++++++++++++++++ .../jscallinvoker/jsireact/JSCallInvoker.h | 21 +++------- .../platform/ios/RCTTurboModuleManager.mm | 3 +- 7 files changed, 57 insertions(+), 23 deletions(-) rename ReactCommon/jscallinvoker/jsireact/{JSCallInvoker.cpp => BridgeJSCallInvoker.cpp} (71%) create mode 100644 ReactCommon/jscallinvoker/jsireact/BridgeJSCallInvoker.h diff --git a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp index 0b1396d6cae..06a224f457a 100644 --- a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp +++ b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp @@ -268,7 +268,7 @@ void CatalystInstanceImpl::handleMemoryPressure(int pressureLevel) { jni::alias_ref CatalystInstanceImpl::getJSCallInvokerHolder() { if (!javaInstanceHolder_) { - jsCallInvoker_ = std::make_shared(instance_); + jsCallInvoker_ = std::make_shared(instance_); javaInstanceHolder_ = jni::make_global(JSCallInvokerHolder::newObjectCxxArgs(jsCallInvoker_)); } diff --git a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h index 826d55411ef..0ea967de16c 100644 --- a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h +++ b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h @@ -8,6 +8,7 @@ #include #include #include +#include #include "CxxModuleWrapper.h" #include "JavaModuleWrapper.h" @@ -86,7 +87,7 @@ class CatalystInstanceImpl : public jni::HybridClass { std::shared_ptr moduleRegistry_; std::shared_ptr moduleMessageQueue_; jni::global_ref javaInstanceHolder_; - std::shared_ptr jsCallInvoker_; + std::shared_ptr jsCallInvoker_; }; }} diff --git a/ReactCommon/jscallinvoker/BUCK b/ReactCommon/jscallinvoker/BUCK index a4b3523476f..2c0dc7bc251 100644 --- a/ReactCommon/jscallinvoker/BUCK +++ b/ReactCommon/jscallinvoker/BUCK @@ -3,10 +3,11 @@ load("//tools/build_defs/oss:rn_defs.bzl", "ANDROID", "APPLE", "react_native_xpl rn_xplat_cxx_library( name = "jscallinvoker", srcs = [ - "jsireact/JSCallInvoker.cpp", + "jsireact/BridgeJSCallInvoker.cpp", ], header_namespace = "", exported_headers = { + "jsireact/BridgeJSCallInvoker.h": "jsireact/BridgeJSCallInvoker.h", "jsireact/JSCallInvoker.h": "jsireact/JSCallInvoker.h", }, compiler_flags = [ diff --git a/ReactCommon/jscallinvoker/jsireact/JSCallInvoker.cpp b/ReactCommon/jscallinvoker/jsireact/BridgeJSCallInvoker.cpp similarity index 71% rename from ReactCommon/jscallinvoker/jsireact/JSCallInvoker.cpp rename to ReactCommon/jscallinvoker/jsireact/BridgeJSCallInvoker.cpp index c513697d450..ed44cf84c3c 100644 --- a/ReactCommon/jscallinvoker/jsireact/JSCallInvoker.cpp +++ b/ReactCommon/jscallinvoker/jsireact/BridgeJSCallInvoker.cpp @@ -5,16 +5,16 @@ * LICENSE file in the root directory of this source tree. */ -#include +#include #include namespace facebook { namespace react { -JSCallInvoker::JSCallInvoker(std::weak_ptr reactInstance) +BridgeJSCallInvoker::BridgeJSCallInvoker(std::weak_ptr reactInstance) : reactInstance_(reactInstance) {} -void JSCallInvoker::invokeAsync(std::function &&func) { +void BridgeJSCallInvoker::invokeAsync(std::function &&func) { auto instance = reactInstance_.lock(); if (instance == nullptr) { return; diff --git a/ReactCommon/jscallinvoker/jsireact/BridgeJSCallInvoker.h b/ReactCommon/jscallinvoker/jsireact/BridgeJSCallInvoker.h new file mode 100644 index 00000000000..bb0712747ed --- /dev/null +++ b/ReactCommon/jscallinvoker/jsireact/BridgeJSCallInvoker.h @@ -0,0 +1,42 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +#pragma once + +#include +#include + +#include + +namespace facebook { +namespace react { + +class Instance; + +/** + * A native-to-JS call invoker that uses the bridge ('Instance'). + * It guarantees that any calls from any thread are queued on the right JS + * thread. + * + * For now, this is a thin-wrapper around existing bridge. Eventually, + * it should be consolidated with Fabric implementation so there's only one + * API to call JS from native, whether synchronously or asynchronously. + * Also, this class should not depend on `Instance` in the future. + */ +class BridgeJSCallInvoker : public JSCallInvoker { + public: + BridgeJSCallInvoker(std::weak_ptr reactInstance); + + void invokeAsync(std::function &&func); + // TODO: add sync support + + private: + std::weak_ptr reactInstance_; +}; + +} // namespace react +} // namespace facebook diff --git a/ReactCommon/jscallinvoker/jsireact/JSCallInvoker.h b/ReactCommon/jscallinvoker/jsireact/JSCallInvoker.h index 770cdf2545d..501a32e7569 100644 --- a/ReactCommon/jscallinvoker/jsireact/JSCallInvoker.h +++ b/ReactCommon/jscallinvoker/jsireact/JSCallInvoker.h @@ -13,26 +13,15 @@ namespace facebook { namespace react { -class Instance; - /** - * A generic native-to-JS call invoker. It guarantees that any calls from any - * thread are queued on the right JS thread. - * - * For now, this is a thin-wrapper around existing bridge (`Instance`). Eventually, - * it should be consolidated with Fabric implementation so there's only one - * API to call JS from native, whether synchronously or asynchronously. - * Also, this class should not depend on `Instance` in the future. + * An interface for a generic native-to-JS call invoker. See BridgeJSCallInvoker + * for an implementation. */ class JSCallInvoker { -public: - JSCallInvoker(std::weak_ptr reactInstance); - - void invokeAsync(std::function&& func); + public: + virtual void invokeAsync(std::function &&func) = 0; // TODO: add sync support - -private: - std::weak_ptr reactInstance_; + virtual ~JSCallInvoker() {} }; } // namespace react diff --git a/ReactCommon/turbomodule/core/platform/ios/RCTTurboModuleManager.mm b/ReactCommon/turbomodule/core/platform/ios/RCTTurboModuleManager.mm index 2edfb8c10fa..09ad85b4a50 100644 --- a/ReactCommon/turbomodule/core/platform/ios/RCTTurboModuleManager.mm +++ b/ReactCommon/turbomodule/core/platform/ios/RCTTurboModuleManager.mm @@ -13,6 +13,7 @@ #import #import #import +#import #import #import @@ -48,7 +49,7 @@ static Class getFallbackClassFromName(const char *name) { - (instancetype)initWithBridge:(RCTBridge *)bridge delegate:(id)delegate { if (self = [super init]) { - _jsInvoker = std::make_shared(bridge.reactInstance); + _jsInvoker = std::make_shared(bridge.reactInstance); _delegate = delegate; _bridge = bridge;