Skip to content

Add BindingsInstaller for TurboModules on iOS#44486

Closed
Kudo wants to merge 4 commits into
facebook:mainfrom
Kudo:@kudo/turbo-modules-bindings-installer
Closed

Add BindingsInstaller for TurboModules on iOS#44486
Kudo wants to merge 4 commits into
facebook:mainfrom
Kudo:@kudo/turbo-modules-bindings-installer

Conversation

@Kudo
Copy link
Copy Markdown
Contributor

@Kudo Kudo commented May 8, 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

Test Plan:

Added test in RN-Tester TurboModule test case

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 8, 2024
@analysis-bot
Copy link
Copy Markdown

analysis-bot commented May 8, 2024

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 19,495,560 -45,615
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 22,867,614 -44,415
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: 26fc40d
Branch: main

@Kudo Kudo marked this pull request as ready for review May 8, 2024 14:14
@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 May 8, 2024
@RSNara
Copy link
Copy Markdown
Contributor

RSNara commented May 8, 2024

@Kudo, I think there might be another way to accomplish this.

What if we could initialize c++ modules with (application or react native)-provided java/objc dependencies? Would that be enough to fulfill your use-case? Also, to confirm, is this your use-case: We need to expose a jsi::HostFunction to javascript, that allows us to reach into java objects, and do java things, right?

There is a way to extend the C++ turbomodule infra, like how I described:

  1. Everyone registers their C++ turbomodules using CxxReactPackages (java <-> c++ hybrid objects).
  2. We extend auto-linking to pick up, coalesce, and register CxxReactPackages (like how it does ReactPackages).

This makes registering c++ only modules a bit harder. Right now, you just drop them into your code-base. But, c++ modules are already an advanced feature, that we only expect advanced users to use. So, maybe it's fine.

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.

i think we have to be careful on this one, if requiresMainQueueSetup is true, then this might not be safe. we can use the callInvoker but that won't block. we can also document and just say this API is incompatible with main thread set up modules. what do you think?

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 this is probably the thing i'm the most iffy about. the thing is in the new arch code is that we never capture the runtime pointer, and this now will break that paradigm. i realized looking at your PR that android can avoid this bc they actually do their installation via runtime executor

instead of capturing this, what about passing it down through provideTurboModule? that flow is triggered via installJSBindings. that way we don't have to have an additional strong reference to the runtime here

the thing is, the callstack for _createAndSetupObjCModule is kinda complex. but i looked into this - i think ultimately at every path, we have thread safe access to the runtime pointer.

image

in _invalidateModules, we only initialize the module to finish any side effects, i believe to avoid some races with invalidation. this seems like a pretty extreme edge case?

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.

good catching @philIip! i've passed runtime to provideTurboModule: without storing the runtime in the class. also moving the installation earlier and right after getTurboModule:. that's a point to ensure running on js thread, even requiresMainQueueSetup is true.

aside from that, i think on ios and android implementation now still passing runtime pointer (android / ios). do you think whether this would be a concern? at least at provideTurboModule: and createTurboModuleProvider(), this is when people call TurboModuleRegistry.get() from js, so runtime should be valid at this point.
we could maybe turn RuntimeExecutor into BindingsInstaller without passing runtime, but we would at least need an UnbufferedRuntimeExecutor. Otherwise it may be too late to install the js bindings.

@philIip
Copy link
Copy Markdown
Contributor

philIip commented May 9, 2024

@Kudo, I think there might be another way to accomplish this.

What if we could initialize c++ modules with (application or react native)-provided java/objc dependencies? Would that be enough to fulfill your use-case? Also, to confirm, is this your use-case: We need to expose a jsi::HostFunction to javascript, that allows us to reach into java objects, and do java things, right?

There is a way to extend the C++ turbomodule infra, like how I described:

  1. Everyone registers their C++ turbomodules using CxxReactPackages (java <-> c++ hybrid objects).
  2. We extend auto-linking to pick up, coalesce, and register CxxReactPackages (like how it does ReactPackages).

