Do not suppress IllegalArgumentExceptions during TurboModule creation (…

…facebook#41509) Summary: We currently ignore `IllegalArgumentException` being thrown from TurboModule getters, as it was deemed acceptable to use that to signal a Package didn't have that module. This however can mask legitimate errors thrown during a TurboModule constructor. TurboReactPackage#getModule is already marked as allowing nullable returns, so let's leave the use of exceptions for exceptional scenarios. Changelog: [Android][Changed] Use null to signal a missing TurboModule instead of IllegalArgumentException. Reviewed By: RSNara Differential Revision: D51395165
javache · Nov 16, 2023 · d47f8fa · d47f8fa
1 parent 5730f56
commit d47f8fa
Show file tree

Hide file tree

Showing 5 changed files with 18 additions and 15 deletions.
diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/DebugCorePackage.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/DebugCorePackage.java
@@ -48,8 +48,7 @@ public NativeModule getModule(String name, ReactApplicationContext reactContext)
       case JSCHeapCapture.NAME:
         return new JSCHeapCapture(reactContext);
       default:
-        throw new IllegalArgumentException(
-            "In DebugCorePackage, could not find Native module for " + name);
+        return null;
     }
   }
 

diff --git a/...-native/ReactAndroid/src/main/java/com/facebook/react/LazyTurboModuleManagerDelegate.java b/...-native/ReactAndroid/src/main/java/com/facebook/react/LazyTurboModuleManagerDelegate.java
@@ -8,8 +8,10 @@
 package com.facebook.react;
 
 import androidx.annotation.Nullable;
+import com.facebook.common.logging.FLog;
 import com.facebook.react.bridge.NativeModule;
 import com.facebook.react.bridge.ReactApplicationContext;
+import com.facebook.react.common.ReactConstants;
 import com.facebook.react.common.annotations.UnstableReactNativeAPI;
 import com.facebook.react.internal.turbomodule.core.interfaces.TurboModule;
 import java.util.List;
@@ -56,11 +58,11 @@ public TurboModule getModule(String moduleName) {
             return nativeModule;
           }
         } catch (IllegalArgumentException ex) {
-          /*
-           TurboReactPackages can throw an IllegalArgumentException when a module isn't found. If
-           this happens, it's safe to ignore the exception because a later TurboReactPackage could
-           provide the module.
-          */
+          FLog.e(
+              ReactConstants.TAG,
+              ex,
+              "Caught exception while constructing module '%s'. This was previously ignored but will not be caught in the future.",
+              moduleName);
         }
       } else {
         throw new IllegalArgumentException("ReactPackage must be an instance of TurboReactPackage");

diff --git a/...ReactAndroid/src/main/java/com/facebook/react/ReactPackageTurboModuleManagerDelegate.java b/...ReactAndroid/src/main/java/com/facebook/react/ReactPackageTurboModuleManagerDelegate.java
@@ -8,11 +8,13 @@
 package com.facebook.react;
 
 import androidx.annotation.Nullable;
+import com.facebook.common.logging.FLog;
 import com.facebook.infer.annotation.Assertions;
 import com.facebook.react.bridge.CxxModuleWrapper;
 import com.facebook.react.bridge.ModuleSpec;
 import com.facebook.react.bridge.NativeModule;
 import com.facebook.react.bridge.ReactApplicationContext;
+import com.facebook.react.common.ReactConstants;
 import com.facebook.react.config.ReactFeatureFlags;
 import com.facebook.react.internal.turbomodule.core.TurboModuleManagerDelegate;
 import com.facebook.react.internal.turbomodule.core.interfaces.TurboModule;
@@ -161,11 +163,11 @@ public TurboModule getModule(String moduleName) {
         }
 
       } catch (IllegalArgumentException ex) {
-        /*
-         TurboReactPackages can throw an IllegalArgumentException when a module isn't found. If
-         this happens, it's safe to ignore the exception because a later TurboReactPackage could
-         provide the module.
-        */
+        FLog.e(
+            ReactConstants.TAG,
+            ex,
+            "Caught exception while constructing module '%s'. This was previously ignored but will not be caught in the future.",
+            moduleName);
       }
     }
 

diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/ModuleHolder.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/ModuleHolder.java
@@ -19,6 +19,7 @@
 import com.facebook.debug.tags.ReactDebugOverlayTags;
 import com.facebook.infer.annotation.Assertions;
 import com.facebook.proguard.annotations.DoNotStrip;
+import com.facebook.react.common.ReactConstants;
 import com.facebook.react.internal.turbomodule.core.interfaces.TurboModule;
 import com.facebook.react.module.model.ReactModuleInfo;
 import com.facebook.systrace.SystraceMessage;
@@ -204,7 +205,7 @@ private NativeModule create() {
        *
        * @todo(T53311351)
        */
-      FLog.e("NativeModuleInitError", "Failed to create NativeModule \"" + getName() + "\"", ex);
+      FLog.e(ReactConstants.TAG, ex, "Failed to create NativeModule '%s'", mName);
       throw ex;
     } finally {
       ReactMarker.logMarker(CREATE_MODULE_END, mName, mInstanceKey);

diff --git a/.../react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/CoreReactPackage.java b/.../react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/CoreReactPackage.java
@@ -68,8 +68,7 @@ public NativeModule getModule(String name, ReactApplicationContext reactContext)
       case ExceptionsManagerModule.NAME:
         return new ExceptionsManagerModule(mDevSupportManager);
       default:
-        throw new IllegalArgumentException(
-            "In BridgelessReactPackage, could not find Native module for " + name);
+        return null;
     }
   }