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

Implement multiple view manager lookup for the interop layer on Android #43595

Conversation

gabrieldonadel
Copy link
Collaborator

Summary:

When running with the new architurece and using the renderer interop layer on Android, view managers don't work if their names start with RCT. This happens because the RCT prefix is automatically removed from the component name, and inside the internal mViewManagers we store view managers with the RCT prefix.

Using the RCT pattern as a prefix works fine with the old architecture and is actually used on the Android Native UI Components tutorial in the docs, making me believe that this same patterns is used across many community libraries.

This diff adds a secondary lookup logic for view managers:

  1. We look for the XXXViewManager.
  2. If not found, we look for RCTXXXViewManager.

Quite similar to the iOS implementation introduced in #38093


With this change we can also remove most of the entries from FabricNameComponentMapping (I can address this in a follow up PR)

class FabricNameComponentMapping {
private static @NonNull final Map<String, String> sComponentNames = new HashMap<>();
static {
// TODO T97384889: unify component names between JS - Android - iOS - C++
sComponentNames.put("View", "RCTView");
sComponentNames.put("Image", "RCTImageView");
sComponentNames.put("ScrollView", "RCTScrollView");
sComponentNames.put("Slider", "RCTSlider");
sComponentNames.put("ModalHostView", "RCTModalHostView");
sComponentNames.put("Paragraph", "RCTText");
sComponentNames.put("Text", "RCText");
sComponentNames.put("RawText", "RCTRawText");
sComponentNames.put("ActivityIndicatorView", "AndroidProgressBar");
sComponentNames.put("ShimmeringView", "RKShimmeringView");
sComponentNames.put("TemplateView", "RCTTemplateView");
sComponentNames.put("AxialGradientView", "RCTAxialGradientView");
sComponentNames.put("Video", "RCTVideo");
sComponentNames.put("Map", "RCTMap");
sComponentNames.put("WebView", "RCTWebView");
sComponentNames.put("Keyframes", "RCTKeyframes");
sComponentNames.put("ImpressionTrackingView", "RCTImpressionTrackingView");
}

Changelog:

[ANDROID] [ADDED] - Implement multiple view manager lookup for the interop layer

Test Plan:

Tested installing a library such as react-native-fbsdk-next that names its view managers starting with RCT

Before After
image image

@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. 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 Mar 21, 2024
@analysis-bot
Copy link

analysis-bot commented Mar 21, 2024

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 18,093,685 +1
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 21,462,802 +7
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: c574ca5
Branch: main

@gabrieldonadel gabrieldonadel force-pushed the @gabrieldonadel/fix-rct-view-manager-android branch from 980aa15 to b9fc47a Compare March 21, 2024 16:36
@brentvatne
Copy link
Collaborator

this seems very reasonable to me! it's consistent with the ios behavior, and it should help make it easier for folks to adopt the new arch. i'd love to see this land in 0.74

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Mar 22, 2024
@facebook-github-bot
Copy link
Contributor

@arushikesarwani94 merged this pull request in 15a5638.

@gabrieldonadel gabrieldonadel deleted the @gabrieldonadel/fix-rct-view-manager-android branch March 22, 2024 02:24
@cortinico
Copy link
Contributor

@gabrieldonadel can you add a pick request on https://github.com/reactwg/react-native-releases/issues/new/choose for this one?

@gabrieldonadel
Copy link
Collaborator Author

@gabrieldonadel can you add a pick request on https://github.com/reactwg/react-native-releases/issues/new/choose for this one?

Yep, already did it yesterday reactwg/react-native-releases#172. Thanks for the remainder

huntie pushed a commit that referenced this pull request Mar 25, 2024
…id (#43595)

Summary:
When running with the new architurece and using the renderer interop layer on Android, view managers don't work if their names start with `RCT`. This happens because the `RCT` prefix is automatically removed from the component name, and inside the internal `mViewManagers` we store view managers with the `RCT` prefix.

Using the `RCT` pattern as a prefix works fine with the old architecture and is actually used on the [Android Native UI Components](https://reactnative.dev/docs/next/native-components-android) tutorial in the docs, making me believe that this same patterns is used across many community libraries.

This diff adds a secondary lookup logic for view managers:

1. We look for the XXXViewManager.
2. If not found, we look for RCTXXXViewManager.

Quite similar to the iOS implementation introduced in  #38093

 ---

With this change we can also remove most of the entries from FabricNameComponentMapping (I can address this in a follow up PR)

https://github.com/facebook/react-native/blob/4e6eba7a2dedaa855af0bff5df3bec73a95f0fc4/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/FabricNameComponentMapping.java#L22-L45

## Changelog:

[ANDROID] [ADDED] - Implement multiple view manager lookup for the interop layer

Pull Request resolved: #43595

Test Plan:
Tested installing a library such as [react-native-fbsdk-next](https://github.com/thebergamo/react-native-fbsdk-next) that names its view managers starting with `RCT`

<table>
<tr>
  <th>Before</th>
  <th>After</th>
</tr>
<tr>
<td>
<img width="519" alt="image" src="https://github.com/facebook/react-native/assets/11707729/123de1d6-f018-424b-b6ce-38221af9d83e">
</td>
<td><img width="519" alt="image" src="https://github.com/facebook/react-native/assets/11707729/0f35b369-e2e4-4bbf-b880-6471fbc05d38">
</td>
</tr>
</table>

Reviewed By: cortinico

Differential Revision: D55208396

Pulled By: arushikesarwani94

fbshipit-source-id: a1fb1f4cee8483cf91ebededd1d7c4ba7021f9d9
This was referenced Jun 28, 2024
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. 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