Skip to content

Extracted IncorrectModuleRegistryCallArityParserError as a seperate function in error-utils.js #34940

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 IncorrectModuleRegistryCallArityParserError as a seperate function in error-utils.js #34940

wants to merge 4 commits into from

Conversation

@dakshbhardwaj
Copy link
Contributor

@dakshbhardwaj dakshbhardwaj commented Oct 11, 2022

Summary

This PR is part of #34872
This PR extracts IncorrectModuleRegistryCallArityParserError exception to throwIfIncorrectModuleRegistryCallArityParserError inside an error-utils.js file.

Changelog

[Internal] [Changed] - Extracted IncorrectModuleRegistryCallArityParserError exception to throwIfIncorrectModuleRegistryCallArityParserError function

Test Plan

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

yarn jest react-native-codegen

Screenshot 2022-10-11 at 4 04 55 PM

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

analysis-bot commented Oct 11, 2022 •
edited
Loading

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,768,561 +0
android hermes armeabi-v7a 7,169,658 +0
android hermes x86 8,081,830 +0
android hermes x86_64 8,053,393 +0
android jsc arm64-v8a 9,629,713 +0
android jsc armeabi-v7a 8,394,120 +0
android jsc x86 9,579,164 +0
android jsc x86_64 10,172,202 +0

Base commit: f353119
Branch: main

cipolleschi
cipolleschi requested changes Oct 11, 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.

Amazing job, thank you for working on this.
I find that the name of the function is a bit obscure. How do you feel about the rename I'm suggesting in the PR?

packages/react-native-codegen/src/parsers/error-utils.js Outdated
nativeModuleName: string,
flowCallExpression: $FlowFixMe,
methodName: string,
incorrectArity: number,
Copy link
Contributor

@cipolleschi cipolleschi Oct 11, 2022

Choose a reason for hiding this comment

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

what about calling this numberOfCallExpressionArgs? I think that it conveys more semantic that just incorrectArity. Also, this is a parameter: it does not know there it is correct or not. It doesn't feel right to state incorrect in the parameter name. What do you think?

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

const {IncorrectModuleRegistryCallArityParserError} = require('./errors');

function throwIfIncorrectModuleRegistryCallArityParserError(
Copy link
Contributor

@cipolleschi cipolleschi Oct 11, 2022

Choose a reason for hiding this comment

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

What about calling this function throwIfWrongNumberOfCallExpressionArgs? I think that it explains better what the method will do: if there is a wrong number of argument in the call expression, it throws.

What do you think?

packages/react-native-codegen/src/parsers/__tests__/error-utils-test.js Outdated
const {IncorrectModuleRegistryCallArityParserError} = require('../errors');

describe('throwIfIncorrectModuleRegistryCallArityParserError', () => {
it('throw error if incorrect module registry call arity is used', () => {
Copy link
Contributor

@cipolleschi cipolleschi Oct 11, 2022

Choose a reason for hiding this comment

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

we should update the it description if we change the name of the function.

@dakshbhardwaj
Copy link
Contributor Author

dakshbhardwaj commented Oct 11, 2022

Amazing job, thank you for working on this. I find that the name of the function is a bit obscure. How do you feel about the rename I'm suggesting in the PR?

Thanks for the review @cipolleschi, Changes suggested by you look good to me.
I have pushed down the changes.

@analysis-bot
Copy link

analysis-bot commented Oct 11, 2022 •
edited
Loading

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

Base commit: f353119
Branch: main

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Oct 12, 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 14, 2022

This pull request was successfully merged by @dakshbhardwaj in 76c5b6f.

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 14, 2022
mohitcharkha pushed a commit to mohitcharkha/react-native that referenced this pull request Oct 17, 2022
@dakshbhardwaj
Extracted IncorrectModuleRegistryCallArityParserError as a seperate f…
c82a0ff 
…unction in error-utils.js (facebook#34940)

Summary:
This PR is part of facebook#34872
This PR extracts `IncorrectModuleRegistryCallArityParserError` exception to `throwIfIncorrectModuleRegistryCallArityParserError` inside an `error-utils.js` file.

[Internal] [Changed] - Extracted IncorrectModuleRegistryCallArityParserError exception to throwIfIncorrectModuleRegistryCallArityParserError function

Pull Request resolved: facebook#34940

Test Plan:
Added unit case in error-utils-test.js file

 yarn jest react-native-codegen

<img width="1144" alt="Screenshot 2022-10-11 at 4 04 55 PM" src="https://user-images.githubusercontent.com/22423684/195070498-627d1bb8-53b1-43d3-b410-462e4f0da23a.png">

Reviewed By: dmytrorykun

Differential Revision: D40296626

Pulled By: cipolleschi

fbshipit-source-id: 33a299b1adf6334753c2390a81a54832c9096ec5
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
@dakshbhardwaj @OlimpiaZurek
Extracted IncorrectModuleRegistryCallArityParserError as a seperate f…
cf7f332 
…unction in error-utils.js (facebook#34940)

Summary:
This PR is part of facebook#34872
This PR extracts `IncorrectModuleRegistryCallArityParserError` exception to `throwIfIncorrectModuleRegistryCallArityParserError` inside an `error-utils.js` file.

## Changelog
[Internal] [Changed] - Extracted IncorrectModuleRegistryCallArityParserError exception to throwIfIncorrectModuleRegistryCallArityParserError function

Pull Request resolved: facebook#34940

Test Plan:
Added unit case in error-utils-test.js file

 yarn jest react-native-codegen

<img width="1144" alt="Screenshot 2022-10-11 at 4 04 55 PM" src="https://user-images.githubusercontent.com/22423684/195070498-627d1bb8-53b1-43d3-b410-462e4f0da23a.png">

Reviewed By: dmytrorykun

Differential Revision: D40296626

Pulled By: cipolleschi

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

Reviewers

@cipolleschi cipolleschi cipolleschi approved these changes

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

@dakshbhardwaj @analysis-bot @facebook-github-bot @react-native-bot @cipolleschi