[Codegen 78 & 79]: Merge TS & Flow parsers' logic for `Partial` case to `emitPartial` fn & `commonTypes` cases into `emitCommonTypes` fn #36450

Pranav-yadav · 2023-03-12T12:15:37Z

Summary

[Codegen 78 - Assigned to @Pranav-yadav] It depends on [Codegen 75][Codegen 76][Codegen 77] Extract the logic that emits Partial values in an emitPartial function, which takes the Parsers as parameter.

[Codegen 79 - Assigned to @Pranav-yadav] It depends on [Codegen 78] Extract the basic cases logics (case Stringish, case Int32, case Double, ..., case Partial. Flow lines and TypeScript lines into a function emitCommonTypes in parsers-primitives.js. Make sure that the default case returns null. Call this function in the default: case Flow, TypeScript of the index.js file: if the function return something, return that from the default case; otherwise if the emitCommonTypes returns null, keep the current default implementation (throw the error).

Changes

merged TS & Flow parsers' logic for Partial case to emitPatial fn
merged TS & Flow parsers' logic for below cases:
- Stringish
- Int32
- Double
- Float
- UnsafeObject
- Object
- Partial
into an emitCommonTypes fn into parsers-primitives.js
add tests for emitPartial and emitCommonTypes fn's
add getAnnotatedElementProperties fn to parser & impl to both TS & Flow parsers

Changelog

[INTERNAL] [CHANGED] - Merge TS & Flow parsers' logic for Partial case to emitPatial fn & commonTypes cases into emitCommonTypes fn

Test Plan

yarn lint && yarn run flow && yarn jest react-native ⇒ 🟢

analysis-bot · 2023-03-12T12:37:59Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,517,218	+0
android	hermes	armeabi-v7a	7,833,068	+0
android	hermes	x86	8,997,378	+0
android	hermes	x86_64	8,852,469	+0
android	jsc	arm64-v8a	9,141,088	+0
android	jsc	armeabi-v7a	8,332,958	+0
android	jsc	x86	9,195,883	+0
android	jsc	x86_64	9,453,706	+0

Base commit: 93c1fbd
Branch: main

Pranav-yadav · 2023-03-12T12:41:47Z

Edit: forgot to mention u @cipolleschi

I tried to look ~~into~~ into below errors, but I'm not familiar with NativeModule Spec. Though looks like it's related to commonTypes
Any guess, what's causing this?

https://app.circleci.com/pipelines/github/facebook/react-native/20277/workflows/41829c63-256e-498d-aefc-64d1dc752dc1/jobs/474925?invite=true#step-107-17

cipolleschi

Thanks for this job. I left a few comment to simplify even further the codebase.

I tried to have a look at the error, but I couldn't figure out what's going wrong.
The problem is not in the NativeModules specifically, it looks like that it can't find the right type when going over the switch... i.e. the commonType variable result null and it throws an error.

I think that the best way to figure out what's going on here is to debug. To do so:

Pick a failing test. For example: RN Codegen TypeScript Parser › can generate fixture NATIVE_MODULE_WITH_UNSAFE_OBJECT
Go to modules/__test_fixtures__/fixtures.js and comment all the objects but the one we choose (for example NATIVE_MODULE_WITH_UNSAFE_OBJECT)
Make sure that module.exports is exporting that object.
Go to modules/__test_fixtures__/failures.js and comment all of them, as we want to work on a single example
Run the tests only in the specific folder, so yarn jest react-native-codegen -t src/parsers/typescript/modules

This should run only one test, the one we isolated, so you can try to debug the code, eventually adding console.log statements to see what's going on.

packages/react-native-codegen/src/parsers/typescript/modules/index.js

packages/react-native-codegen/src/parsers/parsers-primitives.js

packages/react-native-codegen/src/parsers/flow/modules/index.js

packages/react-native-codegen/src/parsers/typescript/modules/index.js

cipolleschi · 2023-03-13T15:05:49Z

packages/react-native-codegen/src/parsers/flow/modules/index.js

            hasteModuleName,
            types,
+            typeAnnotation,


Why don't we pass directly typeAnnotation.id.name?

Actually, the emitPartial requires typeAnnotation as param :)

