[Codegen] Extract the buildSchema function in the parsers-commons.js

[MaeIg]

Summary

This PR aims to extract the buildSchema function into parsers-commons that is shared between typescript and flow. It is a task of #34872:

Extract the buildSchema function (Flow, TypeScript) in the parsers-commons.js file to a top level buildSchema function which takes additional parameters to properly parse the content, get the config type and to build the schema, based on the language used.

Changelog

[Internal] [Changed] - Extract the buildSchema function in the parsers-commons.js

Test Plan

yarn test:

yarn flow:

yarn lint:

[MaeIg]

Hello @cipolleschi,

I don't know what to do to move forward.

I extracted the buildSchema function and used the new parser which implements Parser interface. The problem is that ~15 tests do something like this:

I changed the buildSchema params so I would have to adapt the tests but parseFile has very few logic:

So, for me, parseFile should be moved to FlowParser and TypescriptParser (it seems easier to do it now rather than changing parseFile and all tests to respect new buildSchema interface). So we can do something like:

const schema = RealFlowParser.parsefile(`${FIXTURE_DIR}/${fixture}`);

instead of:

const schema = parseFile(
      `${FIXTURE_DIR}/${fixture}`,
      DeprecatedFlowParser.buildSchema,
    );

What do you think of that ?

For me the best plan is:

1. I open a new PR to move parseFile in FlowParser and TypeScriptParser
2. Once the new PR is merged, I rebase this branch on main and I adapt parseFile from FlowParser and TypeScriptParser
3. All tests should pass and parseFile don't need to take 5 extra args to pass it to buildSchema 🎉

[cipolleschi]

Amazing job! 👏
I left a few comments to improve typing, but I love how we are putting code together.

[cipolleschi]

Could you also verify why it is failing in CI? 🙏

[MaeIg]

Thanks the review! 😃 I will address the comments!

Could you also verify why it is failing in CI? 🙏

@cipolleschi I explained why it's failing here: #35158 (comment)
I would like to have your opinion on what I proposed when you have time to read it.

[cipolleschi]

Hi @MaeIg, sorry for the long delay. :(

The plan sounds good and reasonable. parseFile is also general enough that we may be able not to repeat the implementation in the two parsers, but perhaps keep it as an high-level function in the parsers-commons.js file. What do you think?

The reason why I haven't suggested that is because I believe parseFile, with that signature and from that path, is used in some other files and potentially also internally. That's why it could be tricky to move it.

Could you create another pr with just that change? In that way, I can import it and see what get broken...

Thank you so much for this effort! 🙏

[cipolleschi]

The fix for the circular references landed, so you can rebase, fix the conflict and we can push this forward! :D

[cipolleschi]

Hi @MaeIg! Could you please rebase this PR on top of main? 🙏 Thank you so much.

[MaeIg]

Hello @cipolleschi,

Sorry for the delay, I was sick :/ I extracted the buildSchema function but there is a circular dependency. I will try to fix it next week!

[analysis-bot]

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

Base commit: efd39ee
Branch: main

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,464,380	+0
android	hermes	armeabi-v7a	7,784,752	+0
android	hermes	x86	8,937,785	+0
android	hermes	x86_64	8,796,124	+0
android	jsc	arm64-v8a	9,650,798	+0
android	jsc	armeabi-v7a	8,384,800	+0
android	jsc	x86	9,712,790	+0
android	jsc	x86_64	10,190,369	+0

Base commit: 66927ec
Branch: main

[pull-bot]

PR build artifact for b5df7e8 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.

[pull-bot]

PR build artifact for b5df7e8 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.

[pull-bot]

PR build artifact for 1ca77ee 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.

[pull-bot]

PR build artifact for 1ca77ee 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.

[cipolleschi]

Sorry to bother, but it needs another rebase + conflict fixing. :(

[MaeIg]

Sorry to bother, but it needs another rebase + conflict fixing. :(

Hello @cipolleschi , it's done! 😄

[pull-bot]

PR build artifact for ba1c5d1 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.

[pull-bot]

PR build artifact for ba1c5d1 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.

[cipolleschi]

Thank you for doing this.

I left a few comment to simplify the code a bit. The main suggestion to implement, imho, is to have the parseModuleFixture call parseString rather then invoking the buildSchema directly.

Potentially, we can completely get rid of the index.js file.

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