feat: Add `CallInvoker` to `installJSIBindings(..)`/`BindingsInstaller` by mrousavy · Pull Request #46851 · facebook/react-native

mrousavy · 2024-10-05T12:07:15Z

Summary:

While the new installJSIBindings(..)/BindingsInstaller functionality allows you to synchronously set up stuff in the JS runtime before JS runs, there is no access to the JS CallInvoker, meaning you cannot really set up anything that uses callbacks or asynchronous code.

Nitro needs this.

Changelog:

[IOS] [ADDED] - Add CallInvoker to installJSIBindings(..)
[ANDROID] [ADDED] - Add CallInvoker to BindingsInstaller

Test Plan:

In SampleTurboModuleJSIBindings, we can now use the CallInvoker.

mrousavy · 2024-10-05T12:08:25Z

Note: This is technically not backwards compatible since it adds a new parameter to an existing function.

If anyone uses this, they need to update their method signature.

I am not aware of any public repos using installJSIBindings(...)/BindingsInstaller as it was just recently introduced, so I think it should be fine, but if you guys want I can also make this backwards compatible by leaving in both methods - one with and one without CallInvoker.

javache · 2024-10-07T09:51:09Z

Will import locally to test, but would be great to think about ways to make this backwards compatible.

facebook-github-bot · 2024-10-07T09:57:38Z

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

mrousavy · 2024-10-07T09:57:58Z

Build failed because I am not passing along the CallInvoker here:

react-native/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp

Line 461 in d0912ed

bindingsInstallFunc(runtime);

Is there any way to access the CallInvoker on a RuntimeScheduler?

mrousavy · 2024-10-07T09:58:13Z

Will import locally to test, but would be great to think about ways to make this backwards compatible.

Sure - I'll add both methods.

mrousavy · 2024-10-07T09:59:40Z

~~I guess I could create an instance of RuntimeSchedulerCallInvoker (with the existing RuntimeScheduler) just to pass to that function, not sure if it's a good idea to do that though?~~

EDIT: I did just that. Let me know if there is already an existing instance of CallInvoker somewhere in that location (ReactInstance.cpp:461), if not I think this is fine either way. The allocation is lightweight.

mrousavy · 2024-10-07T10:05:52Z

backwards compatibility might look a bit ugly

javache · 2024-10-07T10:33:47Z

backwards compatibility might look a bit ugly

That seems pretty reasonable to me. We add a little bit of overhead for the legacy path. Just make sure to document is a deprecated and clean it up after the next branch cut :)

javache · 2024-10-07T10:34:23Z

    timerManager_->attachGlobals(runtime);

-    bindingsInstallFunc(runtime);
+    auto callInvoker = std::make_shared<RuntimeSchedulerCallInvoker>(runtimeScheduler_);


We should able to get to the existing one and avoid allocating a new one.

How? Do you have any pointers for me?

I'll have a look. I assumed this would be accessible from Reactinstance, since C++ TurboModules get it, but I may be wrong.

…ule/ReactCommon/BindingsInstallerHolder.cpp Co-authored-by: Pieter De Baets <pieter.debaets@gmail.com>

…form/ios/ReactCommon/RCTTurboModuleWithJSIBindings.h Co-authored-by: Pieter De Baets <pieter.debaets@gmail.com>

javache · 2024-10-07T12:22:04Z

 #include <jsireact/JSIExecutor.h>
 #include <react/featureflags/ReactNativeFeatureFlags.h>
 #include <react/renderer/runtimescheduler/RuntimeSchedulerBinding.h>
+#include <react/renderer/RuntimeSchedulerCallInvoker.h>


Suggested change

#include <react/renderer/RuntimeSchedulerCallInvoker.h>

#include <react/renderer/runtimescheduler/RuntimeSchedulerCallInvoker.h>

javache · 2024-10-07T12:29:39Z

Actually, we should only support this for the TurboModule-based bindings installer - and TurboModuleManager already has the jsCallInvoker. I don't think you need any of the changes to ReactInstance.

mrousavy · 2024-10-07T14:18:43Z

