-
Notifications
You must be signed in to change notification settings - Fork 24.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(RCTAppDelegate): Implement RCTRootViewFactory
#42263
feat(RCTAppDelegate): Implement RCTRootViewFactory
#42263
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that this is what you actually want.
Here, you are not just creating another view.
You are also creating a new Bridge/ReactHost for every different window. You are initializing the Turbomodule system multple times, and so on.
Also, we are overloading the RCTAppDelegate
with responsibilities that it should not have.
The App delegate responsibilityi is to inizialize the app and act as a bridge between the OS and the app. If we want other capabilities, we should have different classes.
For example, we should create an RCTViewFactory
or a RCRootViewFactory
that is able to provide the root view based on the current state of Bridge/React Host, other parameters.
0fed142
to
d3a42d4
Compare
@cipolleschi Creating Also, I can work on implementing the |
Base commit: 496724f |
25fd3ab
to
721b9e7
Compare
RCTRootViewFactory
721b9e7
to
011db65
Compare
@cipolleschi I decided to jump straight into it and implement this class, I think it will make initialization of RN a lot easier also in brownfield apps. I tried to implement everything in a backward-compatible way |
packages/rn-tester/RNTester/NativeExampleViews/FlexibleSizeExampleView.mm
Outdated
Show resolved
Hide resolved
packages/react-native/Libraries/AppDelegate/RCTRootViewFactory.mm
Outdated
Show resolved
Hide resolved
Hi, @cipolleschi would you be willing to take a look at this and let me know if you're interested in pursuing it? It may require some polish, but I'd like to know if you're open to moving forward with this idea or if I should explore alternative options for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @okwasniewski thanks for the proposal.
I'm not completely convinced about it.
First of all, we are breaking the AppDelegate interface. I understand that practically nothing changes because we are moving all the methods to a protocol which is implemented by the AppDelegate, but I'd rather not doing this.
It will make harder to reach out for the documentation.
Secondly, I don't think that everything is properly supported. I saw that you are hardcoding fabricEnabled
to true and bridgelessEnabled
to false, for example.
packages/react-native/Libraries/AppDelegate/RCTRootViewFactory.mm
Outdated
Show resolved
Hide resolved
e0523bb
to
ba4fd7a
Compare
packages/react-native/Libraries/AppDelegate/RCTRootViewFactory.h
Outdated
Show resolved
Hide resolved
0a18f3d
to
9b49dad
Compare
Thanks for your review @cipolleschi I think now it's much better the public interface is not changed except for the I also fixed those hardcoded values |
c324093
to
3afa03c
Compare
@cipolleschi As discussed offline, I've refactored the implementation to drop the delegate pattern and now we pass a configuration object. I've also validated that this PR fixes a bug with RCTRootViewExample when running bridgeless mode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small nit, but it looks good to me. Thank you so much for this improvement! :D
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
…d of overwriting (#42634) Summary: For Bridgeless mode we set `fabric` and `concurrentRoot` property for newly initialized views. This example overwrites those properties. I think we should Instead copy previous dictionary and only overwrite `color` key. This issue results in a warning: "Using Fabric without concurrent root is deprecated. Please enable concurrent root for this application." Note: This Example crashes on Bridgeless but my other PR #42263 fixes it. ## Changelog: [INTERNAL] [FIXED] - UpdatePropertiesExampleView to mutate existing `appProperties` instead of overwriting Pull Request resolved: #42634 Test Plan: 1. Wait for this example to get fixed for Bridgeless 2. Click the button to update props 3. Check if there is no warning Reviewed By: cortinico Differential Revision: D53126953 Pulled By: cipolleschi fbshipit-source-id: fa0e8bda50a47696467d279845616c2ba51fe310
7237acc
to
f2c119a
Compare
Hey @cipolleschi, I've rebased the PR on top of the latest changes, is there anything I can do to help you get this merged? |
Hi @okwasniewski, sorry for the delay. I was quite busy in the past couple of weeks dealing with other high priority tasks and I couldn't really spend some time on this. I'll try to make this move forward this week. Thank you for your patience! |
@cipolleschi merged this pull request in ec928d7. |
Summary: This PR implements `RCTRootViewFactory` a utility class (suggested by cipolleschi) that returns proper RCTRootView based on the current environment state (new arch/old arch/bridgeless). This class aims to preserve background compatibility by implementing a configuration class forwarding necessary class to RCTAppDelegate. This PR leverages the `RCTRootViewFactory` in `RCTAppDelegate` for the default initialization of React Native (greenfield). Here is an example of creating a Brownfield integration (without RCTAppDelegate) using this class (can be later added to docs): 1. Store reference to `rootViewFactory` and to `UIWindow` `AppDelegate.h`: ```objc interface AppDelegate : UIResponder <UIApplicationDelegate> property(nonatomic, strong) UIWindow* window; property(nonatomic, strong) RCTRootViewFactory* rootViewFactory; end ``` 2. Create an initial configuration using `RCTRootViewFactoryConfiguration` and initialize `RCTRootViewFactory` using it. Then you can use the factory to create a new `RCTRootView` without worrying about old arch/new arch/bridgeless. `AppDelegate.mm` ```objc implementation AppDelegate - (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(NSDictionary<UIApplicationLaunchOptionsKey,id> *)launchOptions { // Create configuration RCTRootViewFactoryConfiguration *configuration = [[RCTRootViewFactoryConfiguration alloc] initWithBundleURL:self.bundleURL newArchEnabled:self.fabricEnabled turboModuleEnabled:self.turboModuleEnabled bridgelessEnabled:self.bridgelessEnabled]; // Initialize RCTRootViewFactory self.rootViewFactory = [[RCTRootViewFactory alloc] initWithConfiguration:configuration]; // Create main root view UIView *rootView = [self.rootViewFactory viewWithModuleName:@"RNTesterApp" initialProperties:@{} launchOptions:launchOptions]; // Set main window as you prefer for your Brownfield integration. self.window = [[UIWindow alloc] initWithFrame:[UIScreen mainScreen].bounds]; UIViewController *rootViewController = [UIViewController new]; rootViewController.view = rootView; self.window.rootViewController = rootViewController; [self.window makeKeyAndVisible]; // Later in the codebase you can initialize more rootView's using rootViewFactory. return YES; } end ``` bypass-github-export-checks [INTERNAL] [ADDED] - Implement RCTRootViewFactory Pull Request resolved: facebook#42263 Test Plan: Check if root view is properly created on app initialization Reviewed By: dmytrorykun Differential Revision: D53179625 Pulled By: cipolleschi fbshipit-source-id: 9bc850965ba30d84ad3e67d91dd888f0547c2136
Summary: This PR implements `RCTRootViewFactory` a utility class (suggested by cipolleschi) that returns proper RCTRootView based on the current environment state (new arch/old arch/bridgeless). This class aims to preserve background compatibility by implementing a configuration class forwarding necessary class to RCTAppDelegate. This PR leverages the `RCTRootViewFactory` in `RCTAppDelegate` for the default initialization of React Native (greenfield). Here is an example of creating a Brownfield integration (without RCTAppDelegate) using this class (can be later added to docs): 1. Store reference to `rootViewFactory` and to `UIWindow` `AppDelegate.h`: ```objc interface AppDelegate : UIResponder <UIApplicationDelegate> property(nonatomic, strong) UIWindow* window; property(nonatomic, strong) RCTRootViewFactory* rootViewFactory; end ``` 2. Create an initial configuration using `RCTRootViewFactoryConfiguration` and initialize `RCTRootViewFactory` using it. Then you can use the factory to create a new `RCTRootView` without worrying about old arch/new arch/bridgeless. `AppDelegate.mm` ```objc implementation AppDelegate - (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(NSDictionary<UIApplicationLaunchOptionsKey,id> *)launchOptions { // Create configuration RCTRootViewFactoryConfiguration *configuration = [[RCTRootViewFactoryConfiguration alloc] initWithBundleURL:self.bundleURL newArchEnabled:self.fabricEnabled turboModuleEnabled:self.turboModuleEnabled bridgelessEnabled:self.bridgelessEnabled]; // Initialize RCTRootViewFactory self.rootViewFactory = [[RCTRootViewFactory alloc] initWithConfiguration:configuration]; // Create main root view UIView *rootView = [self.rootViewFactory viewWithModuleName:@"RNTesterApp" initialProperties:@{} launchOptions:launchOptions]; // Set main window as you prefer for your Brownfield integration. self.window = [[UIWindow alloc] initWithFrame:[UIScreen mainScreen].bounds]; UIViewController *rootViewController = [UIViewController new]; rootViewController.view = rootView; self.window.rootViewController = rootViewController; [self.window makeKeyAndVisible]; // Later in the codebase you can initialize more rootView's using rootViewFactory. return YES; } end ``` bypass-github-export-checks [INTERNAL] [ADDED] - Implement RCTRootViewFactory Pull Request resolved: #42263 Test Plan: Check if root view is properly created on app initialization Reviewed By: dmytrorykun Differential Revision: D53179625 Pulled By: cipolleschi fbshipit-source-id: 9bc850965ba30d84ad3e67d91dd888f0547c2136
Summary:
This PR implements
RCTRootViewFactory
a utility class (suggested by @cipolleschi) that returns proper RCTRootView based on the current environment state (new arch/old arch/bridgeless). This class aims to preserve background compatibility by implementing a configuration class forwarding necessary class to RCTAppDelegate.Brownfield use case
This PR leverages the
RCTRootViewFactory
inRCTAppDelegate
for the default initialization of React Native (greenfield).Here is an example of creating a Brownfield integration (without RCTAppDelegate) using this class (can be later added to docs):
rootViewFactory
and toUIWindow
AppDelegate.h
:RCTRootViewFactoryConfiguration
and initializeRCTRootViewFactory
using it. Then you can use the factory to create a newRCTRootView
without worrying about old arch/new arch/bridgeless.AppDelegate.mm
Changelog:
[INTERNAL] [ADDED] - Implement RCTRootViewFactory
Test Plan:
Check if root view is properly created on app initialization