From 6c989fe7c68164c6cd65a8dfbcc609e55b98a6f5 Mon Sep 17 00:00:00 2001 From: David Vacca Date: Fri, 1 Jun 2018 17:47:34 -0700 Subject: [PATCH] refactor JSI module initialization Reviewed By: fkgozali Differential Revision: D8111636 fbshipit-source-id: 6e32703b077144962519485002adff8c9f6084ad --- .../react/testing/ReactAppTestActivity.java | 55 +++++++++++-------- .../facebook/react/ReactInstanceManager.java | 12 ++-- .../react/ReactInstanceManagerBuilder.java | 12 ++-- .../com/facebook/react/ReactNativeHost.java | 7 +-- .../react/bridge/CatalystInstance.java | 2 +- .../react/bridge/CatalystInstanceImpl.java | 2 +- .../react/bridge/JSIModuleHolder.java | 23 ++++++-- .../react/bridge/JSIModulePackage.java | 15 +++++ .../react/bridge/JSIModuleProvider.java | 7 +++ .../react/bridge/JSIModuleRegistry.java | 27 +++------ .../facebook/react/bridge/JSIModuleSpec.java | 12 ++++ .../react/bridge/JSIModulesProvider.java | 15 ----- 12 files changed, 108 insertions(+), 81 deletions(-) create mode 100644 ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModulePackage.java create mode 100644 ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModuleProvider.java create mode 100644 ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModuleSpec.java delete mode 100644 ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModulesProvider.java diff --git a/ReactAndroid/src/androidTest/java/com/facebook/react/testing/ReactAppTestActivity.java b/ReactAndroid/src/androidTest/java/com/facebook/react/testing/ReactAppTestActivity.java index ddaae9d17..f01d74e1f 100644 --- a/ReactAndroid/src/androidTest/java/com/facebook/react/testing/ReactAppTestActivity.java +++ b/ReactAndroid/src/androidTest/java/com/facebook/react/testing/ReactAppTestActivity.java @@ -21,8 +21,9 @@ import com.facebook.react.ReactInstanceManager; import com.facebook.react.ReactInstanceManagerBuilder; import com.facebook.react.ReactRootView; import com.facebook.react.bridge.JSIModule; -import com.facebook.react.bridge.JSIModuleHolder; -import com.facebook.react.bridge.JSIModulesProvider; +import com.facebook.react.bridge.JSIModulePackage; +import com.facebook.react.bridge.JSIModuleProvider; +import com.facebook.react.bridge.JSIModuleSpec; import com.facebook.react.bridge.JavaScriptContextHolder; import com.facebook.react.bridge.ReactApplicationContext; import com.facebook.react.bridge.ReactContext; @@ -244,29 +245,35 @@ public class ReactAppTestActivity extends FragmentActivity .setUseDeveloperSupport(useDevSupport) .setBridgeIdleDebugListener(mBridgeIdleSignaler) .setInitialLifecycleState(mLifecycleState) - .setJSIModulesProvider( - new JSIModulesProvider() { - @Override - public List getJSIModules( - final ReactApplicationContext reactApplicationContext, - final JavaScriptContextHolder jsContext) { - return Arrays.asList(new JSIModuleHolder() { - @Override - public Class getJSIModuleClass() { - return UIManager.class; - } + .setJSIModulesPackage( + new JSIModulePackage() { + @Override + public List getJSIModules( + final ReactApplicationContext reactApplicationContext, + final JavaScriptContextHolder jsContext) { + return Arrays.asList( + new JSIModuleSpec() { + @Override + public Class getJSIModuleClass() { + return UIManager.class; + } - @Override - public FabricUIManager getJSIModule() { - List viewManagers = - mReactInstanceManager.getOrCreateViewManagers(reactApplicationContext); - FabricUIManager fabricUIManager = - new FabricUIManager(reactApplicationContext, new ViewManagerRegistry(viewManagers), jsContext); - new FabricJSCBinding().installFabric(jsContext, fabricUIManager); - return fabricUIManager; - } - }); - }}) + @Override + public JSIModuleProvider getJSIModuleProvider() { + return new JSIModuleProvider() { + @Override + public FabricUIManager get() { + List viewManagers = + mReactInstanceManager.getOrCreateViewManagers(reactApplicationContext); + FabricUIManager fabricUIManager = + new FabricUIManager(reactApplicationContext, new ViewManagerRegistry(viewManagers), jsContext); + new FabricJSCBinding().installFabric(jsContext, fabricUIManager); + return fabricUIManager; + } + }; + } + }); + }}) .setUIImplementationProvider(uiImplementationProvider); final CountDownLatch latch = new CountDownLatch(1); diff --git a/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java b/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java index 661f0d691..35c188eb7 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java @@ -47,7 +47,7 @@ import com.facebook.infer.annotation.ThreadSafe; import com.facebook.react.bridge.CatalystInstance; import com.facebook.react.bridge.CatalystInstanceImpl; import com.facebook.react.bridge.JSBundleLoader; -import com.facebook.react.bridge.JSIModulesProvider; +import com.facebook.react.bridge.JSIModulePackage; import com.facebook.react.bridge.JavaJSExecutor; import com.facebook.react.bridge.JavaScriptExecutor; import com.facebook.react.bridge.JavaScriptExecutorFactory; @@ -159,7 +159,7 @@ public class ReactInstanceManager { private final MemoryPressureRouter mMemoryPressureRouter; private final @Nullable NativeModuleCallExceptionHandler mNativeModuleCallExceptionHandler; private final boolean mLazyNativeModulesEnabled; - private final @Nullable JSIModulesProvider mJSIModulesProvider; + private final @Nullable JSIModulePackage mJSIModulePackage; private List mViewManagers; private class ReactContextInitParams { @@ -208,7 +208,7 @@ public class ReactInstanceManager { @Nullable DevBundleDownloadListener devBundleDownloadListener, int minNumShakes, int minTimeLeftInFrameForNonBatchedOperationMs, - @Nullable JSIModulesProvider jsiModulesProvider) { + @Nullable JSIModulePackage jsiModulePackage) { Log.d(ReactConstants.TAG, "ReactInstanceManager.ctor()"); initializeSoLoaderIfNecessary(applicationContext); @@ -256,7 +256,7 @@ public class ReactInstanceManager { } mPackages.addAll(packages); } - mJSIModulesProvider = jsiModulesProvider; + mJSIModulePackage = jsiModulePackage; // Instantiate ReactChoreographer in UI thread. ReactChoreographer.initialize(); @@ -1102,8 +1102,8 @@ public class ReactInstanceManager { Systrace.endSection(TRACE_TAG_REACT_JAVA_BRIDGE); ReactMarker.logMarker(CREATE_CATALYST_INSTANCE_END); } - if (mJSIModulesProvider != null) { - catalystInstance.addJSIModules(mJSIModulesProvider + if (mJSIModulePackage != null) { + catalystInstance.addJSIModules(mJSIModulePackage .getJSIModules(reactContext, catalystInstance.getJavaScriptContextHolder())); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerBuilder.java b/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerBuilder.java index 9c48a2338..d90f08f75 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerBuilder.java +++ b/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerBuilder.java @@ -10,7 +10,7 @@ import static com.facebook.react.modules.systeminfo.AndroidInfoHelpers.getFriend import android.app.Activity; import android.app.Application; import com.facebook.infer.annotation.Assertions; -import com.facebook.react.bridge.JSIModulesProvider; +import com.facebook.react.bridge.JSIModulePackage; import com.facebook.react.bridge.JSBundleLoader; import com.facebook.react.bridge.JSCJavaScriptExecutorFactory; import com.facebook.react.bridge.JavaScriptExecutorFactory; @@ -51,7 +51,7 @@ public class ReactInstanceManagerBuilder { private @Nullable JavaScriptExecutorFactory mJavaScriptExecutorFactory; private int mMinNumShakes = 1; private int mMinTimeLeftInFrameForNonBatchedOperationMs = -1; - private @Nullable JSIModulesProvider mJSIModulesProvider; + private @Nullable JSIModulePackage mJSIModulesPackage; /* package protected */ ReactInstanceManagerBuilder() { } @@ -66,9 +66,9 @@ public class ReactInstanceManagerBuilder { return this; } - public ReactInstanceManagerBuilder setJSIModulesProvider( - @Nullable JSIModulesProvider jsiModulesProvider) { - mJSIModulesProvider = jsiModulesProvider; + public ReactInstanceManagerBuilder setJSIModulesPackage( + @Nullable JSIModulePackage jsiModulePackage) { + mJSIModulesPackage = jsiModulePackage; return this; } @@ -284,6 +284,6 @@ public class ReactInstanceManagerBuilder { mDevBundleDownloadListener, mMinNumShakes, mMinTimeLeftInFrameForNonBatchedOperationMs, - mJSIModulesProvider); + mJSIModulesPackage); } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/ReactNativeHost.java b/ReactAndroid/src/main/java/com/facebook/react/ReactNativeHost.java index e1537d550..2efa28daa 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/ReactNativeHost.java +++ b/ReactAndroid/src/main/java/com/facebook/react/ReactNativeHost.java @@ -7,7 +7,7 @@ package com.facebook.react; -import com.facebook.react.bridge.JSIModulesProvider; +import com.facebook.react.bridge.JSIModulePackage; import javax.annotation.Nullable; import java.util.List; @@ -15,7 +15,6 @@ import java.util.List; import android.app.Application; import com.facebook.infer.annotation.Assertions; -import com.facebook.react.bridge.JSIModulesProvider; import com.facebook.react.bridge.JavaScriptExecutorFactory; import com.facebook.react.bridge.ReactMarker; import com.facebook.react.bridge.ReactMarkerConstants; @@ -76,7 +75,7 @@ public abstract class ReactNativeHost { .setRedBoxHandler(getRedBoxHandler()) .setJavaScriptExecutorFactory(getJavaScriptExecutorFactory()) .setUIImplementationProvider(getUIImplementationProvider()) - .setJSIModulesProvider(getJSIModulesProvider()) + .setJSIModulesPackage(getJSIModulePackage()) .setInitialLifecycleState(LifecycleState.BEFORE_CREATE); for (ReactPackage reactPackage : getPackages()) { @@ -124,7 +123,7 @@ public abstract class ReactNativeHost { } protected @Nullable - JSIModulesProvider getJSIModulesProvider() { + JSIModulePackage getJSIModulePackage() { return null; } 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 fe6d46112..c58e1afe0 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstance.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstance.java @@ -101,5 +101,5 @@ public interface CatalystInstance */ JavaScriptContextHolder getJavaScriptContextHolder(); - void addJSIModules(List jsiModules); + void addJSIModules(List jsiModules); } 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 b8930d1a4..b247b9b54 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java @@ -464,7 +464,7 @@ public class CatalystInstanceImpl implements CatalystInstance { } @Override - public void addJSIModules(List jsiModules) { + public void addJSIModules(List jsiModules) { mJSIModuleRegistry.registerModules(jsiModules); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModuleHolder.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModuleHolder.java index de98ae54b..8b35a5577 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModuleHolder.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModuleHolder.java @@ -1,12 +1,23 @@ package com.facebook.react.bridge; -/** - * Holder class used to register {@link JSIModule} into JSI Bridge. - */ -public interface JSIModuleHolder { +public class JSIModuleHolder { - Class getJSIModuleClass(); + private JSIModule mModule; + private final JSIModuleSpec mSpec; - T getJSIModule(); + public JSIModuleHolder(JSIModuleSpec spec) { + mSpec = spec; + } + public JSIModule getJSIModule() { + if (mModule == null) { + synchronized (this) { + if (mModule != null) { + return mModule; + } + mModule = mSpec.getJSIModuleProvider().get(); + } + } + return mModule; + } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModulePackage.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModulePackage.java new file mode 100644 index 000000000..9a85f68d6 --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModulePackage.java @@ -0,0 +1,15 @@ +package com.facebook.react.bridge; + +import java.util.List; + +/** + * Interface used to initialize JSI Modules into the JSI Bridge. + */ +public interface JSIModulePackage { + + /** + * @return a {@link List< JSIModuleSpec >} that contain the list of JSI Modules. + */ + List getJSIModules(ReactApplicationContext reactApplicationContext, JavaScriptContextHolder jsContext); + +} diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModuleProvider.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModuleProvider.java new file mode 100644 index 000000000..a94162f12 --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModuleProvider.java @@ -0,0 +1,7 @@ +package com.facebook.react.bridge; + +public interface JSIModuleProvider { + + T get(); + +} diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModuleRegistry.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModuleRegistry.java index 7d4e1141b..cb4eb7478 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModuleRegistry.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModuleRegistry.java @@ -1,37 +1,28 @@ package com.facebook.react.bridge; import com.facebook.infer.annotation.Assertions; -import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; public class JSIModuleRegistry { - private final List mModuleList = new ArrayList<>(); - private final Map mModules = new HashMap<>(); - private volatile boolean mIsInitialized = false; + private final Map mModules = new HashMap<>(); public JSIModuleRegistry() { } public T getModule(Class moduleClass) { - JSIModule jsiModule = mModules.get(moduleClass); - if (jsiModule == null) { - initModules(); + JSIModuleHolder jsiModuleHolder = mModules.get(moduleClass); + if (jsiModuleHolder == null) { + throw new IllegalArgumentException("Unable to find JSIModule for class " + moduleClass); } - return (T) Assertions.assertNotNull(mModules.get(moduleClass)); + return (T) Assertions.assertNotNull(jsiModuleHolder.getJSIModule()); } - public void registerModules(List jsiModules) { - mModuleList.addAll(jsiModules); - } - - public synchronized void initModules() { - if (!mIsInitialized) { - for (JSIModuleHolder holder : mModuleList) { - mModules.put(holder.getJSIModuleClass(), holder.getJSIModule()); - } - mIsInitialized = true; + public void registerModules(List jsiModules) { + for (JSIModuleSpec spec : jsiModules) { + mModules.put(spec.getJSIModuleClass(), new JSIModuleHolder(spec)); } } + } diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModuleSpec.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModuleSpec.java new file mode 100644 index 000000000..d8c212200 --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModuleSpec.java @@ -0,0 +1,12 @@ +package com.facebook.react.bridge; + +/** + * Holder class used to register {@link JSIModule} into JSI Bridge. + */ +public interface JSIModuleSpec { + + Class getJSIModuleClass(); + + JSIModuleProvider getJSIModuleProvider(); + +} diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModulesProvider.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModulesProvider.java deleted file mode 100644 index 8abaa1ff7..000000000 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/JSIModulesProvider.java +++ /dev/null @@ -1,15 +0,0 @@ -package com.facebook.react.bridge; - -import java.util.List; - -/** - * Interface used to initialize JSI Modules into the JSI Bridge. - */ -public interface JSIModulesProvider { - - /** - * @return a {@link List} that contain the list of JSI Modules. - */ - List getJSIModules(ReactApplicationContext reactApplicationContext, JavaScriptContextHolder jsContext); - -}