[Codegen 116] move getEventArgument function into parsers-commons.js

[siddarthkay]

Summary:

[Codegen 116] This PR attempts to extract the logic of getEventArgument function from the following locations :

• parsers/flow/components/events.js
• parsers/typescript/components/events.js
  since they are the same and move the function to parsers/parsers-commons.js as requested on ☂️ Improving Codegen #34872

Changelog:

[Internal] [Changed] - Move getEventArgument to parser-commons and update usages.

Test Plan:

Run yarn jest react-native-codegen and ensure CI is green

[siddarthkay]

Something to think about while moving getEventArgument to parser-commons.js :

• The function getEventArgument depends on function buildPropertiesForEvent.
• The function buildPropertiesForEvent depends on function getPropertyType .
• and finally the function getPropertyType depends on buildPropertiesForEvent and parseTopLevelType
• Now buildPropertiesForEvent would be available if we took care of step 2
• And getPropertyType is exported so it should be available in parser-commons.js

[siddarthkay]

What I could do is pass a parser instance to getEventArgument and then implement buildPropertiesForEvent for flow and typescript parsers.

• Implement equivalent of buildPropertiesForEvent in parser/flow/parser.js
• Implement equivalent of buildPropertiesForEvent in parser/typescript/parser.js

buildPropertiesForEvent depends on getPropertyType so we have to implement them as well in flow and typescript parsers.

• Implement equivalent of getPropertyType in parser/flow/parser.js
• Implement equivalent of getPropertyType in parser/typescript/parser.js

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,639,005	-1
android	hermes	armeabi-v7a	7,949,793	+1
android	hermes	x86	9,126,347	-3
android	hermes	x86_64	8,979,756	+1
android	jsc	arm64-v8a	9,202,924	-1
android	jsc	armeabi-v7a	8,391,167	+1
android	jsc	x86	9,261,616	+0
android	jsc	x86_64	9,518,660	-2

Base commit: 4fd8f40
Branch: main

[siddarthkay]

@cipolleschi @rshest : All checks have passed, This PR is now ready for review.

[cipolleschi]

Hi @siddarthkay, thank you so much for this PR.
I think that the work is very good, but these changes are a bit too disruptive, for the time being, considering also all the other people manipulating similar files.
Although I agree that in principle getPropertyType could be moved to the two parsers (and probably, eventually, it will be) I think that the task should be smaller, with fewer changes. We are taking an iterative approach, manipulating the files so that we will end up in the desired state with small steps.

What I had in mind, was to just move the function getEventArgument(argumentProps, name: $FlowFixMe) to the parsers.
Now, I see that:

1. the function takes a name parameter which is not used => we can remove it
2. the function depends on buildPropertiesForEvent => we can pass it as a parameter.

So, the new signature (to be used in the parsers) would be

function getEventArgument(argumentProps: $FlowFixMe, buildPropertiesForEvent: (propert) => NamedShape<EventTypeAnnotation>)

At this point, you don't have to refactor multiple functions and move all that code around.
In the callsite, you just replace the invocation with:

- getEventArgument(argumentProps, name),
+ getEventArgument(argumentProps, buildPropertiesForEvent), 

Does it make sense? how does it feel?

[siddarthkay]

Hi @cipolleschi : Thanks for taking the time to go through the PR and suggesting an easier way to do this.
I agree with iterative approach and making small changes, I too at first thought these changes are too disruptive and hoped to have that conversation after making changes in this PR.

I'll revert back my changes, rebase main and update the function signature of getEventArgument to pass in getEventArgument .

[cipolleschi]

Thanks again! One last small change and we are good to import the PR! :D

[siddarthkay]

i love resolving conflicts 😇

[facebook-github-bot]

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

[facebook-github-bot]

@cipolleschi merged this pull request in d46f92c.