[Codegen] Extract the parseFile function in the typescript and flow parsers #35318

MaeIg · 2022-11-12T14:33:28Z

Summary

This PR aims to extract the parseFile function in the typescript and flow parsers. This is to solve the problem described here and help with the work done in #34872.

Changelog

[Internal] [Changed] - Extract the parseFile function in the typescript and flow parsers

Test Plan

yarn flow:

yarn lint:

yarn test:

cipolleschi

There are many changes on several files, but it looks very good to me. I like that we are centralizing the shared logic in a specific object.

Thank you so much for taking care of this.

facebook-github-bot · 2022-11-12T14:38:16Z

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

cipolleschi · 2022-11-12T14:38:59Z

There is a problem with the BUCK tests, though... :/

MaeIg · 2022-11-12T14:41:17Z

I think there is a circular dependency problem :/

When running NODE_OPTIONS="--trace-warnings" yarn lint:

MaeIg · 2022-11-12T21:12:33Z

There is a problem with the BUCK tests, though... :/

@cipolleschi, I investigated a bit and I think the errors come from circular imports:

In FlowParser, we import buildSchema from packages/react-native-codegen/src/parsers/flow/index.js
In this file, we import buildModuleSchema from packages/react-native-codegen/src/parsers/flow/modules/index.js
In this file, we import FlowParser

It will be solved in #35147. But, I think the problem can come back later and I don't know how to fix it.

Do you know how we could avoid these circular imports? Aren't we importing the parser in too many places? Or, maybe moving parseFile into the FlowParser/Typescript parser was just a bad idea.

cipolleschi · 2022-11-15T16:28:09Z

The circular dependency problem has landed on main, you can safely rebase on it and it should fix the problems!

cipolleschi · 2022-11-17T13:49:01Z

Hi @MaeIg, could you rebase this, whenever you have some time, please? 🙏

MaeIg · 2022-11-18T15:17:03Z

Hi @MaeIg, could you rebase this, whenever you have some time, please? 🙏

Hi, sorry I was pretty busy this week. I will do it tomorrow!

MaeIg · 2022-11-20T23:41:25Z

I rebased but there is still a circular dependency:

(there is the same for typescript)

It seems complicated to undo these circular dependencies. I wonder if it's time to extract parseFile in the parser. Maybe I should do the other task without it.

cipolleschi · 2022-11-21T14:23:17Z

What if we don't require the FlowParser in the flow/module/index.js file, but we just pass it as a parameter to the methods that need it? 🤔 We can just import type, if we want to be stricter with the types, or just ask for a Parser. At this point, the parsers/index.js module can create the FlowParser and pass it to the methods but we break the require depenencies.
What do you think?

MaeIg · 2022-11-21T21:27:12Z

What if we don't require the FlowParser in the flow/module/index.js file, but we just pass it as a parameter to the methods that need it? 🤔 We can just import type, if we want to be stricter with the types, or just ask for a Parser. At this point, the parsers/index.js module can create the FlowParser and pass it to the methods but we break the require depenencies. What do you think?

I will try to do it. It will make a lot of changes but this rule should avoid circular dependencies in the future!

cipolleschi · 2022-11-24T17:25:16Z

@Pranav-yadav, sorry if I reply here and I haven't replied in the other PR. I understand what you say and it makes a lot of sense. I'm quite scared about how big the file will ends up. It could become very hard to read an navigate. 🤔 That's the main reason why I was pushing to keep the files separated.

However, we currently have a different circular dependency: it is not related to parsers-commons and -primitives but to the Parser itself and the index files.

MaeIg · 2022-11-25T20:58:23Z

Hi @cipolleschi,

After a hard fight I ended up overcoming these circular dependencies! 🎉

I recommend you to review commit by commit to understand well the refacto 😃

Here's a diagram I created to understand what to do. Maybe it can help you to review!

analysis-bot · 2022-11-25T21:08:51Z

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

Base commit: 48966eb
Branch: main

analysis-bot · 2022-11-25T21:15:04Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,110,479	+0
android	hermes	armeabi-v7a	6,479,306	+0
android	hermes	x86	7,529,356	+0
android	hermes	x86_64	7,387,603	+0
android	jsc	arm64-v8a	8,976,992	+0
android	jsc	armeabi-v7a	7,708,149	+0
android	jsc	x86	9,040,016	+0
android	jsc	x86_64	9,517,383	+0

Base commit: 48966eb
Branch: main

pull-bot · 2022-11-25T21:22:15Z

PR build artifact for 6cede0d 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 · 2022-11-25T21:28:33Z

PR build artifact for 6cede0d 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

Hi @MaeIg, thank you endlessly for taking care of this and for all these changes. 🙏

I left some comments to improve this even further, but it already looks extremely good to me.

packages/eslint-plugin-specs/react-native-modules.js

packages/react-native-codegen/src/parsers/typescript/buildSchema.js

pull-bot · 2022-11-28T17:30:18Z

