Implement multiple manager lookup for the interop layer #38093
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D47055969 |
Base commit: 3be3731 |
Summary: Pull Request resolved: facebook#38093 In [this issue](facebook#37905), the community detected a strict assumption in the interop layer for which, given a component `XXXView`, its ViewManager must be called `RCTXXXViewManager`. That's not the case for some components, for example, `BVLinearGradient`, which View manager is `BVLinearGradientManager` and not `RCTBVLinearGradientManager`. This diff adds a secondary lookup logic: 1. We look for the `RCTXXXViewManager`. 2. If not found, we look for `XXXViewManager`. We will assess whether to generalize this logic once and for all or to expand other lookup cases on an example/failure basis as it's not a goal to have a 100% accurate interop layer. The Goal is to cover most use cases. ## Changelog: [iOS][Added] - Allow to lookup for ViewManager without the RCT prefix in the Interop Layer Reviewed By: sammy-SC Differential Revision: D47055969 fbshipit-source-id: b648c79e54d37d571a0ee8c490745f587b1327a1
|
This pull request was exported from Phabricator. Differential Revision: D47055969 |
cipolleschi
force-pushed
the
export-D47055969
branch
from
ece34c4
to
26dd5a5
Compare
|
This pull request has been merged in a28881a. |
cipolleschi
added a commit
that referenced
this pull request
Jun 28, 2023
Summary: Pull Request resolved: #38093 In [this issue](#37905), the community detected a strict assumption in the interop layer for which, given a component `XXXView`, its ViewManager must be called `RCTXXXViewManager`. That's not the case for some components, for example, `BVLinearGradient`, which View manager is `BVLinearGradientManager` and not `RCTBVLinearGradientManager`. This diff adds a secondary lookup logic: 1. We look for the `RCTXXXViewManager`. 2. If not found, we look for `XXXViewManager`. We will assess whether to generalize this logic once and for all or to expand other lookup cases on an example/failure basis as it's not a goal to have a 100% accurate interop layer. The Goal is to cover most use cases. [iOS][Added] - Allow to lookup for ViewManager without the RCT prefix in the Interop Layer Reviewed By: sammy-SC Differential Revision: D47055969 fbshipit-source-id: 1d31f3f4bc6f1f543edbd157ce04ad9daf23dbc6
Kudo
pushed a commit
to expo/react-native
that referenced
this pull request
Jul 3, 2023
Summary: Pull Request resolved: facebook#38093 In [this issue](facebook#37905), the community detected a strict assumption in the interop layer for which, given a component `XXXView`, its ViewManager must be called `RCTXXXViewManager`. That's not the case for some components, for example, `BVLinearGradient`, which View manager is `BVLinearGradientManager` and not `RCTBVLinearGradientManager`. This diff adds a secondary lookup logic: 1. We look for the `RCTXXXViewManager`. 2. If not found, we look for `XXXViewManager`. We will assess whether to generalize this logic once and for all or to expand other lookup cases on an example/failure basis as it's not a goal to have a 100% accurate interop layer. The Goal is to cover most use cases. [iOS][Added] - Allow to lookup for ViewManager without the RCT prefix in the Interop Layer Reviewed By: sammy-SC Differential Revision: D47055969 fbshipit-source-id: 1d31f3f4bc6f1f543edbd157ce04ad9daf23dbc6
This was referenced Aug 10, 2023
This was referenced Jan 4, 2024
This was referenced Feb 28, 2024
facebook-github-bot
pushed a commit
that referenced
this pull request
Mar 22, 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
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
In this issue, the community detected a strict assumption in the interop layer for which, given a component
XXXView, its ViewManager must be calledRCTXXXViewManager.That's not the case for some components, for example,
BVLinearGradient, which View manager isBVLinearGradientManagerand notRCTBVLinearGradientManager.This diff adds a secondary lookup logic:
RCTXXXViewManager.XXXViewManager.We will assess whether to generalize this logic once and for all or to expand other lookup cases on an example/failure basis as it's not a goal to have a 100% accurate interop layer. The Goal is to cover most use cases.
Changelog:
[iOS][Added] - Allow to lookup for ViewManager without the RCT prefix in the Interop Layer
Differential Revision: D47055969