react-native

react-native copied to clipboard

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 🟢

@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 :)

Done suggested changes!

@cipolleschi Though, not sure about these errors;

yarn run flow o/p 🔽

...

...

---

yarn jest react-native-codegen o/p 🔽

...

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.

1. 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.
2. 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)
3. 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?

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

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

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

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

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: 19d65a2baff2b2ab10d423e33baf373712ceda93 Branch: main

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

Base commit: 3d9a15da870a75ed76d60d0c9b0d38c351a15003 Branch: main

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

PR build artifact for 9885bcddc804becf8bcb6cdc00f9447c1666f61d 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

Tests Results

yarn jest react-native-codegen

yarn test

yarn test-ci

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

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

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

@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

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

PR build artifact for bb9a431b42f53e9534eac8bd6568271587fbca43 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 pinging you; in case you missed previous comment :)

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

Rebasing again. Resolving conflicts.

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

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

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 :)

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? :pray:

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 :)

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

PR build artifact for b63d1c1f01e38d87fb3e6791bda2edf24f31498c 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 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

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

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