Pranav-yadav · 2023-03-13T21:05:48Z

@cipolleschi thanks for sharing excellent debugging methods. Though the errors were related to extracting annotatedElementProperties which I've refactored into a getAnnotatedElementProperties fn in parser.

PS: below ci test fail seems unrelated to this diff??
https://app.circleci.com/pipelines/github/facebook/react-native/20316/workflows/5bf9c125-9f25-42ad-98e6-bcb2a85d910e/jobs/476849?invite=true#step-133-5029

Pranav-yadav · 2023-03-16T12:46:32Z

ping @cipolleschi :)

facebook-github-bot · 2023-03-16T12:51:14Z

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

…onTypes` fn - merged TS & Flow parsers' logic for below cases: - `Stringish` - `Int32` - `Double` - `Float` - `UnsafeObject` - `Object` - `Partial` - into an `emitCommonTypes` fn into `parsers-primitives.js` -- 🐨

avoids ternary :) Co-authored-by: Riccardo Cipolleschi <[email protected]>

facebook-github-bot · 2023-03-17T11:35:08Z

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

facebook-github-bot · 2023-03-20T14:38:49Z

@cipolleschi merged this pull request in 2f25261.

Pranav-yadav · 2023-03-20T14:41:31Z

🎉

@gabrieldonadel & @kyawthura-gg you're unblocked now :)

… & `commonTypes` cases into `emitCommonTypes` fn (facebook#36450) Summary: > [Codegen 78 - Assigned to Pranav-yadav] It depends on [Codegen 75][Codegen 76][Codegen 77] Extract the logic that emits Partial values in an emitPartial function, which takes the Parsers as parameter. >[Codegen 79 - Assigned to Pranav-yadav] It depends on [Codegen 78] Extract the basic cases logics (case Stringish, case Int32, case Double, ..., case Partial. `Flow` lines and `TypeScript` lines into a function emitCommonTypes in `parsers-primitives.js`. Make sure that the default case returns `null`. Call this function in the default: case `Flow`, `TypeScript` of the `index.js` file: if the function return something, return that from the default case; otherwise if the `emitCommonTypes` returns `null`, keep the current default implementation (throw the error). ### Changes - merged TS & Flow parsers' logic for `Partial` case to `emitPatial` fn - merged TS & Flow parsers' logic for below cases: - `Stringish` - `Int32` - `Double` - `Float` - `UnsafeObject` - `Object` - `Partial` - into an `emitCommonTypes` fn into `parsers-primitives.js` - add **_tests_** for `emitPartial` and `emitCommonTypes` fn's - add `getAnnotatedElementProperties` fn to parser & impl to both TS & Flow parsers ## Changelog: [INTERNAL] [CHANGED] - Merge TS & Flow parsers' logic for `Partial` case to `emitPatial` fn & `commonTypes` cases into `emitCommonTypes` fn Pull Request resolved: facebook#36450 Test Plan: - `yarn lint && yarn run flow && yarn jest react-native` ⇒ � Reviewed By: rshest Differential Revision: D44132308 Pulled By: cipolleschi fbshipit-source-id: f965e85ecc5d94e57ad85334ce565a55c512fde4

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 Mar 12, 2023

github-actions bot force-pushed the codegen-emitPartial-emitCommonTypes branch from f9ce471 to 7ce1db6 Compare March 13, 2023 14:18

cipolleschi requested changes Mar 13, 2023

View reviewed changes

Pranav-yadav force-pushed the codegen-emitPartial-emitCommonTypes branch from 6768bd3 to bafc18a Compare March 13, 2023 20:09

Pranav-yadav requested a review from cipolleschi March 13, 2023 20:53

Pranav-yadav force-pushed the codegen-emitPartial-emitCommonTypes branch from bafc18a to b904994 Compare March 15, 2023 14:57

Pranav-yadav mentioned this pull request Mar 15, 2023

[Codegen 95]: Merge defaultExports.forEach(statement => ... (Flow, TS) to findNativeComponentType fn in parser-commons.js #36466

Closed

Pranav-yadav force-pushed the codegen-emitPartial-emitCommonTypes branch from b904994 to 7464edc Compare March 16, 2023 12:35

cipolleschi approved these changes Mar 16, 2023

View reviewed changes

Pranav-yadav and others added 6 commits March 16, 2023 23:45

merge TS & Flow parsers' logic for Partial case to emitPatial fn

d575eae

add tests for emitPartial and emitCommonTypes fn's

94f718b

merge TS & Flow parsers' logic for Partial case to emitPatial fn

4bb9130

use parser.nameForGenericTypeAnnotation()

1332277

avoids ternary :) Co-authored-by: Riccardo Cipolleschi <[email protected]>

add getAnnotatedElementProperties fn to parser

a133e23

Pranav-yadav force-pushed the codegen-emitPartial-emitCommonTypes branch from 7464edc to a133e23 Compare March 16, 2023 18:46

resolve merge conflicts ;)

9ae92d5

facebook-github-bot closed this in 2f25261 Mar 20, 2023

facebook-github-bot added the Merged This PR has been merged. label Mar 20, 2023

Pranav-yadav deleted the codegen-emitPartial-emitCommonTypes branch March 20, 2023 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Codegen 78 & 79]: Merge TS & Flow parsers' logic for `Partial` case to `emitPartial` fn & `commonTypes` cases into `emitCommonTypes` fn #36450

[Codegen 78 & 79]: Merge TS & Flow parsers' logic for `Partial` case to `emitPartial` fn & `commonTypes` cases into `emitCommonTypes` fn #36450

Pranav-yadav commented Mar 12, 2023 •

edited

Loading

analysis-bot commented Mar 12, 2023 •

edited

Loading

Pranav-yadav commented Mar 12, 2023 •

edited

Loading

cipolleschi left a comment

cipolleschi Mar 13, 2023

Pranav-yadav Mar 13, 2023 •

edited

Loading

Pranav-yadav commented Mar 13, 2023

Pranav-yadav commented Mar 16, 2023

facebook-github-bot commented Mar 16, 2023

facebook-github-bot commented Mar 17, 2023

facebook-github-bot commented Mar 20, 2023

Pranav-yadav commented Mar 20, 2023

[Codegen 78 & 79]: Merge TS & Flow parsers' logic for Partial case to emitPartial fn & commonTypes cases into emitCommonTypes fn #36450

[Codegen 78 & 79]: Merge TS & Flow parsers' logic for Partial case to emitPartial fn & commonTypes cases into emitCommonTypes fn #36450

Conversation

Pranav-yadav commented Mar 12, 2023 • edited Loading

Summary

Changes

Changelog

Test Plan

analysis-bot commented Mar 12, 2023 • edited Loading

Pranav-yadav commented Mar 12, 2023 • edited Loading

cipolleschi left a comment

Choose a reason for hiding this comment

cipolleschi Mar 13, 2023

Choose a reason for hiding this comment

Pranav-yadav Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

Pranav-yadav commented Mar 13, 2023

Pranav-yadav commented Mar 16, 2023

facebook-github-bot commented Mar 16, 2023

facebook-github-bot commented Mar 17, 2023

facebook-github-bot commented Mar 20, 2023

Pranav-yadav commented Mar 20, 2023

[Codegen 78 & 79]: Merge TS & Flow parsers' logic for `Partial` case to `emitPartial` fn & `commonTypes` cases into `emitCommonTypes` fn #36450

[Codegen 78 & 79]: Merge TS & Flow parsers' logic for `Partial` case to `emitPartial` fn & `commonTypes` cases into `emitCommonTypes` fn #36450

Pranav-yadav commented Mar 12, 2023 •

edited

Loading

analysis-bot commented Mar 12, 2023 •

edited

Loading

Pranav-yadav commented Mar 12, 2023 •

edited

Loading

Pranav-yadav Mar 13, 2023 •

edited

Loading