[Codegen] Extract throwIfUnsupportedFunctionParamTypeAnnotationParserError function

[AntoineDoubovetzky]

Summary

This PR is a task from #34872:

Extract the UnsupportedFunctionParamTypeAnnotationParserError in its own throwing function (if it does not exists already) and reuse that function passing a proper type. Then, refactor the code using a dictionary and avoiding the three different ifs in both parsers.

Changelog

[Internal] [Changed] - Extract the UnsupportedFunctionParamTypeAnnotationParserError in its own throwing function in error-utils

Test Plan

[AntoineDoubovetzky]

@cipolleschi I still need to do the second part of the task ("Then, refactor the code using a dictionary and avoiding the three different ifs in both parsers."), but I have some troubles with Flow.

Flow doesn't understand anymore that paramTypeAnnotation can't be of type VoidTypeAnnotation nor PromiseTypeAnnotation.

I tried to use predicate functions but I didn't manage to make it work in this case. I don't like the idea of adding a $FlowFixMe comment.
Do you have any idea?

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,066,076	+73,448
android	hermes	armeabi-v7a	6,470,155	+101,198
android	hermes	x86	7,377,755	-27,147
android	hermes	x86_64	7,348,765	+79,576
android	jsc	arm64-v8a	8,923,088	+63,147
android	jsc	armeabi-v7a	7,689,029	+90,911
android	jsc	x86	8,869,884	-47,661
android	jsc	x86_64	9,462,806	+61,643

Base commit: cf5addf
Branch: main

[analysis-bot]

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

Base commit: cf5addf
Branch: main

[pull-bot]

PR build artifact for 3e1326d is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

[AntoineDoubovetzky]

I searched if there were other places where we encountered the same problem. I believe it is the case in this PR #34917, and I see that the if(XXX) is not in the throwIfXXX function, but I feel like it's losing the point of extracting the function.

[cipolleschi]

Hi @AntoineDoubovetzky, thanks for the comment.

I understand what you are trying to say and I completely agree. However, I think we have two orders of problems:

1. We have the repetition of the if (type == 'something') then throw for each of the invalid types
2. The logic you highlighted not in the throwIfXXX function.

To keep things as simple as possible, I tried to split the problem in two steps: the PR you linked was addressing only the first one and I think it's fine: we can have another task for the second step.

Does this reasoning make sense to you?

For your task, you can tackle both points together, if you feel it! 😊

[AntoineDoubovetzky]

I'll do the first one, for the second one I have a Flow error that I didn't manage to fix yet (#35057 (comment)).

[cipolleschi]

Amazing, thank you for taking this and to look through the codebase.
I left a comment to improve it further and it could perhaps solve the other problem as well.

[cipolleschi]

What about saving some more lines (deleting line from 453 to 459)with this:

Suggested change

	throwIfUnsupportedFunctionParamTypeAnnotationParserError(
	hasteModuleName,
	flowParam.typeAnnotation,
	paramTypeAnnotation.type,
	paramName,
	'PromiseTypeAnnotation',
	);
	const forbiddenTypes = new Set(['VoidTypeAnnotation', 'PromiseTypeAnnotation']);
	const currentType = paramTypeAnnotation.type;
	if (forbiddenTypes.has(currentType)) {
	throwIfUnsupportedFunctionParamTypeAnnotationParserError(
	hasteModuleName,
	flowParam.typeAnnotation,
	paramTypeAnnotation.type,
	paramName,
	currentType,
	);
	}

This is better because, if in the future we want to forbid more type, we can just work on the Set instead of make one call for each.
Perhaps, with this approach, you may also be able to solve the Flow problem.

What do you think?

[AntoineDoubovetzky]

I tried to use a Set but Flow couldn't infer that paramTypeAnnotation.type could not equal VoidTypeAnnotation or PromiseTypeAnnotation so I had the same Flow error when calling wrappNullable as before (#35057 (comment)).

The only solution I found is to replace the Set with multiple OR conditions and to add an early return.

[AntoineDoubovetzky]

@cipolleschi I updated the PR, I think this is now ready to merge

[facebook-github-bot]

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

[pull-bot]

PR build artifact for 232c852 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

[react-native-bot]

This pull request was successfully merged by @AntoineDoubovetzky in 8c69b6c.

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