Skip to content

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

Closed
dakshbhardwaj wants to merge 4 commits into
facebook:mainfrom
dakshbhardwaj:daksh/IncorrectModuleRegistryCallArityParserError
Closed

Extracted IncorrectModuleRegistryCallArityParserError as a seperate function in error-utils.js#34940
dakshbhardwaj wants to merge 4 commits into
facebook:mainfrom
dakshbhardwaj:daksh/IncorrectModuleRegistryCallArityParserError

Conversation

@dakshbhardwaj
Copy link
Copy Markdown
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
Copy Markdown

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
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.

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?

Comment thread packages/react-native-codegen/src/parsers/error-utils.js Outdated
nativeModuleName: string,
flowCallExpression: $FlowFixMe,
methodName: string,
incorrectArity: number,
Copy link
Copy Markdown
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?

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

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

function throwIfIncorrectModuleRegistryCallArityParserError(
Copy link
Copy Markdown
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?

Comment thread 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
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