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

Adds option for non-ABI TurboModule provider #12383

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rozele
Copy link
Collaborator

@rozele rozele commented Nov 13, 2023

Description

Type of Change

Erase all that don't apply.

  • New feature (non-breaking change which adds functionality)

Why

For react-native-windows apps that bypass the JsiAbiApi when initializing a JS runtime, it would be useful to use the same module provider infrastructure used for other modules in react-native-windows.

Today the only two options are:

  1. Use attributed native modules
  2. Use C++ TurboModules wrapped in JsiHostObjectWrappers

What

This adds a third option:
3. Use C++ TurboModules wrapped in an IJsiNonAbiHostObject

The main reason for creating IJsiNonAbiHostObject and not repurposing IReactNonAbiValue is that there is already a baked in assumption in the TurboModule provider that assumes all IReactNonAbiValues are attributed native modules, so a new interface is needed.

Testing

Integrated this change into our React Native Windows app, and we were able to use IJsiNonAbiTurboModule instances.

Changelog

Should this change be included in the release notes: no

Microsoft Reviewers: Open in CodeFlow

@rozele rozele force-pushed the nonAbiTurboModuleProvider branch 3 times, most recently from 40d3f4c to 68dc99a Compare November 15, 2023 18:28
@rozele rozele marked this pull request as ready for review November 15, 2023 18:29
@rozele rozele requested review from a team as code owners November 15, 2023 18:29
Comment on lines 373 to +426
std::shared_ptr<implementation::HostObjectWrapper> m_hostObjectWrapper;
std::shared_ptr<facebook::jsi::HostObject> m_nonAbiHostObject;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Up for debate, but we could presumably just have one shared_ptr here, since implementation::HostObjectWrapper derives from facebook::jsi::HostObject

"alternate interface to type check against.")
interface IJsiNonAbiHostObject {
DOC_STRING("Gets a value indicating whether the object has a value. This is just a dummy method to avoid issues with empty interfaces in C++/WinRT.")
Boolean HasValue { get; };
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@acoates-ms do you have any pointers to where I should add a GUID to avoid this dummy property?

Also, do you have any suggestions for how we can shove a C++ TurboModule into an IInspectable (to match the IReactModuleBuilder delegate type) and be able to query some interface to decide that it's actually a JsiNonAbiHostObject? If so, we won't need this IDL. Bear in mind that IReactNonAbiValue is already "taken" for NM2 modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

For react-native-windows apps that bypass the JsiAbiApi when
initializing a JS runtime, it would be useful to use the same module
provider infrastructure used for other modules in react-native-windows.

Today the only two options are:
1. Use attributed native modules
2. Use C++ TurboModules wrapped in JsiHostObjectWrappers

This adds a third option:
3. Use C++ TurboModules wrapped in an IJsiNonAbiHostObject

The main reason for creating IJsiNonAbiHostObject and not repurposing
IReactNonAbiValue is that there is already a baked in assumption in the
TurboModule provider that assumes all IReactNonAbiValues are attributed
native modules, so a new interface is needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants