From e3d3533bc5da16ea9cefdb1e1404c1f7d56b1e65 Mon Sep 17 00:00:00 2001
From: Vojtech Novak <vonovak@gmail.com>
Date: Thu, 14 Jun 2018 08:19:37 -0700
Subject: [PATCH] improve NativeModuleRegistryBuilder.java (#16402)

Summary:
I met the error `Native module xyz tried to override xyz for module name xyzModuleName. If this was your intention...` after something went wrong during `react-native link` - one module somehow ended up being included twice in the `getPackages` method, as in:

```java
Override
        protected List<ReactPackage> getPackages() {
            return Arrays.<ReactPackage>asList(
                    new MainReactPackage(),
            new WowPackage(),
            new WowPackage(),
```

Since I have > 20 native modules it took me a little while to find out what the problem was. The improved error message should make the problem clearer to anybody who may encounter it. I did try to refactor the code a little more, by extracting the whole part of:

```java
        String name = moduleHolder.getName();
        if (namesToType.containsKey(name)) {
          Class<? extends NativeModule> existingNativeModule = namesToType.get(name);
          if (!moduleHolder.getCanOverrideExistingModule()) {
            throw new IllegalStateException(getModuleOverridingExceptionMessage(
              type.getSimpleName(),
              existingNativeModule.getSimpleName(),
              name
            ));
          }

          mModules.remove(existingNativeModule);
        }

        namesToType.put(name, type);
        mModules.put(type, moduleHolder);
```

out into a separate method since there were two places where nearly identical code was written.

I have built RN from source and used it in a very simple app with RN vector icons (the package creates a native module as can be seen [here](https://github.com/oblador/react-native-vector-icons/blob/master/android/src/main/java/com/oblador/vectoricons/VectorIconsPackage.java#L19)).

After including the module twice, ie.

```java
        Override
        protected List<ReactPackage> getPackages() {
            return Arrays.<ReactPackage>asList(
                    new MainReactPackage(),
                    new VectorIconsPackage(),
                    new VectorIconsPackage()
            );
        }
```

I get the improved error description, as seen in the screenshot.

<img src="https://user-images.githubusercontent.com/1566403/36340960-3289d9d0-13e7-11e8-8d17-e1651da17841.png" height="500">

[ANDROID] [MINOR] [NativeModuleRegistryBuilder] - Improve error message and refactor putting native modules to module maps
Closes https://github.com/facebook/react-native/pull/16402

Differential Revision: D8421392

Pulled By: hramos

fbshipit-source-id: 719bd37b4681933d35858621b402ae73dd460a5b
---
 .../react/NativeModuleRegistryBuilder.java    | 36 ++++++++-----------
 1 file changed, 15 insertions(+), 21 deletions(-)

diff --git a/ReactAndroid/src/main/java/com/facebook/react/NativeModuleRegistryBuilder.java b/ReactAndroid/src/main/java/com/facebook/react/NativeModuleRegistryBuilder.java
index fe0a2624a..85097e7a6 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/NativeModuleRegistryBuilder.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/NativeModuleRegistryBuilder.java
@@ -80,19 +80,7 @@ public class NativeModuleRegistryBuilder {
         }
 
         String name = moduleHolder.getName();
-        if (namesToType.containsKey(name)) {
-          Class<? extends NativeModule> existingNativeModule = namesToType.get(name);
-          if (!moduleHolder.getCanOverrideExistingModule()) {
-            throw new IllegalStateException("Native module " + type.getSimpleName() +
-              " tried to override " + existingNativeModule.getSimpleName() + " for module name " +
-              name + ". If this was your intention, set canOverrideExistingModule=true");
-          }
-
-          mModules.remove(existingNativeModule);
-        }
-
-        namesToType.put(name, type);
-        mModules.put(type, moduleHolder);
+        putModuleTypeAndHolderToModuleMaps(type, name, moduleHolder);
       }
     } else {
       FLog.d(
@@ -117,19 +105,25 @@ public class NativeModuleRegistryBuilder {
   public void addNativeModule(NativeModule nativeModule) {
     String name = nativeModule.getName();
     Class<? extends NativeModule> type = nativeModule.getClass();
-    if (namesToType.containsKey(name)) {
-      Class<? extends NativeModule> existingModule = namesToType.get(name);
-      if (!nativeModule.canOverrideExistingModule()) {
+    putModuleTypeAndHolderToModuleMaps(type, name, new ModuleHolder(nativeModule));
+  }
+
+  private void putModuleTypeAndHolderToModuleMaps(Class<? extends NativeModule> type, String underName,
+                                                  ModuleHolder moduleHolder) throws IllegalStateException {
+    if (namesToType.containsKey(underName)) {
+      Class<? extends NativeModule> existingNativeModule = namesToType.get(underName);
+      if (!moduleHolder.getCanOverrideExistingModule()) {
         throw new IllegalStateException("Native module " + type.getSimpleName() +
-          " tried to override " + existingModule.getSimpleName() + " for module name " +
-          name + ". If this was your intention, set canOverrideExistingModule=true");
+          " tried to override " + existingNativeModule.getSimpleName() + " for module name " +
+          underName + ". 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(existingModule);
+      mModules.remove(existingNativeModule);
     }
 
-    namesToType.put(name, type);
-    ModuleHolder moduleHolder = new ModuleHolder(nativeModule);
+    namesToType.put(underName, type);
     mModules.put(type, moduleHolder);
   }