Skip to content

Make TurboModule system support int/float args/returns#36628

Closed
RSNara wants to merge 3 commits into
facebook:mainfrom
RSNara:export-D44000389
Closed

Make TurboModule system support int/float args/returns#36628
RSNara wants to merge 3 commits into
facebook:mainfrom
RSNara:export-D44000389

Conversation

@RSNara
Copy link
Copy Markdown
Contributor

@RSNara RSNara commented Mar 24, 2023

Summary:
The legacy NativeModule system supports integer and float in NativeModule method arguments and returns. This diff extends the TurboModule system for the same functionality. T

his is necessary because the TurboModule system will now need to dispatch method calls to legacy NativeModules.

NOTE: We can't actually test these changes until we run interop modules.

Changelog: [Internal]

Reviewed By: javache

Differential Revision: D44000389

@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. p: Facebook Partner: Facebook Partner fb-exported labels Mar 24, 2023
@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D44000389

@analysis-bot
Copy link
Copy Markdown

analysis-bot commented Mar 24, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,529,273 +9,701
android hermes armeabi-v7a 7,844,900 +9,122
android hermes x86 9,009,266 +10,538
android hermes x86_64 8,864,936 +10,037
android jsc arm64-v8a 9,150,563 +9,702
android jsc armeabi-v7a 8,342,123 +9,123
android jsc x86 9,205,098 +10,551
android jsc x86_64 9,463,671 +10,031

Base commit: 4e0dfed
Branch: main

@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D44000389

@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D44000389

Copy link
Copy Markdown
Contributor

@Pranav-yadav Pranav-yadav left a comment

Choose a reason for hiding this comment

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

added comments for few nits :)

Comment on lines 252 to 253
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.

Wouldn't it be nice if we have deprecation statement here as well ?
Just being curious since this is marked public

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.

Suggested change
* Interface to allow for creating and retrieving NativeModules. Why is this this class prefixed
* Interface to allow for creating and retrieving NativeModules. Why is this class prefixed

nit: extra this :)

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.

Suggested change
// jsRepresentation is also cached by by name by the TurboModuleManager
// jsRepresentation is also cached by name by the TurboModuleManager

nit: extra 'by' :)

@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D44000389

@RSNara RSNara force-pushed the export-D44000389 branch from a8323d5 to 410a790 Compare March 26, 2023 18:09
@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D44000389

@RSNara RSNara force-pushed the export-D44000389 branch from 410a790 to e562d20 Compare March 26, 2023 20:11
@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D44000389

@RSNara RSNara force-pushed the export-D44000389 branch from e562d20 to 4ffe0a7 Compare March 26, 2023 20:15
@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D44000389

@RSNara RSNara force-pushed the export-D44000389 branch from 4ffe0a7 to a343c2a Compare March 27, 2023 20:32
@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D44000389

@RSNara RSNara force-pushed the export-D44000389 branch from a343c2a to 4873aab Compare March 27, 2023 21:59
@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D44000389

@RSNara RSNara force-pushed the export-D44000389 branch from 4873aab to d2e8bb1 Compare March 28, 2023 14:05
@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D44000389

@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D44000389

RSNara and others added 3 commits March 28, 2023 08:23
Summary:
In Bridgeless mode, With the TurboModule interop layer, the TurboModule system will need to customize the nativeModuleProxy global.

This customization would be much easier if the nativeModuleProxy global were installed by the TurboModule system (and not the Bridgeless core).

So, this diff moves nativeModuleProxy installation into the TurboModule system.

Changelog: [Internal]

Differential Revision: https://internalfb.com/D43993197

fbshipit-source-id: 898851eff800f5151bb09c6e40a8e2f5cdeb7ba1
Summary:
## Context
Previously, jsRepresentation would only cache the **HostFunctions** returned from TurboModule::createHostFunction().

## Changes
This diff replaces TurboModule::createHostFunction() with TurboModule::create().

Now, jsRepresentation will cache **all** the **properties** returned from TurboModule::create().

## Motivation
For interop modules, constants will be exported as properties on the TurboModule HostObject. This diff allows those constants (which are non HostFunctions) to be cached.

Changelog: [Internal]

Differential Revision: https://internalfb.com/D44253229

fbshipit-source-id: b32ff9c108db414563dd4c81221af161029db965
Summary:
Pull Request resolved: facebook#36628

The legacy NativeModule system supports integer and float in NativeModule method arguments and returns. This diff extends the TurboModule system for the same functionality. T

his is necessary because the TurboModule system will now need to dispatch method calls to legacy NativeModules.

NOTE: We can't actually test these changes until we run interop modules.

Changelog: [Internal]

Reviewed By: javache

Differential Revision: D44000389

fbshipit-source-id: 5b2825164609549cc27f2ed04db0638d93712893
@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D44000389

@RSNara RSNara force-pushed the export-D44000389 branch from cdf8a6d to 449953a Compare March 28, 2023 15:26
@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Mar 28, 2023
@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request has been merged in cb07358.

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. fb-exported Merged This PR has been merged. p: Facebook Partner: Facebook Partner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants