[Codegen]: Refactor `buildPropertySchema` fn of (Flow, TS) into common fn #35288

Pranav-yadav · 2022-11-09T14:35:46Z

Summary

This PR is a task of #34872

combined Flow and TS buildPropertySchema fn 's into common fn
added callback param resolveTypeAnnotation to the same
moved it to parsers-commons.js
re-organized imports and exports

Changelog

[INTERNAL] [CHANGED] - [Codegen]: Refactored buildPropertySchema fn of (Flow, TS) into common fn in parsers-commons.js

Test Plan

ensure 👇 is #00ff00

yarn lint && yarn flow && yarn test-ci

cipolleschi

Great job moving this, thank you so much!

facebook-github-bot · 2022-11-12T11:09:45Z

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

cipolleschi · 2022-11-14T15:32:20Z

Hi @Pranav-yadav!
We should try to fix this circular dependency.
I think that is something that can happen when working in so many people on a refactoring of this size.

I'll try to figure out what's going on and I'll suggest a fix before EOD.

shall I remove the [skip ci] tag from commit msg as it might skip internal workflow/ci?

Yes, please, it's better to have some workflows that fails rather then not having at all.

cipolleschi · 2022-11-14T17:01:19Z

To solve the circular dependency, I think we need to move translateFunctionTypeAnnotation from parsers-primitives,js to parsers-commons.js.

The problem is that parsers-commons is importing parsers-primitives to access that function. parsers-primitives, is importing wrapNullable (and other functions) from parsers-commons and this generates an endless importing loop.

If we break it as described above, it should work.

We should probably define some clear rules about what should go in parsers-primitives and what should go in parsers-commons... 🤔 Probably, it's better to put in primitives all the basic building blocks and commons all the utility functions, so that primitives can import commons but vice-versa should not happen.

What do you think?

cipolleschi · 2022-11-14T17:21:38Z

I understand. I can try myself tomorrow morning.

Pranav-yadav · 2022-11-14T17:43:54Z

I think It's solved on my local, just finishing tests and chores..

Edit: Finished 👇

Dunno where these ☝️ warns were hiding till now 😅

yarn jest react-native-codegen

yarn run v1.22.19
$ flow
No errors!
Done in 0.19s.

yarn test-ci

AntoineDoubovetzky · 2022-11-14T17:47:48Z

@Pranav-yadav Looks like you found a solution to your error. If you encounter an error again, I am available to look at it together.

Pranav-yadav · 2022-11-14T18:17:37Z

Here we go #35343 <-- this should fix the problem(s) :)
Thanks both of you

…#35343) Summary: This PR should solve the `"Circular Dependencies"` problem/issue because of which some PRs are getting blocked as discussed here #35288 (comment) - also moved below helpers to `parsers-commons.js`; - `getTypeAnnotationParameters` - `getFunctionNameFromParameter` - `getParameterName` - `getParameterTypeAnnotation` - `getTypeAnnotationReturnType` <3 ## Changelog [INTERNAL] [CHANGED] - Moved `translateFunctionTypeAnnotation` fn from `parsers-primitives.js` to `parsers-commons.js` also, moved it's helpers Pull Request resolved: #35343 Test Plan: - ensure 👇 is `#00ff00` `yarn lint && yarn flow && yarn test-ci` Reviewed By: christophpurrer Differential Revision: D41273191 Pulled By: rshest fbshipit-source-id: cc1839a91579e7914f05516a90b280a776510c9d

Summary: This is a task of facebook#34872 - combined `Flow` and `TypeScript` `buildPropertySchema` fn 's into single common - uses callback param `resolveTypeAnnotation` - moved it to `parsers-commons.js` - re-organize imports and exports

facebook-github-bot · 2022-11-15T16:12:18Z

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

analysis-bot · 2022-11-15T16:27:10Z

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

Base commit: 64ff077
Branch: main

analysis-bot · 2022-11-15T16:29:11Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,099,332	+12
android	hermes	armeabi-v7a	6,468,970	+0
android	hermes	x86	7,517,173	+7
android	hermes	x86_64	7,375,843	+6
android	jsc	arm64-v8a	8,963,903	-148
android	jsc	armeabi-v7a	7,695,294	-151
android	jsc	x86	9,026,402	-138
android	jsc	x86_64	9,504,545	-131

Base commit: 64ff077
Branch: main

Pranav-yadav · 2022-11-15T16:29:53Z

We should probably define some clear rules about what should go in parsers-primitives and what should go in parsers-commons... 🤔 Probably, it's better to put in primitives all the basic building blocks and commons all the utility functions, so that primitives can import commons but vice-versa should not happen.

What do you think?

Definitely. Block comments should be added at top of both parsers-commons.js and parsers-primitives.js stating their purpose, functionalities, and what NOT to include in those files.. :) Also, It would be very useful for newbie contributors like me 👶 as well as existing..
Thanks

pull-bot · 2022-11-15T16:39:30Z

PR build artifact for 0cdea0e 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-15T16:41:16Z

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

Pranav-yadav · 2022-11-15T18:32:22Z

@cipolleschi did u notice something strange about @react-native-bot ?
It stopped labelling merged PRs Merged, from last week. I hope the bot is okay :) <3
#offtopic :)

cipolleschi · 2022-11-16T13:03:16Z

@Pranav-yadav thank you for reporting this. We have to look into that bot, I think.

Pranav-yadav · 2022-11-18T14:17:15Z

We should probably define some clear rules about what should go in parsers-primitives and what should go in parsers-commons... 🤔 Probably, it's better to put in primitives all the basic building blocks and commons all the utility functions, so that primitives can import commons but vice-versa should not happen.
What do you think? --- Riccardo

Definitely. Block comments should be added at top of both parsers-commons.js and parsers-primitives.js stating their purpose, functionalities, and what NOT to include in those files.. :) Also, It would be very useful for newbie contributors like me 👶 as well as existing.. Thanks --- Pranav

@cipolleschi After giving it another thought, I think we should merge those two files. Reason: Cohesion

Though, (logically) it's correct to keep them separate, the fns/types/interfaces exported by them are not used sparsely (at different places).
We know that, they are highly related to each other (highly cohesive => Good 🙂) and keeping logic together will be better as well as easy to maintain.

Thoughts?

PS: sorry for commenting under closed PR

…facebook#35343) Summary: This PR should solve the `"Circular Dependencies"` problem/issue because of which some PRs are getting blocked as discussed here facebook#35288 (comment) - also moved below helpers to `parsers-commons.js`; - `getTypeAnnotationParameters` - `getFunctionNameFromParameter` - `getParameterName` - `getParameterTypeAnnotation` - `getTypeAnnotationReturnType` <3 ## Changelog [INTERNAL] [CHANGED] - Moved `translateFunctionTypeAnnotation` fn from `parsers-primitives.js` to `parsers-commons.js` also, moved it's helpers Pull Request resolved: facebook#35343 Test Plan: - ensure 👇 is `#00ff00` `yarn lint && yarn flow && yarn test-ci` Reviewed By: christophpurrer Differential Revision: D41273191 Pulled By: rshest fbshipit-source-id: cc1839a91579e7914f05516a90b280a776510c9d

…ook#35288) Summary: This PR is a task of facebook#34872 - combined `Flow` and `TS` `buildPropertySchema` fn 's into common fn - added callback param `resolveTypeAnnotation` to the same - moved it to `parsers-commons.js` - re-organized imports and exports ## Changelog [INTERNAL] [CHANGED] - [Codegen]: Refactored `buildPropertySchema` fn of (Flow, TS) into common fn in `parsers-commons.js` Pull Request resolved: facebook#35288 Test Plan: - ensure 👇 is `#00ff00` ```bash yarn lint && yarn flow && yarn test-ci ``` Reviewed By: christophpurrer Differential Revision: D41247738 Pulled By: cipolleschi fbshipit-source-id: aecc0ed8d07efa1c2c39e8a8e64b4ee73b720b8f

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 9, 2022

Pranav-yadav force-pushed the refactor/buildPropertySchema branch 3 times, most recently from aff6da6 to a722b1a Compare November 12, 2022 08:29

Pranav-yadav marked this pull request as ready for review November 12, 2022 08:31

This comment was marked as resolved.

Sign in to view

cipolleschi approved these changes Nov 12, 2022

View reviewed changes

Pranav-yadav mentioned this pull request Nov 13, 2022

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

Closed

This comment was marked as outdated.

Sign in to view

Pranav-yadav force-pushed the refactor/buildPropertySchema branch from a722b1a to 76e01d4 Compare November 14, 2022 15:53

This comment was marked as resolved.

Sign in to view

Pranav-yadav mentioned this pull request Nov 14, 2022

Chore: mv translateFunctionTypeAnnotation fn > parsers-commons.js #35343

Closed

Pranav-yadav force-pushed the refactor/buildPropertySchema branch from 76e01d4 to 0cdea0e Compare November 15, 2022 16:06

facebook-github-bot closed this in fb37dea Nov 16, 2022

Pranav-yadav deleted the refactor/buildPropertySchema branch November 16, 2022 17:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Codegen]: Refactor `buildPropertySchema` fn of (Flow, TS) into common fn #35288

[Codegen]: Refactor `buildPropertySchema` fn of (Flow, TS) into common fn #35288

Pranav-yadav commented Nov 9, 2022 •

edited

Loading

This comment was marked as resolved.

cipolleschi left a comment

facebook-github-bot commented Nov 12, 2022

This comment was marked as outdated.

cipolleschi commented Nov 14, 2022

cipolleschi commented Nov 14, 2022

This comment was marked as resolved.

cipolleschi commented Nov 14, 2022

Pranav-yadav commented Nov 14, 2022 •

edited

Loading

AntoineDoubovetzky commented Nov 14, 2022 •

edited

Loading

Pranav-yadav commented Nov 14, 2022

facebook-github-bot commented Nov 15, 2022

analysis-bot commented Nov 15, 2022

analysis-bot commented Nov 15, 2022

Pranav-yadav commented Nov 15, 2022

pull-bot commented Nov 15, 2022

pull-bot commented Nov 15, 2022

Pranav-yadav commented Nov 15, 2022

cipolleschi commented Nov 16, 2022

Pranav-yadav commented Nov 18, 2022

[Codegen]: Refactor buildPropertySchema fn of (Flow, TS) into common fn #35288

[Codegen]: Refactor buildPropertySchema fn of (Flow, TS) into common fn #35288

Conversation

Pranav-yadav commented Nov 9, 2022 • edited Loading

Summary

Changelog

Test Plan

This comment was marked as resolved.

cipolleschi left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 12, 2022

This comment was marked as outdated.

cipolleschi commented Nov 14, 2022

cipolleschi commented Nov 14, 2022

This comment was marked as resolved.

cipolleschi commented Nov 14, 2022

Pranav-yadav commented Nov 14, 2022 • edited Loading

AntoineDoubovetzky commented Nov 14, 2022 • edited Loading

Pranav-yadav commented Nov 14, 2022

facebook-github-bot commented Nov 15, 2022

analysis-bot commented Nov 15, 2022

analysis-bot commented Nov 15, 2022

Pranav-yadav commented Nov 15, 2022

pull-bot commented Nov 15, 2022

pull-bot commented Nov 15, 2022

Pranav-yadav commented Nov 15, 2022

cipolleschi commented Nov 16, 2022

Pranav-yadav commented Nov 18, 2022

[Codegen]: Refactor `buildPropertySchema` fn of (Flow, TS) into common fn #35288

[Codegen]: Refactor `buildPropertySchema` fn of (Flow, TS) into common fn #35288

Pranav-yadav commented Nov 9, 2022 •

edited

Loading

Pranav-yadav commented Nov 14, 2022 •

edited

Loading

AntoineDoubovetzky commented Nov 14, 2022 •

edited

Loading