Skip to content

[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

Closed

Conversation

MaeIg
Copy link
Contributor

@MaeIg MaeIg commented Oct 8, 2022

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:

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 #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:
image

@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 Oct 8, 2022
@MaeIg MaeIg force-pushed the refactor/extract-emitRootTag-function branch from 3b0c364 to cd52bee Compare October 8, 2022 14:42
@analysis-bot
Copy link

analysis-bot commented Oct 8, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,747,611 +0
android hermes armeabi-v7a 7,149,395 +0
android hermes x86 8,058,588 +0
android hermes x86_64 8,029,524 +0
android jsc arm64-v8a 9,608,518 +0
android jsc armeabi-v7a 8,373,768 +0
android jsc x86 9,555,784 +0
android jsc x86_64 10,148,324 +0

Base commit: 54fc62c
Branch: main

@MaeIg MaeIg force-pushed the refactor/extract-emitRootTag-function branch from cd52bee to d698e7c Compare October 8, 2022 15:47
@analysis-bot
Copy link

analysis-bot commented Oct 8, 2022

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

Base commit: 54fc62c
Branch: main

@MaeIg MaeIg force-pushed the refactor/extract-emitRootTag-function branch from d698e7c to 5778fe3 Compare October 8, 2022 18:17
@rshest rshest self-requested a review October 9, 2022 11:41
@facebook-github-bot
Copy link
Contributor

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

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 for taking this. 👏 That's amazing!

We can improve the quality even further byadding a unit test.
The steps should be:

  1. add the __tests__ subfolder in the packages/react-native-codegen/src/parsers.
  2. add a parser-primitives-test.js where we unit test this fuction:
    1. we test the output when the nullable parameter is false
    2. we test the output when the nullable parameter is true

An example of unit test can be found here.

Thank you so much! 🙏

@MaeIg MaeIg force-pushed the refactor/extract-emitRootTag-function branch from 5778fe3 to 72d26ac Compare October 9, 2022 19:43
@MaeIg
Copy link
Contributor Author

MaeIg commented Oct 9, 2022

Thanks for taking this. 👏 That's amazing!

We can improve the quality even further byadding a unit test. The steps should be:

  1. add the __tests__ subfolder in the packages/react-native-codegen/src/parsers.

  2. add a parser-primitives-test.js where we unit test this fuction:

    1. we test the output when the nullable parameter is false
    2. we test the output when the nullable parameter is true

An example of unit test can be found here.

Thank you so much! 🙏

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 emitRootTag function 😁

It's done if you want to re-review it!

@MaeIg MaeIg requested review from cipolleschi and removed request for rshest October 9, 2022 20:01
@facebook-github-bot
Copy link
Contributor

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

@rshest
Copy link
Contributor

rshest commented Oct 9, 2022

@MaeIg Just a heads up - looks like there are some conflicting changes in the new main, have to be resolved before merging.

@facebook facebook deleted a comment from facebook-github-bot Oct 9, 2022
@MaeIg MaeIg force-pushed the refactor/extract-emitRootTag-function branch from ed95539 to 1826b7f Compare October 9, 2022 22:55
@MaeIg
Copy link
Contributor Author

MaeIg commented Oct 9, 2022

@MaeIg Just a heads up - looks like there are some conflicting changes in the new main, have to be resolved before merging.

@rshest I rebased. Thanks 🙂

@MaeIg MaeIg force-pushed the refactor/extract-emitRootTag-function branch from 1826b7f to 5894434 Compare October 10, 2022 01:31
@facebook-github-bot
Copy link
Contributor

@rshest 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 @MaeIg in 5f05b07.

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 Oct 10, 2022
@MaeIg MaeIg deleted the refactor/extract-emitRootTag-function branch October 10, 2022 17:59
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
…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
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. hacktoberfest-accepted Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants