Skip to content

[Codegen] Extract the switch(configType) block from the buildSchema function into a new function in the parsers/utils.js file#34992

Closed
MaeIg wants to merge 3 commits into
facebook:mainfrom
MaeIg:refactor/extract-switch-block-buildSchema
Closed

[Codegen] Extract the switch(configType) block from the buildSchema function into a new function in the parsers/utils.js file#34992
MaeIg wants to merge 3 commits into
facebook:mainfrom
MaeIg:refactor/extract-switch-block-buildSchema

Conversation

@MaeIg
Copy link
Copy Markdown
Contributor

@MaeIg MaeIg commented Oct 15, 2022

Summary

This PR aims to extract the switch(configType) block from the buildSchema function into a separate function in a shared file between typescript and flow. It is a task of #34872:

This task is more advanced than the others. Extract the switch(configType) block (Flow, TypeScript) from the buildSchema function into a new function in the parsers/utils.js file and use it in the two functions. Note that the new function must accept some callbacks to wrapModule/ComponentSchema and to buildModule/ComponentSchema.

Changelog

[Internal] [Changed] - Extract the switch(configType) block from the buildSchema function into a new function in the parsers/utils.js file

Test Plan

yarn flow:
image

yarn lint:
image

yarn jest react-native-codegen:
image

I added new tests:
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 15, 2022
@MaeIg
Copy link
Copy Markdown
Contributor Author

MaeIg commented Oct 15, 2022

Hi @cipolleschi,

As requested in the task, I put buildComponentSchema, wrapComponentSchema and buildModuleSchema as parameters of the new function. But, these 3 functions are duplicated in flow and typescript folder.

I think we should consider extracting them into parsers/parsers-commons.js as done previously (2 years ago) for wrapModuleSchema. For me, they are really easy to extract!

What do you think about it? Should I propose these tasks in the umbrella issue?

@analysis-bot
Copy link
Copy Markdown

analysis-bot commented Oct 15, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,778,150 +0
android hermes armeabi-v7a 7,179,206 +0
android hermes x86 8,091,069 +0
android hermes x86_64 8,062,841 +0
android jsc arm64-v8a 9,637,352 +0
android jsc armeabi-v7a 8,401,647 +0
android jsc x86 9,586,544 +0
android jsc x86_64 10,179,709 +0

Base commit: 3c8d678
Branch: main

@analysis-bot
Copy link
Copy Markdown

analysis-bot commented Oct 15, 2022

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

Base commit: 5e6a4c5
Branch: main

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.

Seems very good to me and very thanks for the tests also!

Your proposal on those extra refactoring is pretty good and, eventually, we will do that and we will add them to the umbrella issue.
The wrapModuleSchema has already been refactored. The buildModuleSchema is pretty big and, IIRC, there are other PR that are working on parts of the function. I'd rather to focus on merging all the open PRs, now, to help us in figuring out what's left to do!

I hope to add some more tasks later this week. 👍

@cipolleschi
Copy link
Copy Markdown
Contributor

@MaeIg could you rebase this please? 🙏

MaeIg added 3 commits October 17, 2022 11:59
It doesn't reset their implementation but clears the call history for functions.
It is useful for each test to be independent when testing that a function has been called.
@MaeIg MaeIg force-pushed the refactor/extract-switch-block-buildSchema branch from b1ecf99 to df5bc7d Compare October 17, 2022 10:02
@MaeIg
Copy link
Copy Markdown
Contributor Author

MaeIg commented Oct 17, 2022

@MaeIg could you rebase this please? 🙏

Done! 😃

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@cipolleschi 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 8f484c3.

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 19, 2022
@MaeIg MaeIg deleted the refactor/extract-switch-block-buildSchema branch October 19, 2022 14:02
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