[Codegen] Extract `tryParse` (Flow, TypeScript) lambda into `parseObjectProperty` fn #35076

Pranav-yadav · 2022-10-25T10:54:22Z

This PR is a task of #34872

Summary

Extract the content of the tryParse (Flow, TypeScript)lambda into a proper parseObjectProperty function into the parsers-commons.js file.
also,

added new helper fn isObjectProperty in parsers-commons.js file.
added tests for isObjectProperty and parseObjectProperty fn 's

Changelog

[Internal] [Changed] - Extracted the content of the tryParse (Flow, TypeScript) lambda into a proper parseObjectProperty fn into the parsers-commons.js file.

Test Plan

run

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

and ensure everything is 🟢

Pranav-yadav · 2022-10-25T11:03:22Z

@cipolleschi as mentioned in the issue, I've Extracted the content of the tryParse (Flow, TypeScript) lambda into a proper parseObjectProperty fn into the parsers-commons.js file.

Have prepared some tests for parseObjectProperty fn, but (commented) them because of logical errors in fn
Also tried to fix most of the errors, but was unable to get around some :(
A little guidance to fix the same would be helpful :)

PS: This is first time I'm interacting w/ the react-natives codebase :)

cipolleschi

Thank you so much for engaging with React Native! :D

Let's start with the suggested change.

Then, the other thing you should do is to install Flow and try to run it. It should tell you which errors are there and how to solve them.

Once you did that, the first thing I'll make sure that the command

jest test react-native-codegen

works. You should run it from the react-native root, after running yarn install in the root.

If that is green, you can start adding some unit tests.
Let me know if you can make any progress with this hints.

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

Pranav-yadav · 2022-10-25T19:07:13Z

Done suggested changes!

@cipolleschi
Though, not sure about these errors;

`yarn run flow` o/p 🔽

...

`yarn jest react-native-codegen` o/p 🔽

...

cipolleschi · 2022-10-26T14:26:21Z

With Flow broken, it is normal that the tests are broken as well. Don't worry, we can fix everything! :D

Let's start from Flow.

The @flow strict errors are similar to what another contributor did here
You should try to change the parsers-commons from strict to strict-local. Or you should make error-utils.js and error.js strict as well.
The cannot get property.type because the property.type is missing error is because the type of property is wrong. In this case we are passing the AST node. All the NamedShape stuffs are types we have to return not types that are passed as parameters. The solution here is to change the type of the property from NamedShape<Nullable<...>> to $FlowFixMe (the ASTNode is not properly type, unfortunately)
You need to add the return type for the function. That should be NamedShape<Nullable<NativeModuleBaseTypeAnnotation>>. So the function signature is parseObjectProperty(...): NamedShape<Nullable<NativeModuleBaseTypeAnnotation>>

These should make Flow green.
With Flow green, the react-native-codegen tests should pass as well.

can you give it a shot?

Pranav-yadav · 2022-10-26T14:36:04Z

On it. Thanks for brief instructions. Btw, had overviewed the PR you mentioned; earlier today, and was able to get around those :)

Pranav-yadav · 2022-10-26T16:27:53Z

@cipolleschi Flow is now #00ff00.
But, react-native-codegen is failing #ff0000

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

facebook-github-bot · 2022-10-28T11:42:14Z

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

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

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

cipolleschi · 2022-10-28T15:03:13Z

With these two returns, the tests should be green as well! :D

analysis-bot · 2022-10-28T17:28:44Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,066,177	+0
android	hermes	armeabi-v7a	6,438,561	+0
android	hermes	x86	7,481,495	+0
android	hermes	x86_64	7,340,889	+0
android	jsc	arm64-v8a	8,931,995	+0
android	jsc	armeabi-v7a	7,666,339	+0
android	jsc	x86	8,992,366	+0
android	jsc	x86_64	9,471,121	+0

Base commit: 19d65a2
Branch: main

analysis-bot · 2022-10-28T17:59:46Z

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

Base commit: 3d9a15d
Branch: main

pull-bot · 2022-10-28T18:11:28Z

PR build artifact for 9885bcd 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-10-28T18:11:35Z

PR build artifact for 9885bcd 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-10-31T06:20:46Z

@cipolleschi

Tests Results

yarn jest react-native-codegen

yarn test

yarn test-ci

pull-bot · 2022-10-31T08:04:40Z

PR build artifact for 958072d 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-10-31T08:05:47Z

PR build artifact for 958072d 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 · 2022-10-31T16:28:09Z

Looks great now. It would be awesome if you can uncomment the tests and make them work! :D

Pranav-yadav · 2022-11-01T14:56:11Z

@cipolleschi added tests for isObjectProperty and parseObjectProperty fn 's :)
PS: rebased on main as well.
signing off.. have exams tomorrow; need to study hahaha, will see this afterwards if any further changes are required

Tests Results

yarn test-ci

yarn jest react-native-codegen

pull-bot · 2022-11-01T15:53:09Z

PR build artifact for bb9a431 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-01T15:57:19Z

PR build artifact for bb9a431 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-03T07:23:27Z

@cipolleschi pinging you; in case you missed previous comment :)

cipolleschi · 2022-11-03T18:56:32Z

Sorry, these days have been a bit full of stuff. I hope to find some time to look into this tomorrow.

Pranav-yadav · 2022-11-04T16:11:29Z

Rebasing again. Resolving conflicts.

pull-bot · 2022-11-05T06:31:28Z

PR build artifact for 4197bf4 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-05T06:32:29Z

PR build artifact for 4197bf4 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-07T12:58:43Z

Sorry, these days have been a bit full of stuff. I hope to find some time to look into this tomorrow.

Hey @cipolleschi !
Hope everything went well w/ the release (except that android versioning issue).

Pinging you just in case you don't forget this hahaha, though there is no hurry, review only when you get time :)

cipolleschi · 2022-11-07T17:45:06Z

Hi @Pranav-yadav! Thank you for the ping, and luckily, I hope the Android issue is mitigated now.
Can I ask you to rebase the PR once again? 🙏

- extract `tryParse` lambda of `translateTypeAnnotation` fn - to `parsers-commons`; for both Flow and TS

- add some tests (currenty commented)

- make _changed_ `@flow strict` - add `$FlowFixMe` incoming args where needed - fix other logical errors

- removed unused import; `UnsupportedUnionTypeAnnotationParserError`

- actions-bot provided wrong import warning :)

- `isObjectProperty` fn and, - `parseObjectProperty` fn

Pranav-yadav · 2022-11-07T18:12:17Z

Hi @Pranav-yadav! Thank you for the ping, and luckily, I hope the Android issue is mitigated now. Can I ask you to rebase the PR once again? 🙏

done!
Edit: @cipolleschi :)

pull-bot · 2022-11-07T19:12:52Z

PR build artifact for b63d1c1 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-07T19:17:09Z

PR build artifact for b63d1c1 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-08T15:40:58Z

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

react-native-bot · 2022-11-09T10:20:05Z

This pull request was successfully merged by @Pranav-yadav in 5744b21.

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

