Skip to content

Extract the content of the case 'Int32' (Flow, TypeScript) into a single emitInt32 function in the parsers-primitives.js file.#34906

Closed
Naturalclar wants to merge 3 commits into
facebook:mainfrom
Naturalclar:chore/refactorParserModuleInt32
Closed

Extract the content of the case 'Int32' (Flow, TypeScript) into a single emitInt32 function in the parsers-primitives.js file.#34906
Naturalclar wants to merge 3 commits into
facebook:mainfrom
Naturalclar:chore/refactorParserModuleInt32

Conversation

@Naturalclar
Copy link
Copy Markdown
Contributor

@Naturalclar Naturalclar commented Oct 9, 2022

Summary

Part of #34872

Refactors emitInt32 in codegen parser module into a common parsers-primitive.js

Changelog

[Internal] [Changed] - Extract the content of the case 'Int32' (Flow, TypeScript) into a single emitInt32 function

Test Plan

Ran yarn jest react-native-codegen

Screen Shot 2022-10-09 at 15 45 21

@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 9, 2022
@analysis-bot
Copy link
Copy Markdown

analysis-bot commented Oct 9, 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: 7227bde
Branch: main

@analysis-bot
Copy link
Copy Markdown

analysis-bot commented Oct 9, 2022

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

Base commit: 7227bde
Branch: main

@rshest rshest self-requested a review October 9, 2022 11:39
@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! :D

We can improve the quality even further by adding 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! 🙏

@Naturalclar
Copy link
Copy Markdown
Contributor Author

@cipolleschi
Thanks for the review! I've added a unit test for emitInt32.

@Naturalclar Naturalclar requested review from cipolleschi and removed request for rshest October 9, 2022 14:15
@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

@rshest rshest left a comment

Choose a reason for hiding this comment

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

Please rename parser-primitive-test.js -> parsers-primitives-test.js. Thanks!

@@ -0,0 +1,40 @@
/**
Copy link
Copy Markdown
Contributor

@rshest rshest Oct 9, 2022

Choose a reason for hiding this comment

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

Thanks for adding the test!
Could you please name this parsers-primitives-test.js, for consistency?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rshest thanks for the review!
I realized that the original file was called parsers-primitives.js, both word containing s
So I renamed the test to also follow the same parsers-primitives-test.js

@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 @Naturalclar in db8c11d.

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 9, 2022
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