Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass RuntimeSchedulerCallInvoker to C++ TurboModule initialization #12826

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rozele
Copy link
Collaborator

@rozele rozele commented Mar 12, 2024

Description

Type of Change

Erase all that don't apply.

  • Bug fix (non-breaking change which fixes an issue)

Why

Rather than creating a CallInvoker that posts to the JSDispatcher, this passes the same CallInvoker used for core TurboModules to 3rd party TurboModules and dispatches actions using the RuntimeScheduler if enabled.

Changelog

Should this change be included in the release notes: no

Microsoft Reviewers: Open in CodeFlow

Rather than creating a CallInvoker that posts to the JSDispatcher, this
passes the same CallInvoker used for core TurboModules to 3rd party
TurboModules and dispatches actions using the RuntimeScheduler if
enabled.
@rozele rozele requested a review from a team as a code owner March 12, 2024 17:22
@acoates-ms
Copy link
Contributor

This breaks our ABI. CallInvoker by itself is not ABI safe. The implementation internal to the MS.RN.dll accesses all kinds of non-abi methods. The implementation used in TurboModules is provided outside the DLL and is a separate implementation which only calls ABI safe methods that are exported from the dll.

Copy link
Contributor

@acoates-ms acoates-ms left a comment

Choose a reason for hiding this comment

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

Since this breaks the ABI, I doubt we can take this change.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) label Mar 28, 2024
@rozele
Copy link
Collaborator Author

rozele commented Mar 29, 2024

@acoates-ms Out of curiosity, what is your plan for passing the runtime to C++ TurboModules?

I expect core React Native modules to start using this runtime argument in CallInvoker to invoke emitDeviceEvent. For reference, this is the mobile React Native change that will expose the jsi::Runtime argument in CallInvoker: facebook/react-native#43375. I don't think you've sync'd past this change yet.

I suppose you'll have to put the JsiRuntimeHolder somewhere accessible to the TurboModuleProvider, and pull the runtime off the JsiRuntimeHolder.

For non-ABI JSI use cases like we have in our apps at Meta, perhaps we could fallback on the RuntimeSchedulerCallInvoker (if the JsiRuntimeHolder is null or whatever)?

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 An issue that had been tagged "Needs: Author Feedback" has received activity (label applied by bot) and removed Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) labels Mar 29, 2024
@rozele rozele marked this pull request as draft March 29, 2024 16:17
@jonthysell
Copy link
Contributor

@acoates-ms Any update on this? Can we just close it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Attention 👋 An issue that had been tagged "Needs: Author Feedback" has received activity (label applied by bot)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants