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

Support TypeScript type (T) for turbo module codegen (module only) #34621

Closed
wants to merge 4 commits into from

Conversation

ZihanChen-MSFT
Copy link
Contributor

Summary

  1. In some situation (I don't know exactly how it is triggered but I found that during porting it to react-native-windows), ExportNamedDeclaration.exportKind is missing. Just skip the checking to exportKind because the rest of the checking is sufficient without reading this field.
  2. Add TSParenthesizedType to module codegen in TypeScript, so that type (T) is treated like T.

Changelog

[General] [Changed] - codegen: support TypeScript type (T) for turbo module codegen (module only)

Test Plan

yarn jest passed in packages/react-native-codegen

@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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Sep 7, 2022
@facebook-github-bot
Copy link
Contributor

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

@RSNara
Copy link
Contributor

RSNara commented Sep 7, 2022

Add TSParenthesizedType to module codegen in TypeScript, so that type (T) is treated like T.

@ZihanChen-MSFT, for this case at least, could you add a new case to the snapshot tests?

@cipolleschi cipolleschi closed this Sep 8, 2022
@cipolleschi cipolleschi reopened this Sep 8, 2022
@analysis-bot
Copy link

analysis-bot commented Sep 8, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,641,162 -765
android hermes armeabi-v7a 7,053,523 -757
android hermes x86 7,942,931 -763
android hermes x86_64 7,914,849 -756
android jsc arm64-v8a 9,514,387 -1,154
android jsc armeabi-v7a 8,289,999 -1,154
android jsc x86 9,453,715 -1,155
android jsc x86_64 10,044,784 -1,153

Base commit: e8739e9
Branch: main

@ZihanChen-MSFT
Copy link
Contributor Author

@ZihanChen-MSFT, for this case at least, could you add a new case to the snapshot tests?

@RSNara Done!

@cipolleschi cipolleschi closed this Sep 9, 2022
@cipolleschi cipolleschi reopened this Sep 9, 2022
@facebook-github-bot
Copy link
Contributor

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

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: e8739e9
Branch: main

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @ZihanChen-MSFT in 624bdc7.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Sep 9, 2022
@ZihanChen-MSFT ZihanChen-MSFT deleted the ts-rncodegen5 branch September 9, 2022 21:44
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
…acebook#34621)

Summary:
1. In some situation (I don't know exactly how it is triggered but I found that during porting it to `react-native-windows`), `ExportNamedDeclaration.exportKind` is missing. Just skip the checking to `exportKind` because the rest of the checking is sufficient without reading this field.
2. Add `TSParenthesizedType` to module codegen in TypeScript, so that type `(T)` is treated like `T`.

## Changelog

[General] [Changed] - codegen: support TypeScript type `(T)` for turbo module codegen (module only)

Pull Request resolved: facebook#34621

Test Plan: `yarn jest` passed in `packages/react-native-codegen`

Reviewed By: RSNara

Differential Revision: D39322001

Pulled By: cipolleschi

fbshipit-source-id: 1855711da7062a065c05a10f275e26baa88cf75f
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.

6 participants