-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
[Codegen]: Extract module/errors into a shared file #34896
[Codegen]: Extract module/errors into a shared file #34896
Conversation
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Base commit: fa22a6e |
Base commit: fa22a6e |
/rebase |
beb5481
to
6523b87
Compare
This PR should depend on this: #34874. |
We merged #34874 earlier today! :D |
49765f3
to
563f0aa
Compare
Sure. |
563f0aa
to
8958a1e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for this work! :D
This will enable further improvements!
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@@ -31,6 +33,413 @@ class ParserError extends Error { | |||
} | |||
} | |||
|
|||
class MisnamedModuleInterfaceParserError extends ParserError { | |||
constructor(nativeModuleName: string, id: $FlowFixMe, language: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #34916, another contributor suggested to use a union type to describe the parsers. I really like this suggestion, and Flow
will help users typing the errors if we type this correctly.
Ideally, we can do something like:
type ParserType = 'Flow' | 'TypeScript'
// ...
// update all the constructors
class MisnamedModuleInterfaceParserError extends ParserError {
constructor(nativeModuleName: string, id: $FlowFixMe, language: ParserType) {
// ...
Nothing else in the logic will change, but the types will be more precise.
What do you think? Could you please update the PR? 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I'll update the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it! ❤️ Thank you so much for this change!
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
/rebase |
7a4dbfd
to
1e3b195
Compare
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This pull request was successfully merged by @tarunrajput in 7b345bc. When will my fix make it into a release? | Upcoming Releases |
Summary: This PR reduces code duplication by extracting all the errors in the module/errors into a single parsers/errors.js file. All the errors must drop the corresponding Flow or Typescript token in the name and take an extra language parameter in the constructor. Also, rename the hasteModuleName parameter to nativeModuleName. Part of facebook#34872 ## 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 all the parsers errors in the module/errors into a single parsers/errors.js file Pull Request resolved: facebook#34896 Test Plan: run ```yarn jest react-native-codegen``` and check all test case passes. <img width="793" alt="image" src="https://user-images.githubusercontent.com/34857453/194545577-cf2d980b-b6b7-4f93-b13e-2e45d92dceab.png"> Reviewed By: rshest Differential Revision: D40176486 Pulled By: rshest fbshipit-source-id: b33ae49b2bcceeffd307370ee5e3b24a9e1bb340
Summary
This PR reduces code duplication by extracting all the errors in the module/errors into a single parsers/errors.js file. All the errors must drop the corresponding Flow or Typescript token in the name and take an extra language parameter in the constructor. Also, rename the hasteModuleName parameter to nativeModuleName.
Part of #34872
Changelog
[Internal] [Changed] - Extract all the parsers errors in the module/errors into a single parsers/errors.js file
Test Plan
run
yarn jest react-native-codegen
and check all test case passes.