PR build artifact for bf9f2f6 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 · 2022-11-28T17:32:42Z

PR build artifact for bf9f2f6 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 · 2022-11-28T19:57:20Z

PR build artifact for eabd58d 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 · 2022-11-28T20:04:33Z

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

facebook-github-bot · 2022-11-30T11:40:40Z

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

cipolleschi · 2022-11-30T12:06:42Z

This PR needs some changes internally. Unfortunately, I don't think I'll be able to to them today, so we will merge it in a week. I'm sorry for this. :(

MaeIg · 2022-11-30T16:28:11Z

This PR needs some changes internally. Unfortunately, I don't think I'll be able to to them today, so we will merge it in a week. I'm sorry for this. :(

No problem. Take your time and feel free to ping me if there is anything I can do! :)

I rebased on main!

pull-bot · 2022-11-30T16:59:50Z

PR build artifact for 6de5740 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 · 2022-11-30T17:07:37Z

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

…arsers

… other functions that consume it

…on and other functions that consume it

It is already accessible in parser

…ions that consume it

…hat consume it

It avoid circular dependencies and it is temporary It will be moved again in facebook#35158

At least I tried... There were still circular dependencies... (node:58076) Warning: Accessing non-existent property 'getTypes' of module exports inside circular dependency (node:58076) Warning: Accessing non-existent property 'getValueFromTypes' of module exports inside circular dependency (node:58076) Warning: Accessing non-existent property 'getValueFromTypes' of module exports inside circular dependency (node:58076) Warning: Accessing non-existent property 'buildModuleSchema' of module exports inside circular dependency

MaeIg · 2022-12-08T17:18:03Z

Hello 👋
I rebased and resolved conflicts. Feel free to ping me if there is anything I can do

pull-bot · 2022-12-08T18:16:29Z

PR build artifact for 29505f7 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 · 2022-12-08T18:19:20Z

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

facebook-github-bot · 2022-12-13T09:47:57Z

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

github-actions · 2022-12-13T17:04:05Z

This pull request was successfully merged by @MaeIg in 3f2691c.

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

…cebook#35318) Summary: This PR aims to extract the parseFile function in the typescript and flow parsers. This is to solve the problem described [here](facebook#35158 (comment)) and help with the work done in facebook#34872. ## Changelog  [Internal] [Changed] - Extract the parseFile function in the typescript and flow parsers Pull Request resolved: facebook#35318 Test Plan: yarn flow: <img width="496" alt="image" src="https://user-images.githubusercontent.com/40902940/206518024-83084c3d-ab0d-4a04-810a-d40270add4b0.png"> yarn lint: <img width="495" alt="image" src="https://user-images.githubusercontent.com/40902940/206518076-9e07eafe-db61-4c6e-8aaa-f92f190cf4f3.png"> yarn test: <img width="389" alt="image" src="https://user-images.githubusercontent.com/40902940/206518118-5633b28c-b79b-4421-80f7-de1e03fb8ff2.png"> Reviewed By: cortinico Differential Revision: D41248581 Pulled By: cipolleschi fbshipit-source-id: f5b878a28a7de612fcdd1528f064b44f668503af

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 12, 2022

cipolleschi approved these changes Nov 12, 2022

View reviewed changes

This comment was marked as outdated.

Sign in to view

MaeIg force-pushed the refactor/move-parseFile-in-parser branch from ad6b929 to f28b428 Compare November 20, 2022 22:31

This comment was marked as resolved.

Sign in to view

MaeIg force-pushed the refactor/move-parseFile-in-parser branch from f28b428 to cb0a6be Compare November 25, 2022 12:09

MaeIg force-pushed the refactor/move-parseFile-in-parser branch from 6cede0d to bf9f2f6 Compare November 28, 2022 16:56

MaeIg requested a review from cipolleschi November 28, 2022 16:58

cipolleschi requested changes Nov 28, 2022

View reviewed changes

packages/eslint-plugin-specs/react-native-modules.js Outdated Show resolved Hide resolved

packages/react-native-codegen/src/parsers/typescript/buildSchema.js Show resolved Hide resolved

MaeIg force-pushed the refactor/move-parseFile-in-parser branch from bf9f2f6 to eabd58d Compare November 28, 2022 18:52

MaeIg requested a review from cipolleschi November 28, 2022 19:10

MaeIg force-pushed the refactor/move-parseFile-in-parser branch from eabd58d to 6de5740 Compare November 30, 2022 16:24

MaeIg added 11 commits December 8, 2022 17:49

refactor: only import buildSchema in tests

8582e20

refactor: move parseFile in typescript and flow parsers

02ffd12

refactor: remove buildSchema from parseFile params and import it in p…

c6d43a9

…arsers

type: use @flow strict

0b72a27

refactor(flow): pass parser as a param to translateTypeAnnotation and…

e64de34

… other functions that consume it

refactor(typescript): pass parser as a param to translateTypeAnnotati…

