[Codegen_buildModuleSchema] extract buildModuleSchema to parsers-commons #36330
Conversation
facebook-github-bot
added
the
CLA Signed
| tryParse, | ||
| cxxOnly, | ||
| resolveTypeAnnotation, | ||
| translateTypeAnnotation, |
There was a problem hiding this comment.
@cipolleschi , Here resolveTypeAnnotation and translateTypeAnnotation are different for both parsers. Could you please confirm whether the two functions should be added as callbacks, or they should be added to their respective parsers? Thank you.
There was a problem hiding this comment.
I'll try to go for the simplest approach. So, for now, it would be easiest to accept them as callback, while we work out the difference and we try to unify them.
If that causes some weird import cycle, we can then understand whether it make sense to move them to the Parsers.
What do you think?
cipolleschi
left a comment
cipolleschi
left a comment
There was a problem hiding this comment.
I love that we are removing ~200 LOC here! Thank you so much!
I answered the question in a comment, but it looks good so far!
| tryParse, | ||
| cxxOnly, | ||
| resolveTypeAnnotation, | ||
| translateTypeAnnotation, |
There was a problem hiding this comment.
I'll try to go for the simplest approach. So, for now, it would be easiest to accept them as callback, while we work out the difference and we try to unify them.
If that causes some weird import cycle, we can then understand whether it make sense to move them to the Parsers.
What do you think?
tarunrajput
force-pushed
the
refactor/extract-buildModuleSchema
branch
from
50de956 to
3e8c4de
Compare
Base commit: 947751f |
tests(codegen: Add test for buildModuleSchema
tarunrajput
force-pushed
the
refactor/extract-buildModuleSchema
branch
from
3e8c4de to
4818733
Compare
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
1 similar comment
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@cipolleschi merged this pull request in 3cd97e4. |
Summary: > Extract the buildModuleSchema function ([Flow](https://github.com/facebook/react-native/blob/main/packages/react-native-codegen/src/parsers/flow/modules/index.js#L571), [TypeScript](https://github.com/facebook/react-native/blob/main/packages/react-native-codegen/src/parsers/typescript/modules/index.js#L584))in the parsers-commons.js function. The two functions are almost identical except for the filter(property =>) at the end of the function, which is different based on the language. Part of Codegen Issue facebook#34872 Depends on Codegen 74, Codegen 84. ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [INTERNAL] [CHANGED] - Extract buildModuleSchema from Flow and TypeScript parsers modules to parsers-commons Pull Request resolved: facebook#36330 Test Plan: `yarn lint && yarn run flow && yarn test react-native-codegen ` Reviewed By: christophpurrer Differential Revision: D43833653 Pulled By: cipolleschi fbshipit-source-id: 4d67cb5ef746ecd7ace4b91eb30d36e96252e779
4 participants
Summary
Part of Codegen Issue #34872
Depends on Codegen 74, Codegen 84.
Changelog
[INTERNAL] [CHANGED] - Extract buildModuleSchema from Flow and TypeScript parsers modules to parsers-commons
Test Plan
yarn lint && yarn run flow && yarn test react-native-codegen