-
Notifications
You must be signed in to change notification settings - Fork 24.8k
[Codegen] Extract RootTag case of translateTypeAnnotation from the flow and typescript folders in parsers-primitives #34901
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
Conversation
3b0c364
to
cd52bee
Compare
Base commit: 54fc62c |
cd52bee
to
d698e7c
Compare
Base commit: 54fc62c |
d698e7c
to
5778fe3
Compare
@rshest has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this. 👏 That's amazing!
We can improve the quality even further byadding a unit test.
The steps should be:
- add the
__tests__
subfolder in thepackages/react-native-codegen/src/parsers
. - add a
parser-primitives-test.js
where we unit test this fuction:- we test the output when the
nullable
parameter isfalse
- we test the output when the
nullable
parameter istrue
- we test the output when the
An example of unit test can be found here.
Thank you so much! 🙏
5778fe3
to
72d26ac
Compare
Thanks for the review! I thought this case was already covered in these files (flow and typescript). But you're right, it's clearer and safer to test It's done if you want to re-review it! |
@rshest has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@MaeIg Just a heads up - looks like there are some conflicting changes in the new main, have to be resolved before merging. |
ed95539
to
1826b7f
Compare
1826b7f
to
5894434
Compare
@rshest has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This pull request was successfully merged by @MaeIg in 5f05b07. When will my fix make it into a release? | Upcoming Releases |
…escript folders in parsers-primitives (facebook#34901) Summary: This PR aims to reduce code duplication by extracting `emitRootTag` logic from the flow and typescript folders into a shared parsers-primitives file. It is a task of facebook#34872: > Extract the content of the case 'RootTag' (Flow TypeScript) into a single emitRootTag function in the parsers-primitives.js file. Use the new function in the parsers. ~~Note that facebook#34898 should be merged first. I rebased on it's branch.~~ ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. For an example, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [Internal] [Changed] - Extract RootTag case of translateTypeAnnotation from the flow and typescript folders in parsers-primitives Pull Request resolved: facebook#34901 Test Plan: All tests are passing, with `yarn jest react-native-codegen`: <img width="934" alt="image" src="https://user-images.githubusercontent.com/40902940/194777150-6136c1b6-11f8-4e21-829b-fda418b6925c.png"> Reviewed By: cipolleschi Differential Revision: D40212553 Pulled By: cipolleschi fbshipit-source-id: eadbbfb5cf6dfa6c966f4c08a5d9372a3470b621
Summary
This PR aims to reduce code duplication by extracting
emitRootTag
logic from the flow and typescript folders into a shared parsers-primitives file. It is a task of #34872:Note that #34898 should be merged first. I rebased on it's branch.Changelog
[Internal] [Changed] - Extract RootTag case of translateTypeAnnotation from the flow and typescript folders in parsers-primitives
Test Plan
All tests are passing, with

yarn jest react-native-codegen
: