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 array types for turbo module (module only) #34183

Closed
wants to merge 6 commits into from

Conversation

ZihanChen-MSFT
Copy link
Contributor

Summary

Turbo module codegen supports arrays in both Flow and TypeScript, but it only recognize Array<T> and ReadonlyArray<T> in TypeScript.

In this change, T[] and readonly T[] are made recognizable in codegen.

Changelog

[General] [Added] - Support TypeScript array types for turbo module (module only)

Test Plan

yarn jest passed in packages/react-native-codegen

@facebook-github-bot 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 Jul 13, 2022
@react-native-bot react-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label Jul 13, 2022
@facebook-github-bot 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 Jul 13, 2022
Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

Thanks @ZihanChen-MSFT for the PR.

Left a comment to clarify a line.
Overall, it looks good to me. Many thanks for the tests also!

aliasMap,
cxxOnly,
'ReadonlyArray',
typeAnnotation.typeAnnotation.elementType,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct? I can can see in the previous case that the line was just typeAnnotation.elementType instead of typeAnnotation.typeAnnotation.elementType

Copy link
Contributor Author

@ZihanChen-MSFT ZihanChen-MSFT Jul 13, 2022

Choose a reason for hiding this comment

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

This is intended.
For T[], it is a TSArrayType on a TSTypeReference.
For readonly T[], it is a TSTypeOperator on a TSArrayType on a TSTypeReference.

This is why it has one more .typeAnnotation.

And this should be proven by the unit test, since I copied Array<T> test cases to T[] (same for readonly), and copied snapshots, it ensures that Array<T> case and T[] case provide exactly the same output.

@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.

@react-native-bot
Copy link
Collaborator

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

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 Jul 13, 2022
@ZihanChen-MSFT ZihanChen-MSFT deleted the ts-rncodegen2 branch September 9, 2022 21:44
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. p: Microsoft Partner: Microsoft Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants