From 875f70b8b691cd1fb3e802594bbd4461b9959751 Mon Sep 17 00:00:00 2001 From: Ram N Date: Wed, 5 Sep 2018 22:53:03 -0700 Subject: [PATCH] Refactored NativeModuleRegistryBuilder Summary: The logic in NativeModuleRegistryBuilder.processPackage was getting pretty complex with lot of branches. Moved the logic to get actual packages into each of the packages, instead of having it in NativeModuleRegistryBuilder. Reviewed By: achen1 Differential Revision: D9618140 fbshipit-source-id: 4be82ec65b0bd92f11f8b77044004e10c9d3b1a1 --- .../react/testing/ReactTestHelper.java | 21 +++- .../com/facebook/react/LazyReactPackage.java | 114 +++++++++++++----- .../react/NativeModuleRegistryBuilder.java | 112 +++++------------ .../java/com/facebook/react/ReactPackage.java | 4 +- .../facebook/react/ReactPackageHelper.java | 62 ++++++++++ 5 files changed, 194 insertions(+), 119 deletions(-) create mode 100644 ReactAndroid/src/main/java/com/facebook/react/ReactPackageHelper.java diff --git a/ReactAndroid/src/androidTest/java/com/facebook/react/testing/ReactTestHelper.java b/ReactAndroid/src/androidTest/java/com/facebook/react/testing/ReactTestHelper.java index f3f1ffc29..686be35a9 100644 --- a/ReactAndroid/src/androidTest/java/com/facebook/react/testing/ReactTestHelper.java +++ b/ReactAndroid/src/androidTest/java/com/facebook/react/testing/ReactTestHelper.java @@ -7,6 +7,10 @@ package com.facebook.react.testing; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + import javax.annotation.Nullable; import android.app.Instrumentation; @@ -20,6 +24,7 @@ import com.facebook.react.NativeModuleRegistryBuilder; import com.facebook.react.R; import com.facebook.react.ReactInstanceManager; import com.facebook.react.ReactInstanceManagerBuilder; +import com.facebook.react.ReactPackage; import com.facebook.react.bridge.CatalystInstance; import com.facebook.react.bridge.JavaScriptModuleRegistry; import com.facebook.react.bridge.NativeModule; @@ -32,7 +37,7 @@ import com.facebook.react.bridge.JSBundleLoader; import com.facebook.react.bridge.JSCJavaScriptExecutorFactory; import com.facebook.react.bridge.JavaScriptExecutor; import com.facebook.react.modules.core.ReactChoreographer; - +import com.facebook.react.uimanager.ViewManager; import com.android.internal.util.Predicate; public class ReactTestHelper { @@ -50,14 +55,24 @@ public class ReactTestHelper { } @Override - public ReactInstanceEasyBuilder addNativeModule(NativeModule nativeModule) { + public ReactInstanceEasyBuilder addNativeModule(final NativeModule nativeModule) { if (mNativeModuleRegistryBuilder == null) { mNativeModuleRegistryBuilder = new NativeModuleRegistryBuilder( (ReactApplicationContext) mContext, null); } Assertions.assertNotNull(nativeModule); - mNativeModuleRegistryBuilder.addNativeModule(nativeModule); + mNativeModuleRegistryBuilder.processPackage(new ReactPackage(){ + @Override + public List createViewManagers(ReactApplicationContext reactContext) { + return Collections.emptyList(); + } + + @Override + public List createNativeModules(ReactApplicationContext reactContext) { + return Arrays.asList(nativeModule); + } + }); return this; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/LazyReactPackage.java b/ReactAndroid/src/main/java/com/facebook/react/LazyReactPackage.java index 1b02a82ca..b9e1a0455 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/LazyReactPackage.java +++ b/ReactAndroid/src/main/java/com/facebook/react/LazyReactPackage.java @@ -1,14 +1,15 @@ /** * Copyright (c) 2015-present, Facebook, Inc. * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. + *

This source code is licensed under the MIT license found in the LICENSE file in the root + * directory of this source tree. */ - package com.facebook.react; import static com.facebook.systrace.Systrace.TRACE_TAG_REACT_JAVA_BRIDGE; +import android.support.annotation.NonNull; +import com.facebook.react.bridge.ModuleHolder; import com.facebook.react.bridge.ModuleSpec; import com.facebook.react.bridge.NativeModule; import com.facebook.react.bridge.ReactApplicationContext; @@ -20,13 +21,14 @@ import com.facebook.react.uimanager.ViewManager; import com.facebook.systrace.SystraceMessage; import java.util.ArrayList; import java.util.Collections; +import java.util.Iterator; import java.util.List; import java.util.Map; /** * React package supporting lazy creation of native modules. * - * TODO(t11394819): Make this default and deprecate ReactPackage + *

TODO(t11394819): Make this default and deprecate ReactPackage */ public abstract class LazyReactPackage implements ReactPackage { @@ -34,8 +36,9 @@ public abstract class LazyReactPackage implements ReactPackage { LazyReactPackage lazyReactPackage) { Class reactModuleInfoProviderClass; try { - reactModuleInfoProviderClass = Class.forName( - lazyReactPackage.getClass().getCanonicalName() + "$$ReactModuleInfoProvider"); + reactModuleInfoProviderClass = + Class.forName( + lazyReactPackage.getClass().getCanonicalName() + "$$ReactModuleInfoProvider"); } catch (ClassNotFoundException e) { // In OSS case, when the annotation processor does not run, we fall back to non-lazy mode // For this, we simply return an empty moduleMap. @@ -50,41 +53,105 @@ public abstract class LazyReactPackage implements ReactPackage { } if (reactModuleInfoProviderClass == null) { - throw new RuntimeException("ReactModuleInfoProvider class for " + - lazyReactPackage.getClass().getCanonicalName() + " not found."); + throw new RuntimeException( + "ReactModuleInfoProvider class for " + + lazyReactPackage.getClass().getCanonicalName() + + " not found."); } try { return (ReactModuleInfoProvider) reactModuleInfoProviderClass.newInstance(); } catch (InstantiationException e) { throw new RuntimeException( - "Unable to instantiate ReactModuleInfoProvider for " + lazyReactPackage.getClass(), - e); + "Unable to instantiate ReactModuleInfoProvider for " + lazyReactPackage.getClass(), e); } catch (IllegalAccessException e) { throw new RuntimeException( - "Unable to instantiate ReactModuleInfoProvider for " + lazyReactPackage.getClass(), - e); + "Unable to instantiate ReactModuleInfoProvider for " + lazyReactPackage.getClass(), e); } } + /** + * We return an iterable + * + * @param reactContext + * @return + */ + /* package */ + Iterable getNativeModuleIterator(final ReactApplicationContext reactContext) { + final List eagerNativeModules = getEagerNativeModules(); + final Map reactModuleInfoMap = + getReactModuleInfoProvider().getReactModuleInfos(); + final List nativeModules = getNativeModules(reactContext); + + return new Iterable() { + @NonNull + @Override + public Iterator iterator() { + return new Iterator() { + int position = 0; + + @Override + public ModuleHolder next() { + ModuleSpec moduleSpec = nativeModules.get(position++); + String name = moduleSpec.getName(); + ReactModuleInfo reactModuleInfo = reactModuleInfoMap.get(name); + ModuleHolder moduleHolder; + if (reactModuleInfo == null || eagerNativeModules.contains(name)) { + NativeModule module; + ReactMarker.logMarker(ReactMarkerConstants.CREATE_MODULE_START, name); + try { + module = moduleSpec.getProvider().get(); + } finally { + ReactMarker.logMarker(ReactMarkerConstants.CREATE_MODULE_END); + } + moduleHolder = new ModuleHolder(module); + } else { + moduleHolder = new ModuleHolder(reactModuleInfo, moduleSpec.getProvider()); + } + return moduleHolder; + } + + @Override + public boolean hasNext() { + return position < nativeModules.size(); + } + + @Override + public void remove() { + throw new UnsupportedOperationException("Cannot remove native modules from the list"); + } + }; + } + }; + } + /** * @param reactContext react application context that can be used to create modules * @return list of module specs that can create the native modules */ - public abstract List getNativeModules( - ReactApplicationContext reactContext); + protected abstract List getNativeModules(ReactApplicationContext reactContext); + /** @return list of native modules which should be eagerly initialized. */ + protected List getEagerNativeModules() { + return Collections.emptyList(); + } + + /** + * This is only used when a LazyReactPackage is a part of {@link CompositeReactPackage} Once we + * deprecate {@link CompositeReactPackage}, this can be removed too + * + * @param reactContext react application context that can be used to create modules + * @return + */ @Override public final List createNativeModules(ReactApplicationContext reactContext) { List modules = new ArrayList<>(); for (ModuleSpec holder : getNativeModules(reactContext)) { NativeModule nativeModule; SystraceMessage.beginSection(TRACE_TAG_REACT_JAVA_BRIDGE, "createNativeModule") - .arg("module", holder.getType()) - .flush(); - ReactMarker.logMarker( - ReactMarkerConstants.CREATE_MODULE_START, - holder.getName()); + .arg("module", holder.getType()) + .flush(); + ReactMarker.logMarker(ReactMarkerConstants.CREATE_MODULE_START, holder.getName()); try { nativeModule = holder.getProvider().get(); } finally { @@ -95,15 +162,6 @@ public abstract class LazyReactPackage implements ReactPackage { } return modules; } - - /** - * @return list of native modules which should be eagerly initialized. - */ - public List getEagerNativeModules() { - return Collections.emptyList(); - } - - /** * @param reactContext react application context that can be used to create View Managers. * @return list of module specs that can create the View Managers. diff --git a/ReactAndroid/src/main/java/com/facebook/react/NativeModuleRegistryBuilder.java b/ReactAndroid/src/main/java/com/facebook/react/NativeModuleRegistryBuilder.java index 686b4119a..e1e16ce5c 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/NativeModuleRegistryBuilder.java +++ b/ReactAndroid/src/main/java/com/facebook/react/NativeModuleRegistryBuilder.java @@ -5,23 +5,13 @@ package com.facebook.react; -import com.facebook.common.logging.FLog; import com.facebook.react.bridge.ModuleHolder; -import com.facebook.react.bridge.ModuleSpec; -import com.facebook.react.bridge.NativeModule; import com.facebook.react.bridge.NativeModuleRegistry; import com.facebook.react.bridge.ReactApplicationContext; -import com.facebook.react.bridge.ReactMarker; -import com.facebook.react.bridge.ReactMarkerConstants; -import com.facebook.react.common.ReactConstants; -import com.facebook.react.module.model.ReactModuleInfo; import java.util.HashMap; -import java.util.List; import java.util.Map; -/** - * Helper class to build NativeModuleRegistry. - */ +/** Helper class to build NativeModuleRegistry. */ public class NativeModuleRegistryBuilder { private final ReactApplicationContext mReactApplicationContext; @@ -30,91 +20,43 @@ public class NativeModuleRegistryBuilder { private final Map mModules = new HashMap<>(); public NativeModuleRegistryBuilder( - ReactApplicationContext reactApplicationContext, - ReactInstanceManager reactInstanceManager) { + ReactApplicationContext reactApplicationContext, ReactInstanceManager reactInstanceManager) { mReactApplicationContext = reactApplicationContext; mReactInstanceManager = reactInstanceManager; } public void processPackage(ReactPackage reactPackage) { + // We use an iterable instead of an iterator here to ensure thread safety, and that this list + // cannot be modified + Iterable moduleHolders; if (reactPackage instanceof LazyReactPackage) { - LazyReactPackage lazyReactPackage = (LazyReactPackage) reactPackage; - List moduleSpecs = lazyReactPackage.getNativeModules(mReactApplicationContext); - List eagerNativeModules = lazyReactPackage.getEagerNativeModules(); - Map reactModuleInfoMap = - lazyReactPackage.getReactModuleInfoProvider().getReactModuleInfos(); - - for (ModuleSpec moduleSpec : moduleSpecs) { - String name = moduleSpec.getName(); - ReactModuleInfo reactModuleInfo = reactModuleInfoMap.get(name); - ModuleHolder moduleHolder; - if (reactModuleInfo == null || eagerNativeModules.contains(name)) { - NativeModule module; - ReactMarker.logMarker( - ReactMarkerConstants.CREATE_MODULE_START, - name); - try { - module = moduleSpec.getProvider().get(); - } finally { - ReactMarker.logMarker(ReactMarkerConstants.CREATE_MODULE_END); - } - moduleHolder = new ModuleHolder(module); - } else { - moduleHolder = new ModuleHolder(reactModuleInfo, moduleSpec.getProvider()); - } - - putModuleTypeAndHolderToModuleMaps(name, moduleHolder); - } + moduleHolders = + ((LazyReactPackage) reactPackage).getNativeModuleIterator(mReactApplicationContext); } else { - FLog.d( - ReactConstants.TAG, - reactPackage.getClass().getSimpleName() - + " is not a LazyReactPackage, falling back to old version."); - List nativeModules; - if (reactPackage instanceof ReactInstancePackage) { - ReactInstancePackage reactInstancePackage = (ReactInstancePackage) reactPackage; - nativeModules = - reactInstancePackage.createNativeModules( - mReactApplicationContext, mReactInstanceManager); - } else { - nativeModules = reactPackage.createNativeModules(mReactApplicationContext); - } - for (NativeModule nativeModule : nativeModules) { - addNativeModule(nativeModule); - } - } - } - - public void addNativeModule(NativeModule nativeModule) { - String name = nativeModule.getName(); - putModuleTypeAndHolderToModuleMaps(name, new ModuleHolder(nativeModule)); - } - - private void putModuleTypeAndHolderToModuleMaps( - String name, ModuleHolder moduleHolder) - throws IllegalStateException { - if (mModules.containsKey(name)) { - ModuleHolder existingNativeModule = mModules.get(name); - if (!moduleHolder.getCanOverrideExistingModule()) { - throw new IllegalStateException( - "Native module " - + name - + " tried to override " - + existingNativeModule.getClassName() - + " for module name " - + ". Check the getPackages() method in MainApplication.java, it might be " - + "that module is being created twice. " - + "If this was your intention, set canOverrideExistingModule=true"); - } - - mModules.remove(existingNativeModule); + moduleHolders = + ReactPackageHelper.getNativeModuleIterator( + reactPackage, mReactApplicationContext, mReactInstanceManager); } - mModules.put(name, moduleHolder); + for (ModuleHolder moduleHolder : moduleHolders) { + String name = moduleHolder.getName(); + if (mModules.containsKey(name)) { + ModuleHolder existingNativeModule = mModules.get(name); + if (!moduleHolder.getCanOverrideExistingModule()) { + throw new IllegalStateException( + "Native module " + + name + + " tried to override " + + existingNativeModule.getClassName() + + " for module name .Check the getPackages() method in MainApplication.java, it might be that module is being created twice. If this was your intention, set canOverrideExistingModule=true"); + } + mModules.remove(existingNativeModule); + } + mModules.put(name, moduleHolder); + } } public NativeModuleRegistry build() { - return new NativeModuleRegistry( - mReactApplicationContext, mModules); + return new NativeModuleRegistry(mReactApplicationContext, mModules); } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/ReactPackage.java b/ReactAndroid/src/main/java/com/facebook/react/ReactPackage.java index 51828a144..84541c81a 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/ReactPackage.java +++ b/ReactAndroid/src/main/java/com/facebook/react/ReactPackage.java @@ -7,13 +7,11 @@ package com.facebook.react; -import java.util.List; - import com.facebook.react.bridge.NativeModule; import com.facebook.react.bridge.ReactApplicationContext; -import com.facebook.react.bridge.JavaScriptModule; import com.facebook.react.uimanager.UIManagerModule; import com.facebook.react.uimanager.ViewManager; +import java.util.List; /** * Main interface for providing additional capabilities to the catalyst framework by couple of diff --git a/ReactAndroid/src/main/java/com/facebook/react/ReactPackageHelper.java b/ReactAndroid/src/main/java/com/facebook/react/ReactPackageHelper.java new file mode 100644 index 000000000..83630844b --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/ReactPackageHelper.java @@ -0,0 +1,62 @@ +package com.facebook.react; + +import android.support.annotation.NonNull; +import com.facebook.common.logging.FLog; +import com.facebook.react.bridge.ModuleHolder; +import com.facebook.react.bridge.NativeModule; +import com.facebook.react.bridge.ReactApplicationContext; +import com.facebook.react.common.ReactConstants; +import java.util.Iterator; +import java.util.List; + +public class ReactPackageHelper { + /** + * A helper method to iterate over a list of Native Modules and convert them to an iterable. + * + * @param reactPackage + * @param reactApplicationContext + * @param reactInstanceManager + * @return + */ + public static Iterable getNativeModuleIterator( + ReactPackage reactPackage, + ReactApplicationContext reactApplicationContext, + ReactInstanceManager reactInstanceManager) { + FLog.d( + ReactConstants.TAG, + reactPackage.getClass().getSimpleName() + + " is not a LazyReactPackage, falling back to old version."); + final List nativeModules; + if (reactPackage instanceof ReactInstancePackage) { + ReactInstancePackage reactInstancePackage = (ReactInstancePackage) reactPackage; + nativeModules = + reactInstancePackage.createNativeModules(reactApplicationContext, reactInstanceManager); + } else { + nativeModules = reactPackage.createNativeModules(reactApplicationContext); + } + return new Iterable() { + @NonNull + @Override + public Iterator iterator() { + return new Iterator() { + int position = 0; + + @Override + public ModuleHolder next() { + return new ModuleHolder(nativeModules.get(position++)); + } + + @Override + public boolean hasNext() { + return position < nativeModules.size(); + } + + @Override + public void remove() { + throw new UnsupportedOperationException("Cannot remove methods "); + } + }; + } + }; + } +}