Extract UnsupportedObjectPropertyValueTypeAnnotationParserError to a throwing function by dhruvtailor7 · Pull Request #34917 · facebook/react-native

dhruvtailor7 · 2022-10-10T07:40:15Z

Summary

This PR is a part of #34872.
Extracted the UnsupportedObjectPropertyValueTypeAnnotationParserError in its own throwing function and reuse that function passing a proper type.

Changelog

[Internal] [Changed] - Extract the UnsupportedObjectPropertyValueTypeAnnotationParserError in its own throwing function and reuse that function passing a proper type.

Test Plan

Output of yarn jest react-native-codegen.

…throwing function.

analysis-bot · 2022-10-10T08:05:05Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,778,150	+0
android	hermes	armeabi-v7a	7,179,206	+0
android	hermes	x86	8,091,069	+0
android	hermes	x86_64	8,062,841	+0
android	jsc	arm64-v8a	9,637,352	+0
android	jsc	armeabi-v7a	8,401,647	+0
android	jsc	x86	9,586,544	+0
android	jsc	x86_64	10,179,709	+0

Base commit: 629e8e0
Branch: main

analysis-bot · 2022-10-10T08:13:33Z

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

Base commit: 629e8e0
Branch: main

cipolleschi

Hi @dhruvtailor7! Thanks a lot for taking this task and for the amazing job! 👏

I left some comments to suggest further improvements in the codebase. Could you update the PR with these changes? 🙏

cipolleschi · 2022-10-10T08:27:59Z

+const UnsupportedObjectPropertyTypeToInvalidPropertyValueTypeMap = {
+  FunctionTypeAnnotation: 'FunctionTypeAnnotation',
+  VoidTypeAnnotation: 'void',
+  PromiseTypeAnnotation: 'Promise',
+};
+
+function throwUnsupportedObjectPropertyValueTypeAnnotationParserError(
+  moduleName,
+  propertyValue,
+  propertyKey,
+  type,
+) {
+  throw new UnsupportedObjectPropertyValueTypeAnnotationParserError(
+    moduleName,
+    propertyValue,
+    propertyKey,
+    type,
+  );
+}
+


This can be moved into and error-utils.js file which is shared between typescript and flow. As you can see, the logic is exactly the same.

Yes, I was waiting for the other PR to be merged first.

cipolleschi · 2022-10-10T08:31:20Z