Sorry, you're right I messed up my local files. I built this inside another react-native app and had a few patches that conflicted with eachother.

I now checked out this branch in rn-tester/ specifically and will fix the issues :)

mrousavy · 2024-10-07T15:01:01Z

  static auto constexpr kJavaDescriptor =
      "Lcom/facebook/react/turbomodule/core/interfaces/BindingsInstallerHolder;";
+  using BindingsInstallFunc = std::function<void(jsi::Runtime& runtime,
+                                                 std::shared_ptr<CallInvoker> callInvoker)>;


@javache should we make this const& as well?

philIip · 2024-10-08T05:38:22Z

    _turboModuleCache.insert({moduleName, turboModule});

-    if ([module respondsToSelector:@selector(installJSIBindingsWithRuntime:)]) {
+    if ([module respondsToSelector:@selector(installJSIBindingsWithRuntime:callInvoker:)]) {


so we do have an API to init the TM with the call invoker, i'm wondering if we can use this and then reference it in the implementation of installJSIBindingsWithRuntime:

https://github.com/facebook/react-native/blob/main/packages/react-native/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTTurboModuleManager.mm#L693-L697

I mean we don't have the jsi::Runtime in there where you linked - or are you saying a TurboModule can just fetch the CallInvoker themselves using the callInvoker property (if implemented)? I guess that's fine too, but I think this API is a bit more verbose

I think both of these options work, and perhaps for symmetry with Android, sharing the callInvoker here is nice too? But it would allow us to avoid a breaking API change here too, which is nice.

so we do have an API to init the TM with the call invoker, i'm wondering if we can use this and then reference it in the implementation of installJSIBindingsWithRuntime:

https://github.com/facebook/react-native/blob/main/packages/react-native/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTTurboModuleManager.mm#L693-L697

What's the difference between this codepath and the one in https://github.com/facebook/react-native/blob/main/packages/react-native/React/Base/RCTModuleData.mm#L206-L208 ? Legacy native modules vs TurboModules? Or old bridge vs bridgeless?

The one in RCTModuleData can not access jsi::Runtime - it both doesn't have a reference to it, and it isn't guaranteed to run on the proper Thread (legacy modules)

so we do have an API to init the TM with the call invoker, i'm wondering if we can use this and then reference it in the implementation of installJSIBindingsWithRuntime:
https://github.com/facebook/react-native/blob/main/packages/react-native/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTTurboModuleManager.mm#L693-L697

What's the difference between this codepath and the one in https://github.com/facebook/react-native/blob/main/packages/react-native/React/Base/RCTModuleData.mm#L206-L208 ? Legacy native modules vs TurboModules? Or old bridge vs bridgeless?

it's just a forward compatible API. so the idea is that you can change your code and not worry about legacy vs new arch, you will be guaranteed that your module has access to the callinvoker.

essentially in your module, you could have sth like:

- (void)installJSIBindingsWithRuntime:(jsi::Runtime &)runtime { auto callinvoker = self.callInvoker; // do something with runtime & callInvoker }

i do think having API symmetry would be nice though. i wonder if adding the CallInvoker to this argument will make the other ways of retrieving the callInvoker irrelevant.

philIip · 2024-10-08T05:48:51Z

      auto installer = getBindingsInstaller(moduleInstance);
      if (installer) {
-        installer->cthis()->installBindings(runtime);
+        installer->cthis()->installBindings(runtime, jsCallInvoker_);


we can actually get the callInvoker off the ReactContext, but i can see how that's inapplicable here... but i'm reading all of this and i'm starting to feel like a C++ TM is more appropriate here? what's the current limitation?

I can't use a C++ TM because

I need backwards compatibility for now (I guess this is only a temporary thing)

I need the ReactApplicationContext. As far as I know, there's no way to get that in a C++ TM (since those are platform-independent)

I need it in NitroModules.kt (init) - this will (optionally) be passed along to other Nitro Modules. But it goes through C++ first.

I need it in NitroModules.kt (init) - this will (optionally) be passed along to other Nitro Modules. But it goes through C++ first.

thanks! is the idea that we want to replace install with installJSIBindings?

if so, is it possible to keep the current native pipeline to pass down the CallInvoker (i.e. do what you have now), but use installJSIBindings for the runtime installation? or will there be some sort of timing issue?

i can see how ergonomically that might feel weird, what's your feeling on that?

Yep - I want to have a clear, public and guaranteed to be future-proof API to install Nitro into the jsi::Runtime.
The current approach works, but having to annotate with @OptIn(FrameworkAPI::class) doesn't sound very public-API-like to me?

if so, is it possible to keep the current native pipeline to pass down the CallInvoker (i.e. do what you have now), but use installJSIBindings for the runtime installation? or will there be some sort of timing issue?

Well installJSIBindings (or BindingsInstaller on Android) is accessed before I can call install() on the module. So I don't have a CallInvoker yet at that point on Android.

facebook-github-bot · 2024-10-08T10:42:02Z

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

facebook-github-bot · 2024-10-09T11:39:58Z

@javache merged this pull request in 87bae7f.

react-native-bot · 2024-10-09T11:40:02Z

This pull request was successfully merged by @mrousavy in 87bae7f

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

mrousavy · 2024-10-09T12:44:43Z

thanks guys!! 🖤

feat: Add CallInvoker to Bindings Installer

faef623

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 Oct 5, 2024

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 Oct 5, 2024

mrousavy added 3 commits October 5, 2024 14:33

fix: Fix missing args

ebaf77f

fix: Fix copy-ctor

ade790b

Update RCTTurboModuleWithJSIBindings.h

085d7fe

fix: Create CallInvoker for Bindings Installer

286157d

javache requested changes Oct 7, 2024

View reviewed changes

mrousavy and others added 2 commits October 7, 2024 13:46

Update packages/react-native/ReactAndroid/src/main/jni/react/turbomod…

a8beaa4

…ule/ReactCommon/BindingsInstallerHolder.cpp Co-authored-by: Pieter De Baets <pieter.debaets@gmail.com>

Update packages/react-native/ReactCommon/react/nativemodule/core/plat…

086d241

…form/ios/ReactCommon/RCTTurboModuleWithJSIBindings.h Co-authored-by: Pieter De Baets <pieter.debaets@gmail.com>

javache reviewed Oct 7, 2024

View reviewed changes

mrousavy added 6 commits October 7, 2024 16:19

fix: Remove wrong call to bindingsInstallFunc in ReactInstance

430a442

fix: Fix usage in RCTSampleTurboModule.mm

e3202e7

feat: Make installJSIBindingsWithRuntime: on iOS backwards compatible

760e1fc

fix: Make const&

ab2a762

feat: Make backwards compatible for Android

a1b2d82

fix: Revert changes in JReactInstance

6e1f078

mrousavy commented Oct 7, 2024

View reviewed changes

philIip reviewed Oct 8, 2024

View reviewed changes

facebook-github-bot closed this in 87bae7f Oct 9, 2024

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

	#include <react/renderer/RuntimeSchedulerCallInvoker.h>
	#include <react/renderer/runtimescheduler/RuntimeSchedulerCallInvoker.h>

Conversation

mrousavy commented Oct 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary:

Changelog:

Test Plan:

Uh oh!

mrousavy commented Oct 5, 2024

Uh oh!

javache commented Oct 7, 2024

Uh oh!

facebook-github-bot commented Oct 7, 2024

Uh oh!

mrousavy commented Oct 7, 2024

Uh oh!

mrousavy commented Oct 7, 2024

Uh oh!

mrousavy commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrousavy commented Oct 7, 2024

Uh oh!

javache commented Oct 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javache commented Oct 7, 2024

Uh oh!

mrousavy commented Oct 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philIip Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 8, 2024

Uh oh!

facebook-github-bot commented Oct 9, 2024

Uh oh!

react-native-bot commented Oct 9, 2024

Uh oh!

mrousavy commented Oct 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

mrousavy commented Oct 5, 2024 •

edited

Loading

mrousavy commented Oct 7, 2024 •

edited

Loading

philIip Oct 8, 2024 •

edited

Loading