Skip to content

Extracted UnsupportedFunctionReturnTypeAnnotationParserError to throwIfUnsupportedFunctionReturnTypeAnnotationParserError #34965

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

Extracted UnsupportedFunctionReturnTypeAnnotationParserError to throwIfUnsupportedFunctionReturnTypeAnnotationParserError #34965

wants to merge 4 commits into from

Conversation

@harsh-siriah
Copy link

@harsh-siriah harsh-siriah commented Oct 13, 2022

Summary

This PR is part of #34872
This PR extracts UnsupportedFunctionReturnTypeAnnotationParserError exception to a separate function inside an error-utils.js file

Changelog

[Internal] [Changed] - Extract UnsupportedFunctionReturnTypeAnnotationParserError to a seperate function inside error-utils.js

Test Plan

yarn jest react-native-codegen

Added unit case in error-utils-test.js file

Screenshot 2022-10-13 at 11 46 54 AM

Extracted UnsupportedFunctionReturnTypeAnnotationParserError to throw…
da237df 
…IfUnsupportedFunctionReturnTypeAnnotationParserError
@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 13, 2022
@analysis-bot
Copy link

analysis-bot commented Oct 13, 2022 •
edited
Loading

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,778,988 +0
android hermes armeabi-v7a 7,182,660 +0
android hermes x86 8,093,664 +0
android hermes x86_64 8,065,204 +0
android jsc arm64-v8a 9,637,485 +0
android jsc armeabi-v7a 8,403,080 +0
android jsc x86 9,587,446 +0
android jsc x86_64 10,181,002 +0

Base commit: 31f2199
Branch: main

@analysis-bot
Copy link

analysis-bot commented Oct 13, 2022 •
edited
Loading

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

Base commit: 31f2199
Branch: main

cipolleschi
cipolleschi approved these changes Oct 13, 2022
View reviewed changes
Copy link
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.

Hi, thank you so much for taking this task! 👏

I left a very small comment on naming. A part from that, it looks good to me!

packages/react-native-codegen/src/parsers/error-utils.js Outdated

function throwIfUnsupportedFunctionReturnTypeAnnotationParserError(
nativeModuleName: string,
flowReturnTypeAnnotation: $FlowFixMe,
Copy link
Contributor

@cipolleschi cipolleschi Oct 13, 2022

Choose a reason for hiding this comment

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

nit: can we change the name of the parameter with just returnTypeAnnotation?
At this point, we are not discriminating between the language in the params.

Thank you so much!

Copy link
Author

@harsh-siriah harsh-siriah Oct 13, 2022

Choose a reason for hiding this comment

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

Then should I remove the returnTypeAnnotation on line 25? Can both of them be handled from a common variable?

Copy link
Contributor

@cipolleschi cipolleschi Oct 17, 2022

Choose a reason for hiding this comment

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

I'm not sure I am following your suggestion. My comment was related to the name of the parameter we are using.
Can you expand on how will you remove the returnTypeAnnotation? Would you like to pass directly the returnTypeAnnotation.type value?

Copy link
Author

@harsh-siriah harsh-siriah Oct 17, 2022

Choose a reason for hiding this comment

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

I have made some changes to the parameter name. I hope that works. Please let me know what you think.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Oct 13, 2022

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

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Oct 17, 2022

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

@react-native-bot
Copy link
Collaborator

react-native-bot commented Oct 19, 2022

This pull request was successfully merged by @harsh-siriah in 3247436.

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
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
@OlimpiaZurek
Extracted UnsupportedFunctionReturnTypeAnnotationParserError to throw…
e0767bf 
…IfUnsupportedFunctionReturnTypeAnnotationParserError (facebook#34965)

Summary:
This PR is part of facebook#34872
This PR extracts `UnsupportedFunctionReturnTypeAnnotationParserError` exception to a separate function inside an `error-utils.js` file

## Changelog

[Internal] [Changed] - Extract `UnsupportedFunctionReturnTypeAnnotationParserError` to a seperate function inside `error-utils.js`

Pull Request resolved: facebook#34965

Test Plan:
```sh
yarn jest react-native-codegen
```
Added unit case in `error-utils-test.js` file

<img width="939" alt="Screenshot 2022-10-13 at 11 46 54 AM" src="https://user-images.githubusercontent.com/86605635/195517350-dcb7a26d-434c-4e45-a174-ce82931073e5.png">

Reviewed By: dmytrorykun

Differential Revision: D40338048

Pulled By: cipolleschi

fbshipit-source-id: baa41e0e96c9e17a35f316433c8d80c9bf88d334
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cipolleschi cipolleschi Awaiting requested review from cipolleschi

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@harsh-siriah @analysis-bot @facebook-github-bot @react-native-bot @cipolleschi