This makes registering c++ only modules a bit harder. Right now, you just drop them into your code-base. But, c++ modules are already an advanced feature, that we only expect advanced users to use. So, maybe it's fine.

i think everyone is aligned with you on this being a nice north star for C++ TM, but this native hook can be seen as an intermediate step. and if implemented properly, this native hook should add minimal surface to our API surface area, and as u said it's an advanced use case - so is it worth all of the effort to build autolinking, etc. for < 10 libs? not sure yet

i also responded to ur internal WP comment ! let's chat further if needed :)

@Kudo Kudo changed the title Add BindingsInstaller for TurboModules Add BindingsInstaller for TurboModules on iOS May 10, 2024
@Kudo Kudo force-pushed the @kudo/turbo-modules-bindings-installer branch from a7b7bf6 to f67a9cf Compare May 10, 2024 16:04
@Kudo Kudo requested a review from philIip May 10, 2024 16:08
@Kudo
Copy link
Copy Markdown
Contributor Author

Kudo commented May 10, 2024

apply review comments to the pr and split android implementation to #44526 as requested.
though GH doesn't support stacked prs, the two commits are cloned:

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@philIip has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label May 30, 2024
@facebook-github-bot
Copy link
Copy Markdown
Contributor

@philIip merged this pull request in bed487e.

@github-actions
Copy link
Copy Markdown

This pull request was successfully merged by @Kudo in bed487e.

When will my fix make it into a release? | How to file a pick request?

facebook-github-bot pushed a commit that referenced this pull request May 31, 2024
Summary:
Add synchronous JS bindings installation for TurboModules. That would help some 3rd party JSI based modules to install JS bindings easier.
#44486 for Android

## Changelog:

[Android] [ADDED] - Add BindingsInstaller for TurboModules

Pull Request resolved: #44526

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

 {F1660267530}

 {F1660287029}

Reviewed By: javache

Differential Revision: D57223328

Pulled By: philIip

fbshipit-source-id: d4a69a16f6ce77c0a0fd63f008bea929b1964ab8
@Kudo Kudo deleted the @kudo/turbo-modules-bindings-installer branch June 3, 2024 17:12
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
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.
facebook#44486 for Android

## Changelog:

[Android] [ADDED] - Add BindingsInstaller for TurboModules

Pull Request resolved: facebook#44526

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

 {F1660267530}

 {F1660287029}

Reviewed By: javache

Differential Revision: D57223328

Pulled By: philIip

fbshipit-source-id: d4a69a16f6ce77c0a0fd63f008bea929b1964ab8
philIip added a commit to philIip/react-native that referenced this pull request Aug 12, 2024
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 pushed a commit that referenced this pull request Aug 12, 2024
Summary:
Changelog: [iOS][Android][Breaking]

Pull Request resolved: #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: #44526
- [iOS] RCTTurboModuleWithJSIBindings: #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

fbshipit-source-id: da5d74e2b6161ea7e8dd5f664ae0eb927bb1e2c3
philIip added a commit to philIip/react-native that referenced this pull request Sep 12, 2024
Summary:
We should use [BindingsInstaller](facebook#44486) and [RCTCallInvoker](facebook#44378) instead.

## Changelog:
[iOS][Breaking] - We deprecated RCTRuntimeExecutor in 0.76, so let's get rid of the code now.

Reviewed By: cipolleschi

Differential Revision: D62469005
facebook-github-bot pushed a commit that referenced this pull request Sep 12, 2024
Summary:
Pull Request resolved: #46466

We should use [BindingsInstaller](#44486) and [RCTCallInvoker](#44378) instead.

## Changelog:
[iOS][Breaking] - We deprecated RCTRuntimeExecutor in 0.76, so let's get rid of the code now.

Reviewed By: cipolleschi

Differential Revision: D62469005

fbshipit-source-id: 3ed033b08f52823bd42f8a57d5d6ab40e35d30f1
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. Merged This PR has been merged. 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.

5 participants