Skip to content

RJS-2815: React Native bridgeless support (take 2)#6737

Merged
kraenhansen merged 14 commits intomainfrom
kh/rn-bridgeless-2
Jul 16, 2024
Merged

RJS-2815: React Native bridgeless support (take 2)#6737
kraenhansen merged 14 commits intomainfrom
kh/rn-bridgeless-2

Conversation

@kraenhansen
Copy link
Copy Markdown
Contributor

@kraenhansen kraenhansen commented Jun 18, 2024

What, How & Why?

This closes #6653 by introducing a new implementations of the way we register our native module on both iOS and Android. I suggest relying on the legacy NativeModules compatibility layer for now, since the current state of pure C++ TurboModules are largely undocumented and experimental: We could revisit this decision in the future by introducing an alternative implementation, conditional on the new architecture being enabled in the app.

This approach aligns with others vendors having exposing their native C++ / Rust based native libraries through JSI.

This still injects the binding into the JS global, from where the "JS wrapper" of our native module is able to read and delete it again. With these changes, we're now deferring the injection of this (and the loading of our .so on Android) until the very last moment it's needed, essentially getting the benefits of deferred loading that TurboModules bring:

// Calling loadLibrary multiple times will be ignored
// We do it here instead of statically to allow the app load faster if Realm isn't accessed.
// Effectively emulating the behaviour of TurboModules.
SoLoader.loadLibrary("realm");

Since we're implementing and injecting our binding onto the a global using JSI, we're not relying on the legacy bridge compatibility layer for every call from JS into C++ and I therefore expect this to be as fast as a TurboModule would be.

I did attempt to inject the "JS call invoker" for the "UI flush workaround" only if bridgeless is disabled, as facebook/react-native#43396 fixed this for bridgeless and we don't want to pay the cost if it isn't needed, but it didn't work. We could still experiment with using the JSI Runtime::drainMicrotasks() API if available.

☑️ ToDos

  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 📝 Update COMPATIBILITY.md
  • 🚦 Tests
  • 📦 Updated internal package version in consuming package.jsons (if updating internal packages)
  • 📱 Check the React Native/other sample apps work if necessary
  • 💥 Breaking label has been applied or is not necessary

@kraenhansen kraenhansen self-assigned this Jun 18, 2024
@cla-bot cla-bot bot added the cla: yes label Jun 18, 2024
@kraenhansen kraenhansen force-pushed the kh/rn-bridgeless-2 branch 3 times, most recently from e448000 to 1b09689 Compare June 24, 2024 09:58
// Start the log cat (skipping any initial pid from an old run)
if (spawnLogcat) {
logcat.start("com.realmreactnativetests", true).catch(console.error);
logcat.start("com.microsoft.reacttestapp", true).catch(console.error);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically no longer needed, but it does fix an issue of spawning logcat when running locally.

@@ -1,45 +0,0 @@
/* DO NOT EDIT THIS FILE - it is machine generated */
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is not needed as no code is referring to these symbols externally.


#include <ReactCommon/CallInvoker.h>

namespace realm::js::flush_ui_workaround {
Copy link
Copy Markdown
Contributor Author

@kraenhansen kraenhansen Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As part of #6718 I did a refactor of the "flush UI workaround" which I decided to keep in this PR, since I felt it was a nice increase in the separation of concerns.

@kraenhansen kraenhansen marked this pull request as ready for review July 15, 2024 15:02
Copy link
Copy Markdown
Contributor

@kneth kneth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comments are not blocking but merely suggestions and questions to clarify a few things (for me)


s.source_files = 'binding/jsi/*.cpp',
'binding/apple/*.mm'
'binding/apple/platform.mm',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the any reason for not using all .mm files?

Copy link
Copy Markdown
Contributor Author

@kraenhansen kraenhansen Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. I don't remember why I changed this. I've updated this to be 'binding/apple/*.mm' again.

{
__android_log_print(ANDROID_LOG_VERBOSE, "JSRealm", "setDefaultRealmFileDirectory");

__android_log_print(ANDROID_LOG_VERBOSE, "Realm", "install");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use Core's logger instead?

Copy link
Copy Markdown
Contributor Author

@kraenhansen kraenhansen Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Core isn't necessarily loaded into memory at the point of calling code in this file, so I'd rather - not to keep things simple 🙂

// Calling loadLibrary multiple times will be ignored
// We do it here instead of statically to allow the app load faster if Realm isn't accessed.
// Effectively emulating the behaviour of TurboModules.
SoLoader.loadLibrary("realm");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we wrap it in a try/catch to provide a better error mesage?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine the SoLoader will provide sufficient information - we can add to this if we see common patterns emerge, but right now I don't know what I would write to make it more actionable for developers 🤔 It used to be the case that people forgot to react-native link but that's a thing of the past.

realm::js::flush_ui_workaround::reset_js_call_invoker();
__android_log_print(ANDROID_LOG_VERBOSE, "Realm", "Invalidating caches");
#if DEBUG
realm_jsi_close_sync_sessions();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please include the comments as in RealmReactModule.m

@kraenhansen kraenhansen changed the title React Native bridgeless support (take 2) RJS-2815: React Native bridgeless support (take 2) Jul 16, 2024
@kraenhansen
Copy link
Copy Markdown
Contributor Author

kraenhansen commented Jul 16, 2024

I included a fix with acb518d solving #6787 in the process.

@ferrannp
Copy link
Copy Markdown

I have still not being able to migrate my project to new architecture but super happy to see this ongoing. Just writing to say thank you @kraenhansen !

@kraenhansen
Copy link
Copy Markdown
Contributor Author

kraenhansen commented Aug 16, 2024

@ferrannp Honestly, a comment like that means more to us than you might know 🙂 Thank you!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support "bridgeless" mode by exposing a TurboModule on iOS

3 participants