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

Accept TypeScript type T | null | undefined as a maybe type of T in turbo module #34158

Closed
wants to merge 3 commits into from

Conversation

ZihanChen-MSFT
Copy link
Contributor

Summary

According Flow's document, a maybe type of T means T | null | undefined, instead of T | null | void.
I think keeping TypeScript and Flow being consistent to each other is better.

Changelog

[General] [Changed] - Accept TypeScript type T | null | undefined as a maybe type of T in turbo module.

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 8, 2022
@ZihanChen-MSFT ZihanChen-MSFT changed the title Ts rncodegen Accept TypeScript type T | null | undefined as a maybe type of T in turbo module Jul 8, 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 8, 2022
@facebook-github-bot
Copy link
Contributor

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

@yungsters
Copy link
Contributor

Thanks!

Isn't it also the case that void only ever really makes sense as the return type of a function? (I am asking this question to improve my understanding of TypeScript.)

@cortinico cortinico added p: Microsoft Partner: Microsoft Partner labels Jul 12, 2022
@ZihanChen-MSFT
Copy link
Contributor Author

@yungsters Although TypeScript compiler doesn't limit where you could use void, but yes void only make sense as a function return type.

@facebook-github-bot
Copy link
Contributor

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

@cipolleschi
Copy link
Contributor

Hi @ZihanChen-MSFT, could you please rebase this PR? There are some errors internally that a rebase could fix. Thanks!

@ZihanChen-MSFT
Copy link
Contributor Author

@cipolleschi done!

@facebook-github-bot
Copy link
Contributor

@philIip 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 9ecd203.

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 16, 2022
@ZihanChen-MSFT ZihanChen-MSFT deleted the ts-rncodegen branch September 9, 2022 21:44
dmytrorykun pushed a commit that referenced this pull request Sep 14, 2022
…urbo module (#34158)

Summary:
According Flow's document, a maybe type of T means `T | null | undefined`, instead of `T | null | void`.
I think keeping TypeScript and Flow being consistent to each other is better.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[General] [Changed] - Accept TypeScript type `T | null | undefined` as a maybe type of T in turbo module.

Pull Request resolved: #34158

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

Reviewed By: yungsters

Differential Revision: D37731169

Pulled By: philIip

fbshipit-source-id: b6d9b7e8991f60e12c1004bed5b937b34fb02c47
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. Needs: React Native Team Attention 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants