[Codegen 101] Enrich getCommandProperties with the parser object

[kyawthura-gg]

Summary

[Codegen 101] The code of getCommandProperties is almost identical in Flow and TS. There are small differences between flow/ts, so we need for it to accept a Parser object. Enrich the parser object with the required methods if necessary.

Changelog:

[Internal] [Changed] - Enrich getCommandProperties with the parser object

Test Plan

yarn test react-native-codegen

[cipolleschi]

That's a very good starting points.
There are a few other improvements we can do:

1. Flow, line 126 (it doesn't let me add a comment at that line) and Typescript, line 126: that check can go in the Parser object. So the Flow parser can return InterfaceDeclaration and the TS parser can return TSInterfaceDeclaration
2. Line 134: same as above. Parser can have a bodyProperties function that, given a typeAlias returns body.properties for Flow and body.body for TS.
3. Line 141: we can add the propertyNames function to the parsers that extract the right array of property names.
4. After these changes, the two functions should be actually identical and we can move it in a single file in parser-commons.js, for example.

What do you think?

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,519,533	+0
android	hermes	armeabi-v7a	7,835,743	+0
android	hermes	x86	8,998,692	+0
android	hermes	x86_64	8,854,863	+0
android	jsc	arm64-v8a	9,140,865	+0
android	jsc	armeabi-v7a	8,333,013	+0
android	jsc	x86	9,194,556	+0
android	jsc	x86_64	9,453,649	+0

Base commit: d9f7c4c
Branch: main

[kyawthura-gg]

Moved propertyName to parser common file.
In Flow, from line 114 to 121 are almost the same as TypeScript. If we like to move those lines, propertyName function will need to accept language param. Is this good idea? Let me know what you think.

[cipolleschi]

Yes and no. If property name has to switch over the language, it should not accept a language parameter, but it should accept a Parser. The parser encapsulates the language specific logic, so the TypeScriptParser knows how to return something for TypeScript and the FlowParser knows how to work with Flow.

Does this make sense?

[kyawthura-gg]

I meant these lines. Sorry for confusion.

In Flow,

  let properties;
  try {
    properties = typeAlias.body.properties;
  } catch (e) {
    throw new Error(
      `Failed to find type definition for "${commandTypeName}", please check that you have a valid codegen flow file`,
    );
  }

In TypeScript,

  let properties;
  try {
    properties = typeAlias.body.body;
  } catch (e) {
    throw new Error(
      `Failed to find type definition for "${commandTypeName}", please check that you have a valid codegen typescript file`,
    );
  }

[cipolleschi]

yes, the check should become for both of them:

let properties = parser.bodyProperties(typeAlias);
if (!properties) {
  throw new ....
}

[kyawthura-gg]

Added bodyProperties

[cipolleschi]

Great job! It looks good to me!

[facebook-github-bot]

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

[cipolleschi]

/rebase

[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 969a8d0.