[Codegen 94] move `extendsForProp` into `parsers-commons` #37052

siddarthkay · 2023-04-23T02:54:50Z

Summary:

[Codegen 94] This PR attempts to extracts the logic of extendsForProp function from the following locations :

parsers/flow/components/extends.js
parsers/typescript/components/props.js
since they are the same and move the function to parsers/parsers-commons.js as requested on ☂️ Improving Codegen #34872

Changelog:

[Internal] [Changed] - Move extendsForProp to parser-commons and update usages.

Test Plan:

Run yarn jest react-native-codegen and ensure CI is green

facebook-github-bot · 2023-04-23T02:54:56Z

Hi @siddarthkay!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

facebook-github-bot · 2023-04-23T03:09:34Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

analysis-bot · 2023-04-23T16:36:15Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,624,897	+0
android	hermes	armeabi-v7a	7,937,532	+0
android	hermes	x86	9,111,778	+0
android	hermes	x86_64	8,966,631	+0
android	jsc	arm64-v8a	9,188,944	+0
android	jsc	armeabi-v7a	8,379,041	+0
android	jsc	x86	9,247,173	+0
android	jsc	x86_64	9,505,674	+0

Base commit: c65ab4d
Branch: main

siddarthkay · 2023-04-24T04:58:12Z

@cipolleschi : This PR is ready for review.

facebook-github-bot · 2023-04-24T09:02:58Z

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

cipolleschi

Hi @siddarthkay, thank you so much for taking the time to work on this!

I left some comments to improve the current implementation, following a pattern we have established in other Codegen tasks.

Let me know if it is clear and/or if you need help! :D

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

packages/react-native-codegen/src/parsers/typescript/components/props.js

packages/react-native-codegen/src/parsers/flow/components/extends.js

siddarthkay · 2023-04-24T14:52:50Z

Hi @cipolleschi : I've addressed your feedback and I also took the liberty of removing type PropAST = Object; from 5 files and moving it to packages/react-native-codegen/src/parsers/utils.js hope this is okay?

Let me know :)

facebook-github-bot · 2023-04-24T16:01:09Z

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

cipolleschi

Good! :D Thanks for the changes!

siddarthkay · 2023-04-25T06:56:32Z

Hi @cipolleschi : I've rebased my branch and resolved all conflicts,
Would you mind taking a look at this PR again and let me what are the next steps here?
I'm eager and excited to get my 1st PR merged into react-native 👍🏻

rshest · 2023-04-25T09:43:21Z

Hey @siddarthkay, sorry for back and forth, but this change severely conflicts with #36891, which has been now merged to master (it merges together getExtendedProps and getProps).

Could you please rebase this PR to master and try and resolve the merge conflicts?

Thanks!!!

rshest

Please see my comment above, this requires a rebase to master and merge/resolve conflicts.

siddarthkay · 2023-04-25T09:52:14Z

Hi @rshest : I think I already resolved those conflicts few hours ago, I have just now rebased to latest main branch
and tests seem to pass.
Could you please check again?

facebook-github-bot · 2023-04-25T12:20:00Z

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

siddarthkay · 2023-04-25T13:31:56Z

I see test_android_template-NewArch-Debug-Hermes failed on CI with :

error An unexpected error occurred: "http://localhost:4873/yallist/-/yallist-3.1.1.tgz: 
Request failed \"500 Internal Server Error\""

error Command failed with exit code 1: yarn add /root/react-native/packages/react-native
Error: Command failed with exit code 1: yarn add /root/react-native/packages/react-native
    at makeError (/root/react-native/node_modules/execa/lib/error.js:60:11)
    at handlePromise (/root/react-native/node_modules/execa/index.js:118:26)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async createFromTemplate (/root/react-native/node_modules/@react-native-community/cli/build/
    commands/init/init.js:96:5)
    at async Object.initialize [as func] (/root/react-native/node_modules/@react-native-community/
    cli/build/commands/init/init.js:179:3)
    at async Command.handleAction (/root/react-native/node_modules/@react-native-community/
    cli/build/index.js:109:9)
node:child_process:935
    throw err;
    ^

Error: Command failed: node cli.js init AndroidTemplateProject --directory 
/tmp/AndroidTemplateProject --template /root/react-native/packages/react-native 
--verbose --skip-install
    at checkExecSyncError (node:child_process:861:11)
    at execSync (node:child_process:932:15)
    at install (/root/react-native/scripts/template/initialize.js:72:3)
    at Object.<anonymous> (/root/react-native/scripts/template/initialize.js:95:1)
    at Module._compile (node:internal/modules/cjs/loader:1196:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1250:10)
    at Module.load (node:internal/modules/cjs/loader:1074:32)
    at Function.Module._load (node:internal/modules/cjs/loader:909:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12) {
  status: 1,
  signal: null,
  output: [ null, null, null ],
  pid: 521,
  stdout: null,
  stderr: null
}

Should I rerun this job on CircleCI : https://app.circleci.com/pipelines/github/facebook/react-native/22620/workflows/be7cfad0-d96d-4c5c-b552-5006294413ff/jobs/580151 ?
OR
Amend and force push last commit to run all jobs?

Pranav-yadav · 2023-04-25T19:22:50Z

@siddarthkay as the error says:
'Request failed "500 Internal Server Error"'
It's may be a transient error, due to network or something else, so no need to worry about it. 👍

PS. Reviewer(s) will re-rerun it if required.

siddarthkay · 2023-04-26T06:19:35Z

Hi @Pranav-yadav Thanks for explaining!

rshest · 2023-04-26T09:35:28Z

packages/react-native-codegen/src/parsers/flow/components/props.js

 ): ExtendsForProp {
-  if (!prop.argument) {
+  const argument = parser.argumentForProp(prop);
+  if (argument) {


This now does exactly the opposite of what it used/supposed to do :)

It should instead be:

if (!argument) {

Otherwise this would break codegen completely, at least on our internal tests.

I am surprised, though, that CI on Github didn't catch this (the only failed test seems inside the app template and is unrelated).

I think we need a follow-up on that, CC @cipolleschi

@siddarthkay - note that I fixed this internally myself this time, to save one extra back and forth, but normally that would be your responsibility :)

Thanks @rshest ! This is probably the outcome of resolving multiple rebase conflicts. Thanks for catching and fixing it ❤️

I am surprised, though, that CI on Github didn't catch this (the only failed test seems inside the app template and is unrelated).

I think, those files don't have tests, but, other tests should have been failed; or may be the current test-suite doesn't test this specific case 🤔

facebook-github-bot · 2023-04-26T11:35:24Z

@rshest merged this pull request in c937162.

Summary: [Codegen 94] This PR attempts to extracts the logic of `extendsForProp` function from the following locations : - `parsers/flow/components/extends.js` - `parsers/typescript/components/props.js` since they are the same and move the function to `parsers/parsers-commons.js` as requested on facebook#34872 ## Changelog: [Internal] [Changed] - Move `extendsForProp` to parser-commons and update usages. Pull Request resolved: facebook#37052 Test Plan: Run `yarn jest react-native-codegen` and ensure CI is green Reviewed By: cipolleschi Differential Revision: D45225880 Pulled By: rshest fbshipit-source-id: 45199089746d58d9e9494b28040b34c2a0eb31fe

siddarthkay marked this pull request as draft April 23, 2023 02:58

siddarthkay mentioned this pull request Apr 23, 2023

☂️ Improving Codegen #34872

Closed

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 Apr 23, 2023

siddarthkay force-pushed the codegen-94 branch 6 times, most recently from a29ef09 to e41085e Compare April 23, 2023 16:13

siddarthkay force-pushed the codegen-94 branch from e41085e to 95e6b4c Compare April 24, 2023 02:54

siddarthkay marked this pull request as ready for review April 24, 2023 04:55

cipolleschi requested changes Apr 24, 2023

View reviewed changes

siddarthkay force-pushed the codegen-94 branch 5 times, most recently from 9d48729 to 0b4455a Compare April 24, 2023 14:34

siddarthkay requested a review from cipolleschi April 24, 2023 14:51

cipolleschi approved these changes Apr 24, 2023

View reviewed changes

Pranav-yadav added the Tech: Codegen Related to react-native-codegen label Apr 24, 2023

siddarthkay force-pushed the codegen-94 branch 3 times, most recently from 1af6089 to 301ac6d Compare April 25, 2023 01:40

siddarthkay force-pushed the codegen-94 branch 2 times, most recently from ca8f170 to 40f63b1 Compare April 25, 2023 05:29

rshest requested changes Apr 25, 2023

View reviewed changes

siddarthkay force-pushed the codegen-94 branch from 40f63b1 to bcac77a Compare April 25, 2023 09:48

siddarthkay requested a review from rshest April 25, 2023 10:12

siddarthkay force-pushed the codegen-94 branch from bcac77a to 1af2946 Compare April 25, 2023 11:38

move extendsForProp into parsers-commons.js

228f929

siddarthkay force-pushed the codegen-94 branch from 1af2946 to 228f929 Compare April 25, 2023 11:45

rshest reviewed Apr 26, 2023

View reviewed changes

facebook-github-bot closed this in c937162 Apr 26, 2023

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

[Codegen 94] move extendsForProp into parsers-commons #37052

[Codegen 94] move extendsForProp into parsers-commons #37052

Uh oh!

Conversation

siddarthkay commented Apr 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary:

Changelog:

Test Plan:

Uh oh!

facebook-github-bot commented Apr 23, 2023

Action Required

Process

Uh oh!

facebook-github-bot commented Apr 23, 2023

Uh oh!

analysis-bot commented Apr 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

siddarthkay commented Apr 24, 2023

Uh oh!

facebook-github-bot commented Apr 24, 2023

Uh oh!

cipolleschi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

siddarthkay commented Apr 24, 2023

Uh oh!

facebook-github-bot commented Apr 24, 2023

Uh oh!

cipolleschi left a comment

Choose a reason for hiding this comment

Uh oh!

siddarthkay commented Apr 25, 2023

Uh oh!

rshest commented Apr 25, 2023

Uh oh!

rshest left a comment

Choose a reason for hiding this comment

Uh oh!

siddarthkay commented Apr 25, 2023

Uh oh!

facebook-github-bot commented Apr 25, 2023

Uh oh!

siddarthkay commented Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pranav-yadav commented Apr 25, 2023

Uh oh!

siddarthkay commented Apr 26, 2023

Uh oh!

rshest Apr 26, 2023

Choose a reason for hiding this comment

Uh oh!

siddarthkay Apr 26, 2023

Choose a reason for hiding this comment

Uh oh!

Pranav-yadav Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Apr 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[Codegen 94] move `extendsForProp` into `parsers-commons` #37052

[Codegen 94] move `extendsForProp` into `parsers-commons` #37052

siddarthkay commented Apr 23, 2023 •

edited

Loading

analysis-bot commented Apr 23, 2023 •

edited

Loading

siddarthkay commented Apr 25, 2023 •

edited

Loading

Pranav-yadav Apr 26, 2023 •

edited

Loading