Skip to content

Add installJSIBindings for turbo modules on android#43110

Closed
Kudo wants to merge 3 commits into
facebook:mainfrom
Kudo:@kudo/turbo-module-install-jsi-android
Closed

Add installJSIBindings for turbo modules on android#43110
Kudo wants to merge 3 commits into
facebook:mainfrom
Kudo:@kudo/turbo-module-install-jsi-android

Conversation

@Kudo
Copy link
Copy Markdown
Contributor

@Kudo Kudo commented Feb 20, 2024

Summary:

Add synchronous JSI installation for TurboModules on Android. That would help some 3rd party JSI based modules to get the jsi::Runtime on bridgeless mode.
The iOS implementation will be in a separate PR.

Changelog:

[ANDROID] [ADDED] - Add installJSIBindings for TurboModules

Test Plan:

Added test in RN-Tester TurboModule test case

@facebook-github-bot 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. Contributor A React Native contributor. p: Expo Partner: Expo Partner labels Feb 20, 2024
@analysis-bot
Copy link
Copy Markdown

analysis-bot commented Feb 20, 2024

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 17,959,791 +24,725
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 21,317,968 +24,687
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: 7fffe69
Branch: main

bool enableSyncVoidMethods) {
jsi::Runtime *runtime, bool enableSyncVoidMethods) {
return [turboModuleCache_ = std::weak_ptr<ModuleCache>(turboModuleCache_),
runtime,
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.

the runtime could be a dangling pointer in theory. but the lambda would run on js thread and there's the check. so it should not hit to handling pointer crash.

    if (!turboModuleCache || !jsCallInvoker || !nativeMethodCallInvoker ||
        !delegate || !javaPart) {
      return nullptr;
    }

@Kudo Kudo marked this pull request as ready for review February 20, 2024 15:05
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Feb 20, 2024
Comment on lines +252 to +254
public void installJSIBindings(long runtime) {
installJSIBindingsCxx(runtime);
}
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.

so it feels like one motivation of having this live in native land is that we might have some native dependencies that we want to pass down the the runtime bindings. if that's the case, i feel like this will be called before we could do anything to manipulate the native state of the module, or do you see it differently?

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.

if that's the case, i feel like this will be called before we could do anything to manipulate the native state of the module

from js side, yes, and that's why we need a synchronous way to install bindings right after initiating the module and before the further js call. i.e.

const module = TurboModuleRegistry.getEnforcing<Spec>('Module');
// globalThis.someJSIBindings should be available here
module.call();

* @param runtime The (facebook::jsi::Runtime *) pointer casted to Long type.
*/
@DoNotStrip
public fun installJSIBindings(runtime: Long)
Copy link
Copy Markdown
Contributor

@philIip philIip Feb 21, 2024

Choose a reason for hiding this comment

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

implementation detail, but i think it would be nice if we could have a compile time check here to fail if there's no JNI method here... they have to write C++ anyways to handle the runtime anyways if they're using this API, and maybe we could also obfuscate the pointer cast from them. i am a kotlin noob so not sure if that's possible here

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.

originally i was thinking to introduce the interface as an abstract class and having fbjni c++ hybrid data. we could scaffold more c++ stuff there. ultimately i thought that abstract class might not be a good idea since some native module might need their dedicated base class.

unfortunately kotlin seems not support interface with public external fun, otherwise it helps to indicate the method should be a jni call.

@Kudo
Copy link
Copy Markdown
Contributor Author

Kudo commented Feb 28, 2024

close and looking forward to seeing alternative solutions in the future

@Kudo Kudo closed this Feb 28, 2024
@Kudo Kudo deleted the @kudo/turbo-module-install-jsi-android branch February 28, 2024 17:08
facebook-github-bot pushed a commit that referenced this pull request May 30, 2024
Summary:
Add synchronous JS bindings installation for TurboModules. That would help some 3rd party JSI based modules to install JS bindings easier.
Re-create from #43110 but for iOS

## Changelog:

[IOS] [ADDED] - Add BindingsInstaller for TurboModules

Pull Request resolved: #44486

Test Plan: Added test in RN-Tester TurboModule test case

Reviewed By: javache

Differential Revision: D57224891

Pulled By: philIip

fbshipit-source-id: fabe5c4f8d2087ac9a465f2cb90d884b83265a68
kosmydel pushed a commit to kosmydel/react-native that referenced this pull request Jun 11, 2024
Summary:
Add synchronous JS bindings installation for TurboModules. That would help some 3rd party JSI based modules to install JS bindings easier.
Re-create from facebook#43110 but for iOS

## Changelog:

[IOS] [ADDED] - Add BindingsInstaller for TurboModules

Pull Request resolved: facebook#44486

Test Plan: Added test in RN-Tester TurboModule test case

Reviewed By: javache

Differential Revision: D57224891

Pulled By: philIip

fbshipit-source-id: fabe5c4f8d2087ac9a465f2cb90d884b83265a68
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. p: Expo Partner: Expo Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants