remove cxx TM autolinking #45967

philIip · 2024-08-10T04:32:29Z

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:

[Android] BindingsInstaller: Add BindingsInstaller for TurboModules on Android #44526
[iOS] RCTTurboModuleWithJSIBindings: Add BindingsInstaller for TurboModules on iOS #44486
C++ TurboModules (no autolinking): https://github.com/reactwg/react-native-new-architecture/blob/main/docs/turbo-modules-xplat.md

facebook-github-bot · 2024-08-10T04:32:53Z

This pull request was exported from Phabricator. Differential Revision: D61059182

facebook-github-bot · 2024-08-12T20:25:59Z

This pull request was exported from Phabricator. Differential Revision: D61059182

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

react-native-bot · 2024-08-12T20:26:58Z

	Warnings
⚠️	📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.
⚠️	📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.

Generated by 🚫 dangerJS against befde9a

react-native-bot · 2024-08-12T20:27:27Z

	Warnings
⚠️	📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.
⚠️	📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.

Generated by 🚫 dangerJS against befde9a

react-native-bot · 2024-08-12T20:27:35Z

	Warnings
⚠️	📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.
⚠️	📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.

Generated by 🚫 dangerJS against befde9a

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

facebook-github-bot · 2024-08-12T20:30:58Z

This pull request was exported from Phabricator. Differential Revision: D61059182

react-native-bot · 2024-08-12T20:31:56Z

	Warnings
⚠️	📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.
⚠️	📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.

Generated by 🚫 dangerJS against 06ad6e0

facebook-github-bot · 2024-08-12T22:33:50Z

This pull request has been merged in 184646e.

react-native-bot · 2024-08-12T22:34:06Z

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?}

kraenhansen · 2024-08-28T14:58:13Z

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 realm where I was also contemplating using it.

Although I understand the desire to avoid fragmentation, I'm a bit sad to see it go 😞

hsjoberg · 2024-08-30T16:09:30Z

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.

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?

mrousavy · 2024-08-30T16:11:13Z

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?

mrousavy · 2024-08-30T16:12:58Z

I need access to jsi::Runtime and std::shared_ptr<CallInvoker> in my modules. I don't think the other APIs you suggested have this feature.

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 build.gradle's externalNativeBuild spec.

This would completely break that flow, and I don't think there's any alternatives?..

atlj · 2024-09-04T13:25:12Z

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.

philIip · 2024-09-04T18:31:23Z

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.

philIip · 2024-09-04T18:35:43Z

I need access to jsi::Runtime and std::shared_ptr<CallInvoker> in my modules. I don't think the other APIs you suggested have this feature.

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 build.gradle's externalNativeBuild spec.

This would completely break that flow, and I don't think there's any alternatives?..

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?

philIip · 2024-09-04T18:36:35Z

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.

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?

mrousavy · 2024-09-05T07:34:02Z

No worries thanks @philIip !! :)

do you actually use the macro, or just the C++ methods like registerCxxModuleToGlobalModuleMap? how did you register in Android?

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

mrousavy · 2024-09-05T07:34:36Z

(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)

philIip · 2024-09-06T05:45:10Z

No worries thanks @philIip !! :)

do you actually use the macro, or just the C++ methods like registerCxxModuleToGlobalModuleMap? how did you register in Android?

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

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.

mrousavy · 2024-09-06T08:15:19Z

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.

mrousavy · 2024-09-06T08:21:35Z

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

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: Facebook Partner: Facebook Partner labels Aug 10, 2024

facebook-github-bot added the fb-exported label Aug 10, 2024

philIip force-pushed the export-D61059182 branch from 5e6b051 to befde9a Compare August 12, 2024 20:26

philIip force-pushed the export-D61059182 branch from befde9a to 06ad6e0 Compare August 12, 2024 20:31

facebook-github-bot closed this in 184646e Aug 12, 2024

facebook-github-bot added the Merged This PR has been merged. label Aug 12, 2024

mrousavy added a commit to mrousavy/react-native that referenced this pull request Sep 6, 2024

Revert "remove cxx TM autolinking (facebook#45967)"

337225e

This reverts commit 184646e.

mrousavy mentioned this pull request Sep 6, 2024

fix: Bring back Cxx TurboModule autolinking via registerCxxModuleToGlobalModuleMap #46360

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove cxx TM autolinking #45967

remove cxx TM autolinking #45967

philIip commented Aug 10, 2024 •

edited

Loading

facebook-github-bot commented Aug 10, 2024

facebook-github-bot commented Aug 12, 2024

react-native-bot commented Aug 12, 2024

react-native-bot commented Aug 12, 2024

react-native-bot commented Aug 12, 2024

facebook-github-bot commented Aug 12, 2024

react-native-bot commented Aug 12, 2024

facebook-github-bot commented Aug 12, 2024

react-native-bot commented Aug 12, 2024

kraenhansen commented Aug 28, 2024

hsjoberg commented Aug 30, 2024

mrousavy commented Aug 30, 2024

mrousavy commented Aug 30, 2024 •

edited

Loading

atlj commented Sep 4, 2024

philIip commented Sep 4, 2024 •

edited

Loading

philIip commented Sep 4, 2024 •

edited

Loading

philIip commented Sep 4, 2024

mrousavy commented Sep 5, 2024

mrousavy commented Sep 5, 2024 •

edited

Loading

philIip commented Sep 6, 2024 •

edited

Loading

mrousavy commented Sep 6, 2024

mrousavy commented Sep 6, 2024

remove cxx TM autolinking #45967

remove cxx TM autolinking #45967

Conversation

philIip commented Aug 10, 2024 • edited Loading

facebook-github-bot commented Aug 10, 2024

facebook-github-bot commented Aug 12, 2024

react-native-bot commented Aug 12, 2024

react-native-bot commented Aug 12, 2024

react-native-bot commented Aug 12, 2024

facebook-github-bot commented Aug 12, 2024

react-native-bot commented Aug 12, 2024

facebook-github-bot commented Aug 12, 2024

react-native-bot commented Aug 12, 2024

kraenhansen commented Aug 28, 2024

hsjoberg commented Aug 30, 2024

mrousavy commented Aug 30, 2024

mrousavy commented Aug 30, 2024 • edited Loading

atlj commented Sep 4, 2024

philIip commented Sep 4, 2024 • edited Loading

philIip commented Sep 4, 2024 • edited Loading

philIip commented Sep 4, 2024

mrousavy commented Sep 5, 2024

mrousavy commented Sep 5, 2024 • edited Loading

philIip commented Sep 6, 2024 • edited Loading

mrousavy commented Sep 6, 2024

mrousavy commented Sep 6, 2024

philIip commented Aug 10, 2024 •

edited

Loading

mrousavy commented Aug 30, 2024 •

edited

Loading

philIip commented Sep 4, 2024 •

edited

Loading

philIip commented Sep 4, 2024 •

edited

Loading

mrousavy commented Sep 5, 2024 •

edited

Loading

philIip commented Sep 6, 2024 •

edited

Loading