Skip to content
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

Support customizeRootView from RCTRootViewFactory #44775

Closed

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented Jun 4, 2024

Summary:

The new customizeRootView does not have the feature parity as createRootViewWithBridge where reusing RCTRootViewFactory to create a root view, it does not call customizeRootView. This PR moves the customizeRootView support from RCTAppDelegate into RCTRootViewFactory and improves the customizeRootView support.

Changelog:

[IOS] [CHANGED] - Support customizeRootView from RCTRootViewFactory

Test Plan:

Add customizeRootView to packages/rn-tester/RNTester/AppDelegate.mm and test whether RNTester has blue background color in both new arch and old arch mode.

- (void)customizeRootView:(RCTRootView *)rootView
{
  rootView.backgroundColor = [UIColor blueColor];
}

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. p: Expo Partner: Expo Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Jun 4, 2024
@analysis-bot
Copy link

analysis-bot commented Jun 4, 2024

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 19,675,021 -14
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 23,044,524 -1
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: bb61e07
Branch: main

@cipolleschi
Copy link
Contributor

Thanks for the PR, but I don't think we need this, @Kudo. We moved the customizeRootView method after the rootView is created, so that it is invoked independently from whether method is used to create the view.

Perhaps I'm missing something, but why we need to bring this customization into the RootView Factory?

@Kudo
Copy link
Contributor Author

Kudo commented Jun 4, 2024

@cipolleschi thanks for feedback. this is to resolve the issue when enable new architecture with react-native-bootsplash and expo-updates. there is a workaround patch in react-conf-app but i think it would be good to have the feature in RCTRootViewFactory, so that developers reuse RCTRootViewFactory can fully support customizeRootView.

we could apply the patch inside expo-modules-core if you think RCTRootViewFactory is not the right place.

more details: expo-updates will delay the timing to initialize react instance and create an empty root view rather than the RCTRootView. we need to show splash screen twice. one before empty root view and the other after we use RCTRootViewFactory to create the formal RCTRootView. react-native-bootsplash uses customizeRootView to setup and show splash screen twice. that's why we need customizeRootView to be supported from RCTRootViewFactory (or our derived EXReactRootViewFactory)

@Kudo Kudo force-pushed the @kudo/RCTRootViewFactory-customizeRootView branch from 4d2aedb to 8b54ed7 Compare June 5, 2024 07:47
Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution and the explanation!

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@cipolleschi merged this pull request in 8956869.

Copy link

github-actions bot commented Jun 6, 2024

This pull request was successfully merged by @Kudo in 8956869.

When will my fix make it into a release? | How to file a pick request?

@Kudo Kudo deleted the @kudo/RCTRootViewFactory-customizeRootView branch June 6, 2024 11:19
kosmydel pushed a commit to kosmydel/react-native that referenced this pull request Jun 11, 2024
Summary:
The new `customizeRootView` does not have the feature parity as `createRootViewWithBridge` where reusing RCTRootViewFactory to create a root view, it does not call `customizeRootView`. This PR moves the `customizeRootView` support from RCTAppDelegate into RCTRootViewFactory and improves the customizeRootView support.

## Changelog:

[IOS] [CHANGED] - Support `customizeRootView` from `RCTRootViewFactory`

Pull Request resolved: facebook#44775

Test Plan:
Add customizeRootView to **packages/rn-tester/RNTester/AppDelegate.mm** and test whether RNTester has blue background color in both new arch and old arch mode.

```objc
- (void)customizeRootView:(RCTRootView *)rootView
{
  rootView.backgroundColor = [UIColor blueColor];
}
```

Reviewed By: dmytrorykun

Differential Revision: D58179693

Pulled By: cipolleschi

fbshipit-source-id: 0fac9a1bd5b2583a2700b3a3d2c80d0f608c4481
Titozzz pushed a commit that referenced this pull request Jun 18, 2024
Summary:
The new `customizeRootView` does not have the feature parity as `createRootViewWithBridge` where reusing RCTRootViewFactory to create a root view, it does not call `customizeRootView`. This PR moves the `customizeRootView` support from RCTAppDelegate into RCTRootViewFactory and improves the customizeRootView support.

[IOS] [CHANGED] - Support `customizeRootView` from `RCTRootViewFactory`

Pull Request resolved: #44775

Test Plan:
Add customizeRootView to **packages/rn-tester/RNTester/AppDelegate.mm** and test whether RNTester has blue background color in both new arch and old arch mode.

