[iOS] RNTester enable concurrent root when using Fabric

[zhongwuzw]

Summary:

RNTester's AppDelegate override prepareInitialProps method of super class RCTAppDelegate

react-native/packages/react-native/Libraries/AppDelegate/RCTAppDelegate.mm

Line 152 in 70acd3f

initProps[kRNConcurrentRoot] = @([self fabricEnabled]);

, so we missed concurrentRoot initial prop.

cc @javache @cipolleschi

Changelog:

[IOS] [FIXED] - RNTester enable concurrent root when using Fabric

Test Plan:

Warning disappear.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	17,649,149	-2
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	21,026,731	-4,097
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: 586de42
Branch: main

[javache]

Per the docs, prepareInitialProps is a method that's supposed to be overwritten (although confusing that we have two ways do it - we also have the initialProps property...)

Maybe a better fix would be to change RCTAppDelegate.m to always override this property consistently, outside of prepareInitialProps.

[cipolleschi]

Wow, very well spotted. Thank you for this fix!

[facebook-github-bot]

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[zhongwuzw]

@javache @cipolleschi Thanks for the review, should I need to update the commit like below?

diff --git a/packages/react-native/Libraries/AppDelegate/RCTAppDelegate.mm b/packages/react-native/Libraries/AppDelegate/RCTAppDelegate.mm
index 43d4b0bd4b0..fa409a1d8ce 100644
--- a/packages/react-native/Libraries/AppDelegate/RCTAppDelegate.mm
+++ b/packages/react-native/Libraries/AppDelegate/RCTAppDelegate.mm
@@ -122,7 +122,12 @@ static NSString *const kRNConcurrentRoot = @"concurrentRoot";
     [self unstable_registerLegacyComponents];
     [RCTComponentViewFactory currentComponentViewFactory].thirdPartyFabricComponentsProvider = self;
 #endif
-    NSDictionary *initProps = [self prepareInitialProps];
+    NSMutableDictionary *initProps = [[self prepareInitialProps] mutableCopy] ?: [NSMutableDictionary new];
+#ifdef RCT_NEW_ARCH_ENABLED
+    // Hardcoding the Concurrent Root as it it not recommended to
+    // have the concurrentRoot turned off when Fabric is enabled.
+    initProps[kRNConcurrentRoot] = @([self fabricEnabled]);
+#endif
     rootView = [self createRootViewWithBridge:self.bridge moduleName:self.moduleName initProps:initProps];
   }
   self.window = [[UIWindow alloc] initWithFrame:[UIScreen mainScreen].bounds];
@@ -144,15 +149,7 @@ static NSString *const kRNConcurrentRoot = @"concurrentRoot";
 
 - (NSDictionary *)prepareInitialProps
 {
-  NSMutableDictionary *initProps = self.initialProps ? [self.initialProps mutableCopy] : [NSMutableDictionary new];
-
-#ifdef RCT_NEW_ARCH_ENABLED
-  // Hardcoding the Concurrent Root as it it not recommended to
-  // have the concurrentRoot turned off when Fabric is enabled.
-  initProps[kRNConcurrentRoot] = @([self fabricEnabled]);
-#endif
-
-  return initProps;
+  return self.initialProps;
 }
 
 - (RCTBridge *)createBridgeWithDelegate:(id<RCTBridgeDelegate>)delegate launchOptions:(NSDictionary *)launchOptions
diff --git a/packages/rn-tester/RNTester/AppDelegate.mm b/packages/rn-tester/RNTester/AppDelegate.mm
index 3f1209e3615..40582e7296b 100644
--- a/packages/rn-tester/RNTester/AppDelegate.mm
+++ b/packages/rn-tester/RNTester/AppDelegate.mm
@@ -36,12 +36,12 @@ NSString *kBundlePath = @"js/RNTesterApp.ios";
   self.moduleName = @"RNTesterApp";
   // You can add your custom initial props in the dictionary below.
   // They will be passed down to the ViewController used by React Native.
-  self.initialProps = [self setUpInitialProps];
+  self.initialProps = [self prepareInitialProps];
 
   return [super application:application didFinishLaunchingWithOptions:launchOptions];
 }
 
-- (NSDictionary *)setUpInitialProps
+- (NSDictionary *)prepareInitialProps
 {
   NSMutableDictionary *initProps = [NSMutableDictionary new];
 

[cipolleschi]

Oh sorry, I haven't seen the @javache review. @zhongwuzw I think that the best way would be in the RCTAppDelegate:

1. create another method that set the concurrent root
2. keep the prepareInitialProps as returning an empty dictionary, so it is safe to override
3. merge the two dictionaries before using them around line 124

So, if in the future we have to add other prop to the initialProps that we have to handle internally, we already have a place where to put them.

Thoughts?

[zhongwuzw]

@cipolleschi Done, please review again.

[zhongwuzw]

@javache Done. please review again, thanks!

[zhongwuzw]

@javache Done. Also update commit to fix the bridgeless mode.

[facebook-github-bot]

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[github-actions]

This pull request was successfully merged by @zhongwuzw in b1e92d6.

^{When will my fix make it into a release? | Upcoming Releases}