+                if (
+                  propertyTypeAnnotation.type === 'FunctionTypeAnnotation' ||
+                  propertyTypeAnnotation.type === 'PromiseTypeAnnotation' ||
+                  propertyTypeAnnotation.type === 'VoidTypeAnnotation'
+                ) {


You can use const keys = Object.keys(UnsupportedObjectPropertyTypeToInvalidPropertyValueTypeMap) to obtain an array with all the keys. Then, you can create a new const invalidKeys = new Set(keys); to create a set from it and, finally use the set with something like:

if (invalidKeys.has(propertyTypeAnnotation.type)) { throw new error }

This avoids to explicitly list the three items. Imagine that we have to unsupport other objects, we will have to add other OR clauses which is not very elegant. 😉

cipolleschi · 2022-10-10T08:32:43Z

+  propertyKey,
+  type,
+) {
+  throw new UnsupportedObjectPropertyValueTypeAnnotationParserError(


I will put also the if logic in the function, s we can factor out more code.

Also, we can have a better name. Something like: throwIfPropertyValueTypeIsUnsupported. What do you think?

cipolleschi · 2022-10-10T08:33:29Z

-                    'Promise',
+                    invalidPropertyValueType,
                  );
+                } else {


You don't need the else. If the function throws, the execution flow changes, so we can safely remove this indentation! ;)

cipolleschi · 2022-10-10T08:34:10Z

+const UnsupportedObjectPropertyTypeToInvalidPropertyValueTypeMap = {
+  FunctionTypeAnnotation: 'FunctionTypeAnnotation',
+  VoidTypeAnnotation: 'void',
+  PromiseTypeAnnotation: 'Promise',
+};
+
+function throwUnsupportedObjectPropertyValueTypeAnnotationParserError(
+  moduleName,
+  propertyValue,
+  propertyKey,
+  type,
+) {
+  throw new UnsupportedObjectPropertyValueTypeAnnotationParserError(
+    moduleName,
+    propertyValue,
+    propertyKey,
+    type,
+  );
+}
+


Theoreticaly, this code should appear only in a single place. It is exactly the same of the Flow one. 😉

cipolleschi · 2022-10-10T08:35:33Z

+                if (
+                  propertyTypeAnnotation.type === 'FunctionTypeAnnotation' ||
+                  propertyTypeAnnotation.type === 'PromiseTypeAnnotation' ||
+                  propertyTypeAnnotation.type === 'VoidTypeAnnotation'
+                ) {
+                  const invalidPropertyValueType =
+                    UnsupportedObjectPropertyTypeToInvalidPropertyValueTypeMap[
+                      propertyTypeAnnotation.type
+                    ];
+
+                  throwUnsupportedObjectPropertyValueTypeAnnotationParserError(
                    hasteModuleName,
                    property.typeAnnotation.typeAnnotation,
                    property.key,
-                    'Promise',
+                    invalidPropertyValueType,
                  );
+                } else {
+                  return {
+                    name: key.name,
+                    optional,
+                    typeAnnotation: wrapNullable(
+                      isPropertyNullable,
+                      propertyTypeAnnotation,
+                    ),
+                  };
                }


with the suggested changes, this code will just become:

throwIfPropertyValueTypeIsUnsupported(/*params*/) return { name: key.name, optional, typeAnnotation: wrapNullable( isPropertyNullable, propertyTypeAnnotation, ), };

Hello @cipolleschi, I did the suggested changes but after doing them I get an error with flow as typeAnnotation expects a value of type NativeModuleBaseTypeAnnotation but the type returned by wrapNullable is of type NativeModuleTypeAnnotation. This was also the reason i used else statement before.

It was working before due to flow's type refinement because of the if condition and throw statement. I looked into the Flow documentation but couldn’t find a solution to it. Do you have any suggestions on how I should proceed?

That's... weird. :/
Let's keep the else, then..

cipolleschi · 2022-10-10T09:04:01Z

After the suggested refactoring, it would be great to have some unit tests, ideally, one for each unsupported case + 1 for a supported case where we check that the function does not throw.

cipolleschi · 2022-10-14T15:25:25Z

@dhruvtailor7 could you also rebase and resolve the conflicts, please? 🙏

cipolleschi · 2022-10-17T07:27:00Z

/rebase

facebook-github-bot · 2022-10-17T07:27:39Z

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

react-native-bot · 2022-10-19T08:45:05Z

This pull request was successfully merged by @dhruvtailor7 in aba6be6.

^{When will my fix make it into a release? | Upcoming Releases}

dhruvtailor7 added 2 commits October 10, 2022 12:56

extract UnsupportedObjectPropertyValueTypeAnnotationParserError to a …

de098b7

…throwing function.

typescript module parameter change

0619532

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 10, 2022

cipolleschi added the hacktoberfest-accepted label Oct 10, 2022

cipolleschi requested changes Oct 10, 2022

View reviewed changes

Merge branch 'main' into extract-unsupported-object-parse-error

572492e

dhruvtailor7 added 3 commits October 15, 2022 17:42

Merge branch 'main' into extract-unsupported-object-parse-error

2213b88

moved throwIfPropertyValueTypeIsUnsupported to error-utils

25f8aae

prettier fix

1bbf1c2

cortinico approved these changes Oct 18, 2022

View reviewed changes

facebook-github-bot closed this in aba6be6 Oct 19, 2022

react-native-bot added the Merged This PR has been merged. label Oct 19, 2022

AntoineDoubovetzky mentioned this pull request Oct 23, 2022

[Codegen] Extract throwIfUnsupportedFunctionParamTypeAnnotationParserError function #35057

Closed

Conversation

dhruvtailor7 commented Oct 10, 2022

Summary

Changelog

Test Plan

Uh oh!

analysis-bot commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

analysis-bot commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cipolleschi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhruvtailor7 Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cipolleschi commented Oct 10, 2022

Uh oh!

cipolleschi commented Oct 14, 2022

Uh oh!

cipolleschi commented Oct 17, 2022

Uh oh!

facebook-github-bot commented Oct 17, 2022

Uh oh!

react-native-bot commented Oct 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

analysis-bot commented Oct 10, 2022 •

edited

Loading

analysis-bot commented Oct 10, 2022 •

edited

Loading

dhruvtailor7 Oct 12, 2022 •

edited

Loading