Skip to content

Commit

Permalink
Ensure TM system has consistent view of interop flags (#39086)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: #39086

Our largest blocker for the TurboModule interop layer is a "module not found" issue.

**Hypothesis:** This is a gating-related bug.

## Changes
This diff tries to simplify the gating of the TurboModule interop layer: Instead of reading the flags again and again from two different classes (the module manager and its delegate), just read the flags once, when the module system is initialized:

https://www.internalfb.com/code/fbsource/[ae79b760626ec81ceadbf2829e1593199d4df031]/xplat/js/react-native-github/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridgeless/ReactInstance.java?lines=106-113%2C210-215%2C217-223%2C251

This will ensure that the TurboModule system has one consistent view of the interop layer flags, throughout its lifetime.

Changelog: [Internal]

Reviewed By: mdvacca

Differential Revision: D48489274

fbshipit-source-id: 05eb64c5f7bd89dd65aac7390c3eb09234d87f96
  • Loading branch information
RSNara authored and facebook-github-bot committed Aug 21, 2023
1 parent 489d890 commit f312d6e
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,26 @@ interface ModuleProvider {
private final Map<ModuleProvider, Map<String, ReactModuleInfo>> mPackageModuleInfos =
new HashMap<>();

private static boolean shouldSupportLegacyPackages() {
return ReactFeatureFlags.enableBridgelessArchitecture
&& ReactFeatureFlags.unstable_useTurboModuleInterop;
private final boolean mShouldEnableLegacyModuleInterop =
ReactFeatureFlags.enableBridgelessArchitecture
&& ReactFeatureFlags.unstable_useTurboModuleInterop;

private final boolean mShouldRouteTurboModulesThroughLegacyModuleInterop =
mShouldEnableLegacyModuleInterop
&& ReactFeatureFlags.unstable_useTurboModuleInteropForAllTurboModules;

@Override
public boolean unstable_shouldEnableLegacyModuleInterop() {
return mShouldEnableLegacyModuleInterop;
}

@Override
public boolean unstable_shouldRouteTurboModulesThroughLegacyModuleInterop() {
return mShouldRouteTurboModulesThroughLegacyModuleInterop;
}

private static boolean shouldCreateLegacyModules() {
return ReactFeatureFlags.enableBridgelessArchitecture
&& ReactFeatureFlags.unstable_useTurboModuleInterop;
private boolean shouldSupportLegacyPackages() {
return unstable_shouldEnableLegacyModuleInterop();
}

protected ReactPackageTurboModuleManagerDelegate(
Expand Down Expand Up @@ -191,7 +203,7 @@ public boolean unstable_isLegacyModuleRegistered(String moduleName) {
@Nullable
@Override
public NativeModule getLegacyModule(String moduleName) {
if (!shouldCreateLegacyModules()) {
if (!unstable_shouldEnableLegacyModuleInterop()) {
return null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import com.facebook.react.bridge.ReactNoCrashSoftException;
import com.facebook.react.bridge.ReactSoftExceptionLogger;
import com.facebook.react.bridge.RuntimeExecutor;
import com.facebook.react.config.ReactFeatureFlags;
import com.facebook.react.turbomodule.core.interfaces.CallInvokerHolder;
import com.facebook.react.turbomodule.core.interfaces.NativeMethodCallInvokerHolder;
import com.facebook.react.turbomodule.core.interfaces.TurboModule;
Expand Down Expand Up @@ -72,7 +71,7 @@ public TurboModuleManager(
(CallInvokerHolderImpl) jsCallInvokerHolder,
(NativeMethodCallInvokerHolderImpl) nativeMethodCallInvokerHolder,
delegate);
installJSIBindings(shouldCreateLegacyModules());
installJSIBindings(shouldEnableLegacyModuleInterop());

mEagerInitModuleNames =
delegate == null ? new ArrayList<>() : delegate.getEagerInitModuleNames();
Expand All @@ -85,7 +84,7 @@ public TurboModuleManager(
: moduleName -> (NativeModule) delegate.getModule(moduleName);

mLegacyModuleProvider =
delegate == null || !shouldCreateLegacyModules()
delegate == null || !shouldEnableLegacyModuleInterop()
? nullProvider
: moduleName -> {
NativeModule nativeModule = delegate.getLegacyModule(moduleName);
Expand All @@ -108,15 +107,13 @@ private boolean isLegacyModule(String moduleName) {
return mDelegate != null && mDelegate.unstable_isLegacyModuleRegistered(moduleName);
}

private static boolean shouldCreateLegacyModules() {
return ReactFeatureFlags.enableBridgelessArchitecture
&& ReactFeatureFlags.unstable_useTurboModuleInterop;
private boolean shouldEnableLegacyModuleInterop() {
return mDelegate != null && mDelegate.unstable_shouldEnableLegacyModuleInterop();
}

private static boolean shouldRouteTurboModulesThroughInteropLayer() {
return ReactFeatureFlags.enableBridgelessArchitecture
&& ReactFeatureFlags.unstable_useTurboModuleInterop
&& ReactFeatureFlags.unstable_useTurboModuleInteropForAllTurboModules;
private boolean shouldRouteTurboModulesThroughLegacyModuleInterop() {
return mDelegate != null
&& mDelegate.unstable_shouldRouteTurboModulesThroughLegacyModuleInterop();
}

@Override
Expand All @@ -134,7 +131,7 @@ private static List<TurboModuleInteropUtils.MethodDescriptor> getMethodDescripto
@DoNotStrip
@Nullable
private NativeModule getLegacyJavaModule(String moduleName) {
if (shouldRouteTurboModulesThroughInteropLayer()) {
if (shouldRouteTurboModulesThroughLegacyModuleInterop()) {
final NativeModule module = getModule(moduleName);
return !(module instanceof CxxModuleWrapper) ? module : null;
}
Expand All @@ -157,7 +154,7 @@ private NativeModule getLegacyJavaModule(String moduleName) {
@DoNotStrip
@Nullable
private CxxModuleWrapper getLegacyCxxModule(String moduleName) {
if (shouldRouteTurboModulesThroughInteropLayer()) {
if (shouldRouteTurboModulesThroughLegacyModuleInterop()) {
final NativeModule module = getModule(moduleName);
return module instanceof CxxModuleWrapper ? (CxxModuleWrapper) module : null;
}
Expand All @@ -180,7 +177,7 @@ private CxxModuleWrapper getLegacyCxxModule(String moduleName) {
@DoNotStrip
@Nullable
private CxxModuleWrapper getTurboLegacyCxxModule(String moduleName) {
if (shouldRouteTurboModulesThroughInteropLayer()) {
if (shouldRouteTurboModulesThroughLegacyModuleInterop()) {
return null;
}

Expand All @@ -201,7 +198,7 @@ private CxxModuleWrapper getTurboLegacyCxxModule(String moduleName) {
@DoNotStrip
@Nullable
private TurboModule getTurboJavaModule(String moduleName) {
if (shouldRouteTurboModulesThroughInteropLayer()) {
if (shouldRouteTurboModulesThroughLegacyModuleInterop()) {
return null;
}

Expand Down Expand Up @@ -398,9 +395,9 @@ public boolean hasModule(String moduleName) {
return false;
}

private static void logError(String message) {
private void logError(String message) {
FLog.e("TurboModuleManager", message);
if (shouldRouteTurboModulesThroughInteropLayer()) {
if (shouldRouteTurboModulesThroughLegacyModuleInterop()) {
ReactSoftExceptionLogger.logSoftException(
"TurboModuleManager", new ReactNoCrashSoftException(message));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,5 +57,18 @@ public List<String> getEagerInitModuleNames() {
return new ArrayList<>();
}

/** Can the TurboModule system create legacy modules? */
public boolean unstable_shouldEnableLegacyModuleInterop() {
return false;
}

/**
* Should the TurboModule system treat all turbo native modules as though they were legacy
* modules? This method is for testing purposes only.
*/
public boolean unstable_shouldRouteTurboModulesThroughLegacyModuleInterop() {
return false;
}

protected synchronized void maybeLoadOtherSoLibraries() {}
}

0 comments on commit f312d6e

Please sign in to comment.