Deprecate TurboModuleManagerDelegate.getLegacyCxxModule #36667
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: D44407802 |
react-native-bot
added
Platform: Android
Base commit: 4e0dfed |
|
This pull request was exported from Phabricator. Differential Revision: D44407802 |
RSNara
force-pushed
the
export-D44407802
branch
from
8f50f99 to
9d41630
Compare
|
This pull request was exported from Phabricator. Differential Revision: D44407802 |
RSNara
force-pushed
the
export-D44407802
branch
from
9d41630 to
ec0a2ad
Compare
|
This pull request was exported from Phabricator. Differential Revision: D44407802 |
RSNara
added a commit
to RSNara/react-native
that referenced
this pull request
Mar 28, 2023
Summary: Pull Request resolved: facebook#36667 ## Context TurboModuleManagerDelegate exposes two methods that create TurboModules: - TurboModule getModule() - CxxModuleWrapper getLegacyCxxModule() ## Problem TurboModuleManagerDelegate.getLegacyCxxModule() is redundant: getModule() could just return all the modules that getLegacyCxxModule() returns: getLegacyCxxModule returns modules that implement TurboModule. ## Changes So, let's deprecate getLegacyCxxModule(). This will simplify the implementation of TurboModuleManager. Changelog: [Android][Deprecated] - Deprecate TurboModuleManager.getLegacyCxxModule Reviewed By: cortinico Differential Revision: D44407802 fbshipit-source-id: 2ebaca6fbcaac6d8a67ea2ab3394be2f110f71ec
RSNara
force-pushed
the
export-D44407802
branch
from
ec0a2ad to
7c6a03d
Compare
|
This pull request was exported from Phabricator. Differential Revision: D44407802 |
RSNara
added a commit
to RSNara/react-native
that referenced
this pull request
Mar 28, 2023
Summary: Pull Request resolved: facebook#36667 ## Context TurboModuleManagerDelegate exposes two methods that create TurboModules: - TurboModule getModule() - CxxModuleWrapper getLegacyCxxModule() ## Problem TurboModuleManagerDelegate.getLegacyCxxModule() is redundant: getModule() could just return all the modules that getLegacyCxxModule() returns: getLegacyCxxModule returns modules that implement TurboModule. ## Changes So, let's deprecate getLegacyCxxModule(). This will simplify the implementation of TurboModuleManager. Changelog: [Android][Deprecated] - Deprecate TurboModuleManager.getLegacyCxxModule Reviewed By: cortinico Differential Revision: D44407802 fbshipit-source-id: 1cecca4ba3633d1d2b12f1611210be22ea637962
RSNara
force-pushed
the
export-D44407802
branch
from
7c6a03d to
67b8487
Compare
Summary: In Bridgeless mode, With the TurboModule interop layer, the TurboModule system will need to customize the nativeModuleProxy global. This customization would be much easier if the nativeModuleProxy global were installed by the TurboModule system (and not the Bridgeless core). So, this diff moves nativeModuleProxy installation into the TurboModule system. Changelog: [Internal] Differential Revision: https://internalfb.com/D43993197 fbshipit-source-id: 300b77e84cf2df3fd6712f2788817b487a999001
Summary: ## Context Previously, jsRepresentation would only cache the **HostFunctions** returned from TurboModule::createHostFunction(). ## Changes This diff replaces TurboModule::createHostFunction() with TurboModule::create(). Now, jsRepresentation will cache **all** the **properties** returned from TurboModule::create(). ## Motivation For interop modules, constants will be exported as properties on the TurboModule HostObject. This diff allows those constants (which are non HostFunctions) to be cached. Changelog: [Internal] Differential Revision: https://internalfb.com/D44253229 fbshipit-source-id: d5576b4d0d063035fe1cca2df582c4f5514fe6e7
Summary: The legacy NativeModule system supports integer and float in NativeModule method arguments and returns. This diff extends the TurboModule system for the same functionality. T his is necessary because the TurboModule system will now need to dispatch method calls to legacy NativeModules. NOTE: We can't actually test these changes until we run interop modules. Changelog: [Internal] Differential Revision: https://internalfb.com/D44000389 fbshipit-source-id: c82fe05309334d9a60484a7ab9b6f7e3b26760e9
Summary: The scope of TurboModuleManager is increasing: - Eventually, it'll be capable of creating interop NativeModules (i.e: NativeModules that don't implement TurboModule). So, instead of creating duplicate methods for NativeModules on the TurboModuleManager, this diff changes the APIs of TurboModuleManager to work with the NativeModule interface. Thoughts? ## Questions **Question:** Is this a breaking change for open source? - Technically, yes. This diff changes the public interface of TurboModuleManager. **Question:** How large of a thrash will this cause for open source apps? - The thrash should be minimal. People in open source shouldn't be creating their own TurboModuleManager. They also shouldn't be directly accessing the TurboModuleManager object either. **Question:** Is this change safe? - Yeah. All the code that calls into TurboModuleRegistry converts TurboModules it returns into NativeModules. **Question:** Is this change move us in the right direction? - Long term, the TurboModule system will support legacy modules as well as TurboModules. - I think it makes a lot of sense to have one Java-facing registry: after all, Java will just treat these NativeModules/TurboModules as regular Java objects, and call public methods on them. It doesn't care if the module is TurboModule-compatible or not. - As for the TurboModuleRegistry abstraction, I think we should eventually rename this to NativeModuleRegistry after we delete the current NativeModuleRegistry. - Still thinking about this though. I will leave this diff in review to welcome comments. Changelog: [Android][Deprecated] - Deprecate TurboModuleRegistry.getModule(), getModules(), hasModule(), Differential Revision: https://internalfb.com/D43801531 fbshipit-source-id: 76d7cc84c385d0c4e8fcaabfceb257e8690595bc
Summary: Pull Request resolved: facebook#36667 ## Context TurboModuleManagerDelegate exposes two methods that create TurboModules: - TurboModule getModule() - CxxModuleWrapper getLegacyCxxModule() ## Problem TurboModuleManagerDelegate.getLegacyCxxModule() is redundant: getModule() could just return all the modules that getLegacyCxxModule() returns: getLegacyCxxModule returns modules that implement TurboModule. ## Changes So, let's deprecate getLegacyCxxModule(). This will simplify the implementation of TurboModuleManager. Changelog: [Android][Deprecated] - Deprecate TurboModuleManager.getLegacyCxxModule Reviewed By: cortinico Differential Revision: D44407802 fbshipit-source-id: fc711c4682c9bdd303dfb2e94e68dc088f6cabb5
|
This pull request was exported from Phabricator. Differential Revision: D44407802 |
RSNara
force-pushed
the
export-D44407802
branch
from
67b8487 to
b9f0431
Compare
|
This pull request has been merged in 7a08fbb. |
jeongshin
pushed a commit
to jeongshin/react-native
that referenced
this pull request
May 7, 2023
Summary: Pull Request resolved: facebook#36667 ## Context TurboModuleManagerDelegate exposes two methods that create TurboModules: - TurboModule getModule() - CxxModuleWrapper getLegacyCxxModule() ## Problem TurboModuleManagerDelegate.getLegacyCxxModule() is redundant: getModule() could just return all the modules that getLegacyCxxModule() returns: getLegacyCxxModule returns modules that implement TurboModule. ## Changes So, let's deprecate getLegacyCxxModule(). This will simplify the implementation of TurboModuleManager. Changelog: [Android][Deprecated] - Deprecate TurboModuleManager.getLegacyCxxModule Reviewed By: cortinico Differential Revision: D44407802 fbshipit-source-id: 88a6cf6597db76d8a74fd777d68ccf4f43aa6811
OlimpiaZurek
pushed a commit
to OlimpiaZurek/react-native
that referenced
this pull request
May 22, 2023
Summary: Pull Request resolved: facebook#36667 ## Context TurboModuleManagerDelegate exposes two methods that create TurboModules: - TurboModule getModule() - CxxModuleWrapper getLegacyCxxModule() ## Problem TurboModuleManagerDelegate.getLegacyCxxModule() is redundant: getModule() could just return all the modules that getLegacyCxxModule() returns: getLegacyCxxModule returns modules that implement TurboModule. ## Changes So, let's deprecate getLegacyCxxModule(). This will simplify the implementation of TurboModuleManager. Changelog: [Android][Deprecated] - Deprecate TurboModuleManager.getLegacyCxxModule Reviewed By: cortinico Differential Revision: D44407802 fbshipit-source-id: 88a6cf6597db76d8a74fd777d68ccf4f43aa6811
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:
Context
TurboModuleManagerDelegate exposes two methods that create TurboModules:
Problem
TurboModuleManagerDelegate.getLegacyCxxModule() is redundant: getModule() could just return all the modules that getLegacyCxxModule() returns: getLegacyCxxModule returns modules that implement TurboModule.
Changes
So, let's deprecate getLegacyCxxModule(). This will simplify the implementation of TurboModuleManager.
Changelog: [Android][Deprecated] - Deprecate TurboModuleManager.getLegacyCxxModule
Reviewed By: cortinico
Differential Revision: D44407802