Skip to content

[Codegen] Extract RootTag case of translateTypeAnnotation from the flow and typescript folders in parsers-primitives#34901

Closed
MaeIg wants to merge 2 commits into
facebook:mainfrom
MaeIg:refactor/extract-emitRootTag-function
Closed

[Codegen] Extract RootTag case of translateTypeAnnotation from the flow and typescript folders in parsers-primitives#34901
MaeIg wants to merge 2 commits into
facebook:mainfrom
MaeIg:refactor/extract-emitRootTag-function

Conversation

@MaeIg
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown

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
Copy Markdown
Contributor

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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

@rshest
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
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