[Codegen 86] Use buildPropSchema from parser-commons

[ken0nek]

Summary:

The buildPropSchema function in parsers/typescript/components/props.js and parsers/flow/components/props.js is the same. move it to parser-commons and use it in the original files.

part of #34872

• Make the getTypeAnnotation signature from the Flow package to be equal to the typescript one. Specifically, the Typescript one needs an additional parameter withNullDefault that we can safely ignore in the implementation.
• buildPropSchema signature can be updated to accept those two functions in input as callbacks. Then, the getProps function can feed the right functions based on the language to the shared build prop schema.

ref: #34872 (comment)

Changelog:

[Internal] [Changed] - Use buildPropSchema from parser-commons

Test Plan:

• yarn jest react-native-codegen pass

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,723,630	+2
android	hermes	armeabi-v7a	8,034,458	-1
android	hermes	x86	9,210,700	-1
android	hermes	x86_64	9,065,039	-1
android	jsc	arm64-v8a	9,288,253	+0
android	jsc	armeabi-v7a	8,476,516	+0
android	jsc	x86	9,346,723	+1
android	jsc	x86_64	9,604,660	+2

Base commit: 33e0521
Branch: main

[facebook-github-bot]

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

[cipolleschi]

For what I can see, you are feeding the callbacks in the right way.

There are some issues with the typings, though, that we have to iron out. If you clic the CircleCI jobs, you can see the errors, like here.

For example:

1. in parser-commons, you are using ASTNode, which has not been declared in the file.
2. there are some incompatible return type we need to address. buildPropSchema for Flow returns ?NamedShape<PropTypeAnnotation>, while for typescript it returns NamedShape<PropTypeAnnotation> (non optional)
3. This error instead is more cryptic... I'll have to investigate it better.

[cipolleschi]

Ok, i figured out what's the problem. We basically have a circular reference that cannot be resolved by Flow.

Specifically:

1. buildPropSchema requires a function getTypeAnnotation
2. getTypeAnnotation requires a function buildSchema
3. when implementing buildPropSchema, we are calling getTypeAnnotation using buildPropSchema as the buildSchema parameter
4. So we have a cycle buildPropSchema => getTypeAnnotation => buildPropSchema => getTypeAnnotation...

I have to think about how to break that, but I don't think we can go down this path, unfortunately. :(

[cipolleschi]

I found a way to make it work. Basically, we have a problem here:

// in parser-commons.js
function buildPropSchema(
 // params
): ?NamedShape<PropTypeAnnotation> {
  // good lines...

  return {
    name,
    optional,
    typeAnnotation: getTypeAnnotation(
      name,
      typeAnnotation,
      defaultValue,
      withNullDefault,
      types,
      buildPropSchema, // <-- this don't work
    ),
  };
}

Looking better at the problem, the reason why it doesn't work is because getTypeAnnotation takes a parameter whose signature is (property: PropAST, types: TypeDeclarationMap) => $FlowFixMe, while we are trying to pass a parameter where the signature is (property: PropAST, types: TypeDeclarationMap, getSchemaInfo: GetSchemaInfoFN, getTypeAnnotation: getTypeAnnotationFN) => $FlowFixMe.

So, Flow is properly complaining that it is missing a couple of parameters to make it work. 😅

The way to solve this is to provide a function signature which matches. So, we can do something like:

// in parser-commons.js
function buildPropSchema(
  // params
  ) => $FlowFixMe,
): ?NamedShape<PropTypeAnnotation> {
  // good lines...
  
  const wrappedBuildSchema = (passedProps: PropAST, passedTypes: TypeDeclarationMap): $FlowFixMe => {
    buildPropSchema(passedProps, passedTypes, getSchemaInfo, getTypeAnnotation);
  }

  return {
    name,
    optional,
    typeAnnotation: getTypeAnnotation(
      name,
      typeAnnotation,
      defaultValue,
      withNullDefault,
      types,
      wrappedBuildSchema, // <-- this is different: it calls the function above, with all the params
    ),
  };
}

the execution of getTypeAnnotation will take care of passing the right PropAST and TypeDeclarationMap parameters to wrapped build schema, while the wrappedBuildSchema is capturing the two functions getSchemaInfo and getTypeAnnotation.

If why this is happening is not clear, I'll be happy to try and explain it better or in a different way! 😄

[ken0nek]

Thank you for the investigation and the suggestion!

Now, it is time to resolve the test failures... 💪

---

yarn jest react-native-codegen

Snapshot Summary
 › 13 snapshots failed from 4 test suites. Inspect your code changes or re-run jest with `-u` to update them.

Test Suites: 4 failed, 50 passed, 54 total
Tests:       13 failed, 2540 passed, 2553 total
Snapshots:   13 failed, 828 passed, 841 total
Time:        3.192 s

[ken0nek]

Error ----------------------------------- packages/react-native-codegen/src/parsers/typescript/components/props.js:112:7

Cannot return object literal because null or undefined [1] is incompatible with `NamedShape` [2] in array element of
property `props`. [incompatible-return]

   packages/react-native-codegen/src/parsers/typescript/components/props.js:112:7
   112|       buildPropSchema(property, types, getSchemaInfo, getTypeAnnotation),
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

References:
   packages/react-native-codegen/src/parsers/parsers-commons.js:845:4
   845| ): ?NamedShape<PropTypeAnnotation> {
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [1]
   packages/react-native-codegen/src/parsers/typescript/components/props.js:74:25
    74|   props: $ReadOnlyArray<NamedShape<PropTypeAnnotation>>,
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [2]

[ken0nek]

I took this code from flow one tho
https://github.com/facebook/react-native/pull/37043/files#diff-5cbd2ff460e869762c61aabc2adca0c30f77d52c181a2c1c807532e64ce0e9e2L28

[cipolleschi]

Hi @ken0nek, could you please:

• rebase on top of main
• resolve the merge conflicts
• and have a look at the other errors in CI?

Thank you so much!

[ken0nek]

Hi @ken0nek, could you please:

• rebase on top of main
• resolve the merge conflicts
• and have a look at the other errors in CI?

Thank you so much!

Sure!

Finished rebasing and resolving conflicts. Will look into the errors on CI.

[ken0nek]

Have to handle the nullability of getSchemaInfo

[cipolleschi]

@ken0nek gentle ping on this. Do you think you might be able to address these issues by the end of this week?

[ken0nek]

@ken0nek gentle ping on this. Do you think you might be able to address these issues by the end of this week?

Thank you for checking in.

Sorry to say but that might be difficult...

I don't want to block other codegen tasks, please take this task if possible 🙏

I wish I could contribute to the repository quickly and effectively...

[cipolleschi]

Given you make most of the work, I can import it and finish up the work myself. It won't take much. Is that ok for you? If yes, could you just set the PR as Ready For Review? 🙏
In that way, the commit would get attributed to you anyway. 😉

[ken0nek]

Is that ok for you?

Sure, thank you for your help and support!

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

[ken0nek]

Thank you for handling this pull request! Learned a lot by reading 0212179