```objc
- (void)customizeRootView:(RCTRootView *)rootView
{
  rootView.backgroundColor = [UIColor blueColor];
}
```

Reviewed By: dmytrorykun

Differential Revision: D58179693

Pulled By: cipolleschi

fbshipit-source-id: 0fac9a1bd5b2583a2700b3a3d2c80d0f608c4481
Titozzz pushed a commit that referenced this pull request Jun 18, 2024
Summary:
The new `customizeRootView` does not have the feature parity as `createRootViewWithBridge` where reusing RCTRootViewFactory to create a root view, it does not call `customizeRootView`. This PR moves the `customizeRootView` support from RCTAppDelegate into RCTRootViewFactory and improves the customizeRootView support.

[IOS] [CHANGED] - Support `customizeRootView` from `RCTRootViewFactory`

Pull Request resolved: #44775

Test Plan:
Add customizeRootView to **packages/rn-tester/RNTester/AppDelegate.mm** and test whether RNTester has blue background color in both new arch and old arch mode.

```objc
- (void)customizeRootView:(RCTRootView *)rootView
{
  rootView.backgroundColor = [UIColor blueColor];
}
```

Reviewed By: dmytrorykun

Differential Revision: D58179693

Pulled By: cipolleschi

fbshipit-source-id: 0fac9a1bd5b2583a2700b3a3d2c80d0f608c4481
Titozzz pushed a commit that referenced this pull request Jun 18, 2024
Summary:
The new `customizeRootView` does not have the feature parity as `createRootViewWithBridge` where reusing RCTRootViewFactory to create a root view, it does not call `customizeRootView`. This PR moves the `customizeRootView` support from RCTAppDelegate into RCTRootViewFactory and improves the customizeRootView support.

[IOS] [CHANGED] - Support `customizeRootView` from `RCTRootViewFactory`

Pull Request resolved: #44775

Test Plan:
Add customizeRootView to **packages/rn-tester/RNTester/AppDelegate.mm** and test whether RNTester has blue background color in both new arch and old arch mode.

```objc
- (void)customizeRootView:(RCTRootView *)rootView
{
  rootView.backgroundColor = [UIColor blueColor];
}
```

Reviewed By: dmytrorykun

Differential Revision: D58179693

Pulled By: cipolleschi

fbshipit-source-id: 0fac9a1bd5b2583a2700b3a3d2c80d0f608c4481
@zoontek
Copy link
Contributor

zoontek commented Jun 28, 2024

@cipolleschi @Kudo Does a 0.74.x version with this fix is planned?

@Kudo
Copy link
Contributor Author

Kudo commented Jun 28, 2024

yes, it was already picked reactwg/react-native-releases#312, would be available from next version

Kudo added a commit to expo/expo that referenced this pull request Jul 6, 2024
# Why

react-native 0.74.3 added `customizeRootView` support from
facebook/react-native#44775. we should clone the
code from `RCTAppDelegate.createRCTRootViewFactory` to our
`EXAppDelegateWrapper.createRCTRootViewFactory`
that could possibly fix react-native-bootsplash integration for
zoontek/react-native-bootsplash#590

# How

- reference
https://github.com/facebook/react-native/blob/8c8c77b974ce3d4c3036361e56276a6ff2b02208/packages/react-native/Libraries/AppDelegate/RCTAppDelegate.mm#L238-L262
to copy to `EXAppDelegateWrapper.createRCTRootViewFactory`
- since we are still on 0.74.2, we should use `respondsToSelector` to
check the existing property from RCTRootViewFactoryConfiguration.
Kudo added a commit to expo/expo that referenced this pull request Jul 6, 2024
react-native 0.74.3 added `customizeRootView` support from
facebook/react-native#44775. we should clone the
code from `RCTAppDelegate.createRCTRootViewFactory` to our
`EXAppDelegateWrapper.createRCTRootViewFactory`
that could possibly fix react-native-bootsplash integration for
zoontek/react-native-bootsplash#590

- reference
https://github.com/facebook/react-native/blob/8c8c77b974ce3d4c3036361e56276a6ff2b02208/packages/react-native/Libraries/AppDelegate/RCTAppDelegate.mm#L238-L262
to copy to `EXAppDelegateWrapper.createRCTRootViewFactory`
- since we are still on 0.74.2, we should use `respondsToSelector` to
check the existing property from RCTRootViewFactoryConfiguration.

(cherry picked from commit 7c7499a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. Merged This PR has been merged. p: Expo Partner: Expo Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants