remove cxx TM autolinking #45967
remove cxx TM autolinking #45967
Conversation
facebook-github-bot
added
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D61059182 |
|
This pull request was exported from Phabricator. Differential Revision: D61059182 |
philIip
force-pushed
the
export-D61059182
branch
from
5e6b051
to
befde9a
Compare
Summary: Changelog: [iOS][Android][Breaking] Pull Request resolved: facebook#45967 this was a meaningful exploration, but since we have support in TM for jsi runtime access and that's been widely advertised and accepted, let's get rid of this. it's a bit hacky and hard to use (shown by no one using it), so i want to stop any possibility of its usage. if interested in accessing jsi::Runtime via a native module, please consider the following options: - [Android] BindingsInstaller: facebook#44526 - [iOS] RCTTurboModuleWithJSIBindings: facebook#44486 - C++ TurboModules (no autolinking): https://github.com/reactwg/react-native-new-architecture/blob/main/docs/turbo-modules-xplat.md Reviewed By: christophpurrer Differential Revision: D61059182
|
2 similar comments
|
|
Summary: Changelog: [iOS][Android][Breaking] Pull Request resolved: facebook#45967 this was a meaningful exploration, but since we have support in TM for jsi runtime access and that's been widely advertised and accepted, let's get rid of this. it's a bit hacky and hard to use (shown by no one using it), so i want to stop any possibility of its usage. if interested in accessing jsi::Runtime via a native module, please consider the following options: - [Android] BindingsInstaller: facebook#44526 - [iOS] RCTTurboModuleWithJSIBindings: facebook#44486 - C++ TurboModules (no autolinking): https://github.com/reactwg/react-native-new-architecture/blob/main/docs/turbo-modules-xplat.md Reviewed By: christophpurrer Differential Revision: D61059182
|
This pull request was exported from Phabricator. Differential Revision: D61059182 |
philIip
force-pushed
the
export-D61059182
branch
from
befde9a
to
06ad6e0
Compare
|
|
This pull request has been merged in 184646e. |
|
This pull request was successfully merged by @philIip in 184646e When will my fix make it into a release? | How to file a pick request? |
|
For what it's worth, I actually liked the simplicity of this API as it didn't rely on any React Native auto-linking / code generation to "pick up" and call into it. All you needed was some C++ code calling into this function "on load". Looking at https://github.com/search?q=registerCxxModuleToGlobalModuleMap&type=code it also seems other notable packages were toying around with it) and I had a branch of Although I understand the desire to avoid fragmentation, I'm a bit sad to see it go 😞 |
What does this mean? There seems to be a couple of projects out in the wild that actually relies on this, like https://github.com/mrousavy/react-native-mmkv/blob/12c051704e0db363f9f63d2b30dbf291151f90d4/package/ios/MmkvOnLoad.mm#L18, cc @mrousavy), and I was going to make my own C++ TM using this method. Instead of using this, what would be the recommended approach? |
|
woah - I use this in a lot of libraries! I use this in react-native-mmkv, nitro (the upcoming "Nitro Modules" library), and a few others that fully rely on this. Any chance we can keep this? |
|
I need access to Also, Nitro is a CxxTurboModule (both on iOS and Android) that does not use CodeGen - this means that on Android, it is not built by CodeGen's CMake project, but instead built by it's own This would completely break that flow, and I don't think there's any alternatives?.. |
|
Feels like this could've been a discussion. Some developers were already building on top of this. For instance we had a C++ only TurboModule in the works for create-react-native-library. |
|
sorry all i apologize, i did not realize that there was actually interest in using this, i thought it was so unwieldy that no one would understand how to use it! but this is a good opportunity now to see if we can come up with a more stable replacement. the main reason i wanted to get rid of it is because it was extremely hacky, didn't work in it's original form without updating the compiler, and @mrousavy had to add that +load in order to get things to actually work. i don't think he even used the macro. (also apologies, i didn't know you actually committed to using this, thought it was just prototyping!). and i was skeptical it would lead us to a place where we would find a safer autolinking solution. since TM now has the installJSIBindings flow, i thought that we can use TM as a forwarding layer to a shared C++ layer which kinda can fulfill the role of reusing shared C++. but i understand that we can't leverage the lifecycle management of TM within a pure C++ environment. let me follow up to these comments to see if we can reintroduce this in a form that is more stable. |
thanks for surfacing this marc. i see the callsite in +load on iOS. how does the registration work on android? do you actually use the macro, or just the C++ methods like registerCxxModuleToGlobalModuleMap? how did you register in Android? |
yes, in hindsight that appears to be the case! i'm sorry for the churn. do you have examples of how you were using the API to register your modules? |
|
No worries thanks @philIip !! :)
Yea on iOS I use +load, but on Android I have to create a dummy Java package which does nothing except loading the C++ library. This calls JNI_OnLoad, which then calls the register... func, same way as on iOS. I'm not so sure about the installJSIBindings - does this also contain the CallInvoker? I'd need that too |
|
(it has to be a dummy Java package for autolinking to initialize it, which itself then initializes the C++ JNI code, which then calls register... TM) |
i see, so there's no usage of the RCT_EXPORT_CXX_MODULE_EXPERIMENTAL macro? that's the part i mainly found problematic, because it would often get stripped by the default compiler. i'd like to get rid of that, would that be okay? it shifts the burden of the actual link onto the library, but it seems like that's already the case for you. it might be a more sustainable solution since we don't have to burden the startup path with this, the library can decide on when is the best time to link. |
|
Yep the macro is not used! I can also create the revert PR with just the macro removed if that lifts some work off your shoulders! Thanks |
This reverts commit 184646e.
|
Created a PR here: #46360 |
…lobalModuleMap` (#46360) Summary: Reverts the PR #45967 from philIip to bring back the `registerCxxModuleToGlobalModuleMap(..)` function, which I use in Nitro Modules and MMKV. Ontop of that, this also removes the "experimental" `RCT_EXPORT_CXX_MODULE_EXPERIMENTAL` macro, which I think was the original intent of this PR as this macro is a bit unsafe. I also added some small docs to `registerCxxModuleToGlobalModuleMap` while I'm at it. ## Changelog: <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [GENERAL] [CHANGED] - Bring back CxxTurboModule autolinking function, but remove `RCT_EXPORT_CXX_MODULE_EXPERIMENTAL` macro [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: #46360 Test Plan: Build Nitro Modules. Worked for me! :) Reviewed By: realsoelynn Differential Revision: D62310637 Pulled By: philIip fbshipit-source-id: 2caa2b8ea094dda5e13c81431a9a645cbcf8f807
Summary:
Changelog: [iOS][Android][Breaking]
this was a meaningful exploration, but since we have support in TM for jsi runtime access and that's been widely advertised and accepted, let's get rid of this. it's a bit hacky and hard to use (shown by no one using it), so i want to stop any possibility of its usage.
if interested in accessing jsi::Runtime via a native module, please consider the following options: