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

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<ViewManager> createViewManagers(ReactApplicationContext reactContext) {
+        	   return Collections.emptyList();
+        	}
+
+        	@Override
+        	public List<NativeModule> createNativeModules(ReactApplicationContext reactContext) {
+        		return Arrays.asList(nativeModule);
+        	}
+        });
         return this;
       }
 

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
+ * <p>This source code is licensed under the MIT license found in the LICENSE file in the root
- * LICENSE file in the root directory of this source tree.
+ * 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
+ * <p>TODO(t11394819): Make this default and deprecate ReactPackage
  */
 public abstract class LazyReactPackage implements ReactPackage {
 
@@ -34,7 +36,8 @@ public abstract class LazyReactPackage implements ReactPackage {
       LazyReactPackage lazyReactPackage) {
     Class<?> reactModuleInfoProviderClass;
     try {
-      reactModuleInfoProviderClass = Class.forName(
+      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
@@ -50,30 +53,96 @@ public abstract class LazyReactPackage implements ReactPackage {
     }
 
     if (reactModuleInfoProviderClass == null) {
-      throw new RuntimeException("ReactModuleInfoProvider class for " +
+      throw new RuntimeException(
-          lazyReactPackage.getClass().getCanonicalName() + " not found.");
+          "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(),
+          "Unable to instantiate ReactModuleInfoProvider for " + lazyReactPackage.getClass(), e);
-          e);
     } catch (IllegalAccessException e) {
       throw new RuntimeException(
-          "Unable to instantiate ReactModuleInfoProvider for " + lazyReactPackage.getClass(),
+          "Unable to instantiate ReactModuleInfoProvider for " + lazyReactPackage.getClass(), e);
-          e);
     }
   }
 
+  /**
+   * We return an iterable
+   *
+   * @param reactContext
+   * @return
+   */
+  /* package */
+  Iterable<ModuleHolder> getNativeModuleIterator(final ReactApplicationContext reactContext) {
+    final List<String> eagerNativeModules = getEagerNativeModules();
+    final Map<String, ReactModuleInfo> reactModuleInfoMap =
+        getReactModuleInfoProvider().getReactModuleInfos();
+    final List<ModuleSpec> nativeModules = getNativeModules(reactContext);
+
+    return new Iterable<ModuleHolder>() {
+      @NonNull
+      @Override
+      public Iterator<ModuleHolder> iterator() {
+        return new Iterator<ModuleHolder>() {
+          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<ModuleSpec> getNativeModules(
+  protected abstract List<ModuleSpec> getNativeModules(ReactApplicationContext reactContext);
-    ReactApplicationContext reactContext);
 
+  /** @return list of native modules which should be eagerly initialized. */
+  protected List<String> 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<NativeModule> createNativeModules(ReactApplicationContext reactContext) {
     List<NativeModule> modules = new ArrayList<>();
@@ -82,9 +151,7 @@ public abstract class LazyReactPackage implements ReactPackage {
       SystraceMessage.beginSection(TRACE_TAG_REACT_JAVA_BRIDGE, "createNativeModule")
           .arg("module", holder.getType())
           .flush();
-      ReactMarker.logMarker(
+      ReactMarker.logMarker(ReactMarkerConstants.CREATE_MODULE_START, holder.getName());
-        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<String> 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.

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,69 +20,26 @@ public class NativeModuleRegistryBuilder {
   private final Map<String, ModuleHolder> mModules = new HashMap<>();
 
   public NativeModuleRegistryBuilder(
-    ReactApplicationContext reactApplicationContext,
+      ReactApplicationContext reactApplicationContext, ReactInstanceManager reactInstanceManager) {
-    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<ModuleHolder> moduleHolders;
     if (reactPackage instanceof LazyReactPackage) {
-      LazyReactPackage lazyReactPackage = (LazyReactPackage) reactPackage;
+      moduleHolders =
-      List<ModuleSpec> moduleSpecs = lazyReactPackage.getNativeModules(mReactApplicationContext);
+          ((LazyReactPackage) reactPackage).getNativeModuleIterator(mReactApplicationContext);
-      List<String> eagerNativeModules = lazyReactPackage.getEagerNativeModules();
-      Map<String, ReactModuleInfo> 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());
+      moduleHolders =
+          ReactPackageHelper.getNativeModuleIterator(
+              reactPackage, mReactApplicationContext, mReactInstanceManager);
     }
 
-        putModuleTypeAndHolderToModuleMaps(name, moduleHolder);
+    for (ModuleHolder moduleHolder : moduleHolders) {
-      }
+      String name = moduleHolder.getName();
-    } else {
-      FLog.d(
-          ReactConstants.TAG,
-          reactPackage.getClass().getSimpleName()
-              + " is not a LazyReactPackage, falling back to old version.");
-      List<NativeModule> 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()) {
@@ -101,20 +48,15 @@ public class NativeModuleRegistryBuilder {
                   + name
                   + " tried to override "
                   + existingNativeModule.getClassName()
-                + " for module name "
+                  + " 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");
-                + ". 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(
+    return new NativeModuleRegistry(mReactApplicationContext, mModules);
-        mReactApplicationContext, mModules);
   }
 }

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

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<ModuleHolder> 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<NativeModule> nativeModules;
+    if (reactPackage instanceof ReactInstancePackage) {
+      ReactInstancePackage reactInstancePackage = (ReactInstancePackage) reactPackage;
+      nativeModules =
+          reactInstancePackage.createNativeModules(reactApplicationContext, reactInstanceManager);
+    } else {
+      nativeModules = reactPackage.createNativeModules(reactApplicationContext);
+    }
+    return new Iterable<ModuleHolder>() {
+      @NonNull
+      @Override
+      public Iterator<ModuleHolder> iterator() {
+        return new Iterator<ModuleHolder>() {
+          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 ");
+          }
+        };
+      }
+    };
+  }
+}