From 30d186c3683228d4fb7a42f804eb2fdfa7c8ac03 Mon Sep 17 00:00:00 2001 From: Nicola Corti Date: Wed, 7 Feb 2024 05:29:52 -0800 Subject: [PATCH] Set concurrentRoot to true whenever Fabric is used in renderApplication (#42821) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/42821 As the title says, we want `concurrentRoot iif fabric`. This makes sure we invoke renderApplication correctly. Changelog: [Internal] [Changed] - Set concurrentRoot to true whenever Fabric is used in renderApplication Reviewed By: sammy-SC Differential Revision: D53353017 fbshipit-source-id: 8de88adf528eb71f233233bd85c2c6ef9430fb16 --- .../Libraries/AppDelegate/RCTAppDelegate.h | 1 - .../Libraries/AppDelegate/RCTAppDelegate.mm | 5 ----- .../Libraries/ReactNative/renderApplication.js | 11 ++++------- .../com/facebook/react/ReactActivityDelegate.java | 1 - .../main/java/com/facebook/react/ReactDelegate.java | 1 - .../java/com/facebook/react/ReactInstanceManager.java | 4 +--- .../react/runtime/BridgelessDevSupportManager.java | 5 +---- .../com/facebook/react/ReactActivityDelegateTest.kt | 8 ++++---- 8 files changed, 10 insertions(+), 26 deletions(-) diff --git a/packages/react-native/Libraries/AppDelegate/RCTAppDelegate.h b/packages/react-native/Libraries/AppDelegate/RCTAppDelegate.h index 4c89afce6d7..e0edbef47a2 100644 --- a/packages/react-native/Libraries/AppDelegate/RCTAppDelegate.h +++ b/packages/react-native/Libraries/AppDelegate/RCTAppDelegate.h @@ -42,7 +42,6 @@ NS_ASSUME_NONNULL_BEGIN * - (UIViewController *)createRootViewController; * - (void)setRootView:(UIView *)rootView toRootViewController:(UIViewController *)rootViewController; * New Architecture: - * - (BOOL)concurrentRootEnabled * - (BOOL)turboModuleEnabled; * - (BOOL)fabricEnabled; * - (NSDictionary *)prepareInitialProps diff --git a/packages/react-native/Libraries/AppDelegate/RCTAppDelegate.mm b/packages/react-native/Libraries/AppDelegate/RCTAppDelegate.mm index 718f8ca5f83..8b811ab73e5 100644 --- a/packages/react-native/Libraries/AppDelegate/RCTAppDelegate.mm +++ b/packages/react-native/Libraries/AppDelegate/RCTAppDelegate.mm @@ -39,8 +39,6 @@ #import #import -static NSString *const kRNConcurrentRoot = @"concurrentRoot"; - @interface RCTAppDelegate () < RCTTurboModuleManagerDelegate, RCTComponentViewFactoryComponentProvider, @@ -53,9 +51,6 @@ static NSString *const kRNConcurrentRoot = @"concurrentRoot"; static NSDictionary *updateInitialProps(NSDictionary *initialProps, BOOL isFabricEnabled) { NSMutableDictionary *mutableProps = [initialProps mutableCopy] ?: [NSMutableDictionary new]; - // Hardcoding the Concurrent Root as it it not recommended to - // have the concurrentRoot turned off when Fabric is enabled. - mutableProps[kRNConcurrentRoot] = @(isFabricEnabled); return mutableProps; } diff --git a/packages/react-native/Libraries/ReactNative/renderApplication.js b/packages/react-native/Libraries/ReactNative/renderApplication.js index 613dfc52d8c..c6ca67f4014 100644 --- a/packages/react-native/Libraries/ReactNative/renderApplication.js +++ b/packages/react-native/Libraries/ReactNative/renderApplication.js @@ -83,16 +83,13 @@ export default function renderApplication( ); } - if (fabric && !useConcurrentRoot) { - console.warn( - 'Using Fabric without concurrent root is deprecated. Please enable concurrent root for this application.', - ); - } + // We want to have concurrentRoot always enabled when you're on Fabric. + const useConcurrentRootOverride = fabric; performanceLogger.startTimespan('renderApplication_React_render'); performanceLogger.setExtra( 'usedReactConcurrentRoot', - useConcurrentRoot ? '1' : '0', + useConcurrentRootOverride ? '1' : '0', ); performanceLogger.setExtra('usedReactFabric', fabric ? '1' : '0'); performanceLogger.setExtra( @@ -103,7 +100,7 @@ export default function renderApplication( element: renderable, rootTag, useFabric: Boolean(fabric), - useConcurrentRoot: Boolean(useConcurrentRoot), + useConcurrentRoot: Boolean(useConcurrentRootOverride), }); performanceLogger.stopTimespan('renderApplication_React_render'); } diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactActivityDelegate.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactActivityDelegate.java index 9df5a6800f7..87ffae6ef54 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactActivityDelegate.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactActivityDelegate.java @@ -61,7 +61,6 @@ public class ReactActivityDelegate { if (composedLaunchOptions == null) { composedLaunchOptions = new Bundle(); } - composedLaunchOptions.putBoolean("concurrentRoot", true); } return composedLaunchOptions; } diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactDelegate.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactDelegate.java index d64e45a1363..2140f4a0a1b 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactDelegate.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactDelegate.java @@ -240,7 +240,6 @@ public class ReactDelegate { if (composedLaunchOptions == null) { composedLaunchOptions = new Bundle(); } - composedLaunchOptions.putBoolean("concurrentRoot", true); } return composedLaunchOptions; } diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java index b0788f87da4..a600833e264 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java @@ -340,9 +340,7 @@ public class ReactInstanceManager { ReactRootView rootView = new ReactRootView(currentActivity); boolean isFabric = ReactFeatureFlags.enableFabricRenderer; rootView.setIsFabric(isFabric); - Bundle launchOptions = new Bundle(); - launchOptions.putBoolean("concurrentRoot", isFabric); - rootView.startReactApplication(ReactInstanceManager.this, appKey, launchOptions); + rootView.startReactApplication(ReactInstanceManager.this, appKey, new Bundle()); return rootView; } diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/BridgelessDevSupportManager.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/BridgelessDevSupportManager.java index 20ad955c085..3e4da7e9dc0 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/BridgelessDevSupportManager.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/BridgelessDevSupportManager.java @@ -135,11 +135,8 @@ class BridgelessDevSupportManager extends DevSupportManagerBase { public View createRootView(String appKey) { Activity currentActivity = getCurrentActivity(); if (currentActivity != null && !reactHost.isSurfaceWithModuleNameAttached(appKey)) { - Bundle launchOptions = new Bundle(); - launchOptions.putBoolean("concurrentRoot", true); - ReactSurfaceImpl reactSurface = - ReactSurfaceImpl.createWithView(currentActivity, appKey, launchOptions); + ReactSurfaceImpl.createWithView(currentActivity, appKey, new Bundle()); reactSurface.attach(reactHost); reactSurface.start(); diff --git a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/ReactActivityDelegateTest.kt b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/ReactActivityDelegateTest.kt index 66b4c9c4919..f0eb097b697 100644 --- a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/ReactActivityDelegateTest.kt +++ b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/ReactActivityDelegateTest.kt @@ -29,8 +29,8 @@ class ReactActivityDelegateTest { } assertNotNull(delegate.inspectLaunchOptions) - assertTrue(delegate.inspectLaunchOptions!!.containsKey("concurrentRoot")) - assertTrue(delegate.inspectLaunchOptions!!.getBoolean("concurrentRoot")) + // False because oncurrentRoot is hardcoded to true for Fabric inside renderApplication + assertFalse(delegate.inspectLaunchOptions!!.containsKey("concurrentRoot")) } @Test @@ -60,8 +60,8 @@ class ReactActivityDelegateTest { } assertNotNull(delegate.inspectLaunchOptions) - assertTrue(delegate.inspectLaunchOptions!!.containsKey("concurrentRoot")) - assertTrue(delegate.inspectLaunchOptions!!.getBoolean("concurrentRoot")) + // False because oncurrentRoot is hardcoded to true for Fabric inside renderApplication + assertFalse(delegate.inspectLaunchOptions!!.containsKey("concurrentRoot")) assertTrue(delegate.inspectLaunchOptions!!.containsKey("test-property")) assertEquals("test-value", delegate.inspectLaunchOptions!!.getString("test-property")) }