793fcdd

…on and other functions that consume it

refactor: remove useless language properties

9ba9967

It is already accessible in parser

refactor: pass parser as a param to buildModuleSchema and other funct…

7c59203

…ions that consume it

refactor: pass parser as a param to buildSchema and other functions t…

f510934

…hat consume it

refactor: move buildSchema into utils file

1672d29

It avoid circular dependencies and it is temporary It will be moved again in facebook#35158

MaeIg force-pushed the refactor/move-parseFile-in-parser branch from 6de5740 to 29505f7 Compare December 8, 2022 17:08

cipolleschi approved these changes Dec 13, 2022

View reviewed changes

facebook-github-bot closed this in 3f2691c Dec 13, 2022

github-actions bot added the Merged This PR has been merged. label Dec 13, 2022

MaeIg deleted the refactor/move-parseFile-in-parser branch December 14, 2022 19:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Codegen] Extract the parseFile function in the typescript and flow parsers #35318

[Codegen] Extract the parseFile function in the typescript and flow parsers #35318

MaeIg commented Nov 12, 2022 •

edited

Loading

cipolleschi left a comment

facebook-github-bot commented Nov 12, 2022

cipolleschi commented Nov 12, 2022

MaeIg commented Nov 12, 2022 •

edited

Loading

MaeIg commented Nov 12, 2022 •

edited

Loading

This comment was marked as outdated.

cipolleschi commented Nov 15, 2022

cipolleschi commented Nov 17, 2022

MaeIg commented Nov 18, 2022

MaeIg commented Nov 20, 2022

cipolleschi commented Nov 21, 2022

MaeIg commented Nov 21, 2022 •

edited

Loading

This comment was marked as resolved.

cipolleschi commented Nov 24, 2022

MaeIg commented Nov 25, 2022

analysis-bot commented Nov 25, 2022 •

edited

Loading

analysis-bot commented Nov 25, 2022 •

edited

Loading

pull-bot commented Nov 25, 2022

pull-bot commented Nov 25, 2022

cipolleschi left a comment

pull-bot commented Nov 28, 2022

pull-bot commented Nov 28, 2022

pull-bot commented Nov 28, 2022

pull-bot commented Nov 28, 2022

facebook-github-bot commented Nov 30, 2022

cipolleschi commented Nov 30, 2022

MaeIg commented Nov 30, 2022

pull-bot commented Nov 30, 2022

pull-bot commented Nov 30, 2022

MaeIg commented Dec 8, 2022 •

edited

Loading

pull-bot commented Dec 8, 2022

pull-bot commented Dec 8, 2022

facebook-github-bot commented Dec 13, 2022

github-actions bot commented Dec 13, 2022

[Codegen] Extract the parseFile function in the typescript and flow parsers #35318

[Codegen] Extract the parseFile function in the typescript and flow parsers #35318

Conversation

MaeIg commented Nov 12, 2022 • edited Loading

Summary

Changelog

Test Plan

cipolleschi left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 12, 2022

cipolleschi commented Nov 12, 2022

MaeIg commented Nov 12, 2022 • edited Loading

MaeIg commented Nov 12, 2022 • edited Loading

This comment was marked as outdated.

cipolleschi commented Nov 15, 2022

cipolleschi commented Nov 17, 2022

MaeIg commented Nov 18, 2022

MaeIg commented Nov 20, 2022

cipolleschi commented Nov 21, 2022

MaeIg commented Nov 21, 2022 • edited Loading

This comment was marked as resolved.

cipolleschi commented Nov 24, 2022

MaeIg commented Nov 25, 2022

analysis-bot commented Nov 25, 2022 • edited Loading

analysis-bot commented Nov 25, 2022 • edited Loading

pull-bot commented Nov 25, 2022

pull-bot commented Nov 25, 2022

cipolleschi left a comment

Choose a reason for hiding this comment

pull-bot commented Nov 28, 2022

pull-bot commented Nov 28, 2022

pull-bot commented Nov 28, 2022

pull-bot commented Nov 28, 2022

facebook-github-bot commented Nov 30, 2022

cipolleschi commented Nov 30, 2022

MaeIg commented Nov 30, 2022

pull-bot commented Nov 30, 2022

pull-bot commented Nov 30, 2022

MaeIg commented Dec 8, 2022 • edited Loading

pull-bot commented Dec 8, 2022

pull-bot commented Dec 8, 2022

facebook-github-bot commented Dec 13, 2022

github-actions bot commented Dec 13, 2022

MaeIg commented Nov 12, 2022 •

edited

Loading

MaeIg commented Nov 12, 2022 •

edited

Loading

MaeIg commented Nov 12, 2022 •

edited

Loading

MaeIg commented Nov 21, 2022 •

edited

Loading

analysis-bot commented Nov 25, 2022 •

edited

Loading

analysis-bot commented Nov 25, 2022 •

edited

Loading

MaeIg commented Dec 8, 2022 •

edited

Loading