From d9deee20e7bdc0766f5692dce396f94c037bbf32 Mon Sep 17 00:00:00 2001
From: Ramanpreet Nara <ramanpreet@fb.com>
Date: Thu, 5 Dec 2019 18:57:16 -0800
Subject: [PATCH] Refactor TurboModule filtering in NativeModuleRegistryBuilder

Summary:
## Context
`NativeModuleRegistryBuilder` calls `TurboReactPackage.getNativeModuleIterator()` to access ModuleHolders for all the NativeModules in the `TurboReactPackage`. We then filter out the ModuleHolders that contain `TurboModules`, before using that list to make create the `NativeModuleRegistry`.

## Problem
Creating `ModuleHolders` has the side-effect of actually creating the NativeModule if it requires eager initialization. See [ModuleHolder.java](https://fburl.com/diffusion/4avdtio0):

```
class ModuleHolder {
  // ...
  public ModuleHolder(ReactModuleInfo moduleInfo, Provider<? extends NativeModule> provider) {
    mName = moduleInfo.name();
    mProvider = provider;
    mReactModuleInfo = moduleInfo;
    if (moduleInfo.needsEagerInit()) {
      mModule = create(); // HERE!
    }
  }

```

So, we need to filter out TurboModules before we even create ModuleHolders.

Changelog:
[Android][Fixed] - Refactor TurboModule filtering in NativeModuleRegistryBuilder

Reviewed By: PeteTheHeat, mdvacca

Differential Revision: D18814010

fbshipit-source-id: a120d2b619b9280ba70e21d131dccc5a9fc6346d
---
 .../react/NativeModuleRegistryBuilder.java    | 13 ------
 .../com/facebook/react/TurboReactPackage.java | 40 ++++++++++++++++++-
 2 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/ReactAndroid/src/main/java/com/facebook/react/NativeModuleRegistryBuilder.java b/ReactAndroid/src/main/java/com/facebook/react/NativeModuleRegistryBuilder.java
index c85ed9c7e2710b..894230a9e54267 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/NativeModuleRegistryBuilder.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/NativeModuleRegistryBuilder.java
@@ -10,7 +10,6 @@
 import com.facebook.react.bridge.ModuleHolder;
 import com.facebook.react.bridge.NativeModuleRegistry;
 import com.facebook.react.bridge.ReactApplicationContext;
-import com.facebook.react.config.ReactFeatureFlags;
 import java.util.HashMap;
 import java.util.Map;
 
@@ -59,18 +58,6 @@ public void processPackage(ReactPackage reactPackage) {
         }
         mModules.remove(existingNativeModule);
       }
-      if (ReactFeatureFlags.useTurboModules && moduleHolder.isTurboModule()) {
-        // If this module is a TurboModule, and if TurboModules are enabled, don't add this module
-
-        // This condition is after checking for overrides, since if there is already a module,
-        // and we want to override it with a turbo module, we would need to remove the modules thats
-        // already in the list, and then NOT add the new module, since that will be directly exposed
-
-        // Note that is someone uses {@link NativeModuleRegistry#registerModules}, we will NOT check
-        // for TurboModules - assuming that people wanted to explicitly register native modules
-        // there
-        continue;
-      }
       mModules.put(name, moduleHolder);
     }
   }
diff --git a/ReactAndroid/src/main/java/com/facebook/react/TurboReactPackage.java b/ReactAndroid/src/main/java/com/facebook/react/TurboReactPackage.java
index 295396cee99ca0..de2b6347ec3456 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/TurboReactPackage.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/TurboReactPackage.java
@@ -12,6 +12,7 @@
 import com.facebook.react.bridge.ModuleSpec;
 import com.facebook.react.bridge.NativeModule;
 import com.facebook.react.bridge.ReactApplicationContext;
+import com.facebook.react.config.ReactFeatureFlags;
 import com.facebook.react.module.model.ReactModuleInfo;
 import com.facebook.react.module.model.ReactModuleInfoProvider;
 import com.facebook.react.uimanager.ViewManager;
@@ -20,6 +21,7 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.NoSuchElementException;
 import java.util.Set;
 import javax.inject.Provider;
 
@@ -62,14 +64,48 @@ public Iterable<ModuleHolder> getNativeModuleIterator(
       // This should ideally be an IteratorConvertor, but we don't have any internal library for it
       public Iterator<ModuleHolder> iterator() {
         return new Iterator<ModuleHolder>() {
+          Map.Entry<String, ReactModuleInfo> nextEntry = null;
+
+          private void findNext() {
+            while (entrySetIterator.hasNext()) {
+              Map.Entry<String, ReactModuleInfo> entry = entrySetIterator.next();
+              ReactModuleInfo reactModuleInfo = entry.getValue();
+
+              // This Iterator is used to create the NativeModule registry. The NativeModule
+              // registry must not have TurboModules. Therefore, if TurboModules are enabled, and
+              // the current NativeModule is a TurboModule, we need to skip iterating over it.
+              if (ReactFeatureFlags.useTurboModules && reactModuleInfo.isTurboModule()) {
+                continue;
+              }
+
+              nextEntry = entry;
+              return;
+            }
+            nextEntry = null;
+          }
+
           @Override
           public boolean hasNext() {
-            return entrySetIterator.hasNext();
+            if (nextEntry == null) {
+              findNext();
+            }
+            return nextEntry != null;
           }
 
           @Override
           public ModuleHolder next() {
-            Map.Entry<String, ReactModuleInfo> entry = entrySetIterator.next();
+            if (nextEntry == null) {
+              findNext();
+            }
+
+            if (nextEntry == null) {
+              throw new NoSuchElementException("ModuleHolder not found");
+            }
+
+            Map.Entry<String, ReactModuleInfo> entry = nextEntry;
+
+            // Advance iterator
+            findNext();
             String name = entry.getKey();
             ReactModuleInfo reactModuleInfo = entry.getValue();
             return new ModuleHolder(reactModuleInfo, new ModuleHolderProvider(name, reactContext));