…y` fn (facebook#35076) Summary: This PR is a task of facebook#34872 > Extract the content of the tryParse (Flow, TypeScript)lambda into a proper `parseObjectProperty` function into the parsers-commons.js file. also, - added new helper fn `isObjectProperty` in `parsers-commons.js` file. - added tests for `isObjectProperty` and `parseObjectProperty` fn 's ## Changelog [Internal] [Changed] - Extracted the content of the `tryParse` ([Flow](https://github.com/facebook/react-native/blob/main/packages/react-native-codegen/src/parsers/flow/modules/index.js#L292-L337), [TypeScript](https://github.com/facebook/react-native/blob/main/packages/react-native-codegen/src/parsers/typescript/modules/index.js#L306-L351)) lambda into a proper `parseObjectProperty` fn into the `parsers-commons.js` file. Pull Request resolved: facebook#35076 Test Plan: - run ```bash yarn lint && yarn flow && yarn test-ci ``` - and ensure everything is � <img width="720" alt="image" src="https://user-images.githubusercontent.com/55224033/200105151-360b9b5e-52c7-4586-89b0-6860e9725f6e.png"> Reviewed By: cortinico Differential Revision: D40797241 Pulled By: cipolleschi fbshipit-source-id: 48b8900ead70d5eda2496f9ce044c11a9599a177

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 Oct 25, 2022

cipolleschi requested changes Oct 25, 2022

View reviewed changes

packages/react-native-codegen/src/parsers/parsers-commons.js Outdated Show resolved Hide resolved

cipolleschi added the hacktoberfest-accepted label Oct 25, 2022

Pranav-yadav requested a review from cipolleschi October 25, 2022 20:29

Pranav-yadav commented Oct 26, 2022

View reviewed changes

packages/react-native-codegen/src/parsers/typescript/modules/index.js Outdated Show resolved Hide resolved

Pranav-yadav marked this pull request as ready for review October 26, 2022 19:01

cipolleschi requested changes Oct 28, 2022

View reviewed changes

packages/react-native-codegen/src/parsers/flow/modules/index.js Outdated Show resolved Hide resolved

packages/react-native-codegen/src/parsers/typescript/modules/index.js Outdated Show resolved Hide resolved

Pranav-yadav requested a review from cipolleschi October 28, 2022 17:30

Pranav-yadav force-pushed the refactor/tryParse-lambda branch from 958072d to bb9a431 Compare November 1, 2022 14:50

Pranav-yadav force-pushed the refactor/tryParse-lambda branch 2 times, most recently from a3e5bc4 to 4197bf4 Compare November 5, 2022 06:00

Pranav-yadav added 10 commits November 7, 2022 17:52

(codegen) extract tryParse (Flow, TS) lambda to parsers-commons

a0bf1bb

- extract `tryParse` lambda of `translateTypeAnnotation` fn - to `parsers-commons`; for both Flow and TS

(codegen) fix imports and add parseObjectProperty fn

7b93735

TEMP-(codegen) add parseObjectProperty tests

9fd739c

(codegen) prepare parseObjectProperty tests

d922dfd

- add some tests (currenty commented)

objectProperty chk -> isObjectProperty fn

28d51b3

fix @flow strict & other errors

a21a6b7

- make _changed_ `@flow strict` - add `$FlowFixMe` incoming args where needed - fix other logical errors

rm unused import

b32ea70

- removed unused import; `UnsupportedUnionTypeAnnotationParserError`

fix unused import

af27ae8

- actions-bot provided wrong import warning :)

reorganize code + rm unused import

7920002

chore: added tests - react-native-codegen/ for;

b63d1c1

- `isObjectProperty` fn and, - `parseObjectProperty` fn

Pranav-yadav force-pushed the refactor/tryParse-lambda branch from 4197bf4 to b63d1c1 Compare November 7, 2022 18:11

cipolleschi approved these changes Nov 8, 2022

View reviewed changes

facebook-github-bot closed this in 5744b21 Nov 9, 2022

react-native-bot added the Merged This PR has been merged. label Nov 9, 2022

Pranav-yadav deleted the refactor/tryParse-lambda branch November 11, 2022 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Codegen] Extract `tryParse` (Flow, TypeScript) lambda into `parseObjectProperty` fn #35076

[Codegen] Extract `tryParse` (Flow, TypeScript) lambda into `parseObjectProperty` fn #35076

Pranav-yadav commented Oct 25, 2022 •

edited

Loading

Pranav-yadav commented Oct 25, 2022 •

edited

Loading

cipolleschi left a comment •

edited

Loading

Pranav-yadav commented Oct 25, 2022 •

edited

Loading

`yarn run flow` o/p 🔽

`yarn jest react-native-codegen` o/p 🔽

cipolleschi commented Oct 26, 2022

Pranav-yadav commented Oct 26, 2022

Pranav-yadav commented Oct 26, 2022

facebook-github-bot commented Oct 28, 2022

cipolleschi commented Oct 28, 2022

analysis-bot commented Oct 28, 2022 •

edited

Loading

analysis-bot commented Oct 28, 2022 •

edited

Loading

pull-bot commented Oct 28, 2022

pull-bot commented Oct 28, 2022

Pranav-yadav commented Oct 31, 2022 •

edited

Loading

pull-bot commented Oct 31, 2022

pull-bot commented Oct 31, 2022

cipolleschi commented Oct 31, 2022 •

edited

Loading

Pranav-yadav commented Nov 1, 2022 •

edited

Loading

pull-bot commented Nov 1, 2022

pull-bot commented Nov 1, 2022

Pranav-yadav commented Nov 3, 2022

cipolleschi commented Nov 3, 2022

Pranav-yadav commented Nov 4, 2022

pull-bot commented Nov 5, 2022

pull-bot commented Nov 5, 2022

Pranav-yadav commented Nov 7, 2022

cipolleschi commented Nov 7, 2022

Pranav-yadav commented Nov 7, 2022 •

edited

Loading

pull-bot commented Nov 7, 2022

pull-bot commented Nov 7, 2022

facebook-github-bot commented Nov 8, 2022

react-native-bot commented Nov 9, 2022

[Codegen] Extract tryParse (Flow, TypeScript) lambda into parseObjectProperty fn #35076

[Codegen] Extract tryParse (Flow, TypeScript) lambda into parseObjectProperty fn #35076

Conversation

Pranav-yadav commented Oct 25, 2022 • edited Loading

Summary

Changelog

Test Plan

Pranav-yadav commented Oct 25, 2022 • edited Loading

cipolleschi left a comment • edited Loading

Choose a reason for hiding this comment

Pranav-yadav commented Oct 25, 2022 • edited Loading

yarn run flow o/p 🔽

yarn jest react-native-codegen o/p 🔽

cipolleschi commented Oct 26, 2022

Pranav-yadav commented Oct 26, 2022

Pranav-yadav commented Oct 26, 2022

facebook-github-bot commented Oct 28, 2022

cipolleschi commented Oct 28, 2022

analysis-bot commented Oct 28, 2022 • edited Loading

analysis-bot commented Oct 28, 2022 • edited Loading

pull-bot commented Oct 28, 2022

pull-bot commented Oct 28, 2022

Pranav-yadav commented Oct 31, 2022 • edited Loading

pull-bot commented Oct 31, 2022

pull-bot commented Oct 31, 2022

cipolleschi commented Oct 31, 2022 • edited Loading

Pranav-yadav commented Nov 1, 2022 • edited Loading

pull-bot commented Nov 1, 2022

pull-bot commented Nov 1, 2022

Pranav-yadav commented Nov 3, 2022

cipolleschi commented Nov 3, 2022

Pranav-yadav commented Nov 4, 2022

pull-bot commented Nov 5, 2022

pull-bot commented Nov 5, 2022

Pranav-yadav commented Nov 7, 2022

cipolleschi commented Nov 7, 2022

Pranav-yadav commented Nov 7, 2022 • edited Loading

pull-bot commented Nov 7, 2022

pull-bot commented Nov 7, 2022

facebook-github-bot commented Nov 8, 2022

react-native-bot commented Nov 9, 2022

[Codegen] Extract `tryParse` (Flow, TypeScript) lambda into `parseObjectProperty` fn #35076

[Codegen] Extract `tryParse` (Flow, TypeScript) lambda into `parseObjectProperty` fn #35076

Pranav-yadav commented Oct 25, 2022 •

edited

Loading

Pranav-yadav commented Oct 25, 2022 •

edited

Loading

cipolleschi left a comment •

edited

Loading

Pranav-yadav commented Oct 25, 2022 •

edited

Loading

`yarn run flow` o/p 🔽

`yarn jest react-native-codegen` o/p 🔽

analysis-bot commented Oct 28, 2022 •

edited

Loading

analysis-bot commented Oct 28, 2022 •

edited

Loading

Pranav-yadav commented Oct 31, 2022 •

edited

Loading

cipolleschi commented Oct 31, 2022 •

edited

Loading

Pranav-yadav commented Nov 1, 2022 •

edited

Loading

Pranav-yadav commented Nov 7, 2022 •

edited

Loading