Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

Pranav-yadav
Copy link
Contributor

Summary

[Codegen 95] Extract the defaultExports.forEach(statement => (Flow, TS) function in parser-commons, so that it accept a Parser parameter to unify the behaviors between flow and typescript. The Parser object needs to be enriched with all the methods to extract the required information from the Node, if they are not there yet.

Changes

  • merged TS & Flow parsers' logic for defaultExports.forEach(statement => of /components/index.js:findComponentConfig() into;
  • findNativeComponentType fn to parsers-commons.js
  • add getTypeArgumentParamsFromDeclaration & getNativeComponentType fn's
  • add tests for getTypeArgumentParamsFromDeclaration & getNativeComponentType fn's

Changelog

[INTERNAL] [CHANGED] - Merge defaultExports.forEach(statement => ... (Flow, TS) to findNativeComponentType fn in parser-commons.js

Test Plan

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

@facebook-github-bot 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 13, 2023
@analysis-bot
Copy link

analysis-bot commented Mar 13, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,517,160 +0
android hermes armeabi-v7a 7,833,020 +0
android hermes x86 8,997,326 +0
android hermes x86_64 8,852,413 +0
android jsc arm64-v8a 9,141,081 +0
android jsc armeabi-v7a 8,332,959 +0
android jsc x86 9,195,883 +0
android jsc x86_64 9,453,713 +0

Base commit: 681d7f8
Branch: main

Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work here! I left a comment for something that you may have missed.
A part from that, it looks good to me! ;)

@cipolleschi
Copy link
Contributor

(also, it needs a rebase and conflict resolution) ;)

@facebook-github-bot
Copy link
Contributor

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

@Pranav-yadav Pranav-yadav force-pushed the codegen95defaultExports branch 2 times, most recently from 1c40c47 to 29c85f1 Compare March 15, 2023 14:37
@Pranav-yadav
Copy link
Contributor Author

Pranav-yadav commented Mar 15, 2023

@cipolleschi rebased & resolved conflicts but, dk why ci keeps failing these days 🤔 (especially on my PRs 🫠 jk)
Quick hrefs to flailing ci tests👇

Edit: the same thing is happening on my another PR
You may need to look at the CI configs
cc: @cortinico

@cortinico
Copy link
Contributor

/rebase

@cortinico
Copy link
Contributor

Edit: the same thing is happening on my another PR
You may need to look at the CI configs
cc: @cortinico

Not sure why this is the case. Trying to rebase to see if this solves the issue. One of the failure was a transient network issue

… fn's

Add below fn's to `parser` (Flow & TS):

- `getTypeArgumentParamsFromDeclaration` : Given a declaration, it returns the typeArgumentParams of the declaration.
- `getNativeComponentType` : Given a typeArgumentParams and funcArgumentParams it returns a native component type.
Add below fn which unifies the `defaultExports.forEach(statement => ..`
behaviors between Flow & TS:

- `findNativeComponentType` : This function is used to find the type of
   a native component provided the default exports statement from generated AST.
…tiveComponentType` fn in `parser-commons.js`
@github-actions github-actions bot force-pushed the codegen95defaultExports branch from 29c85f1 to d5089b9 Compare March 16, 2023 12:14
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Mar 16, 2023
@facebook-github-bot
Copy link
Contributor

@cipolleschi merged this pull request in c0a46c6.

@Pranav-yadav Pranav-yadav deleted the codegen95defaultExports branch March 16, 2023 18:08
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
…tiveComponentType` fn in `parser-commons.js` (facebook#36466)

Summary:
>[Codegen 95] Extract the `defaultExports.forEach(statement =>` (Flow, TS) function in `parser-commons`, so that it accept a Parser parameter to unify the behaviors between flow and typescript. The Parser object needs to be enriched with all the methods to extract the required information from the Node, if they are not there yet.

### Changes

- merged TS & Flow parsers' logic for `defaultExports.forEach(statement =>` of `/components/index.js:findComponentConfig()` into;
- `findNativeComponentType` fn to `parsers-commons.js`
- add `getTypeArgumentParamsFromDeclaration` & `getNativeComponentType` fn's
- add **_tests_** for getTypeArgumentParamsFromDeclaration & `getNativeComponentType` fn's

## Changelog

[INTERNAL] [CHANGED] - Merge `defaultExports.forEach(statement => ...` (Flow, TS) to `findNativeComponentType` fn in `parser-commons.js`

Pull Request resolved: facebook#36466

Test Plan: - `yarn lint && yarn run flow && yarn jest react-native` ⇒ �

Reviewed By: rshest

Differential Revision: D44088862

Pulled By: cipolleschi

fbshipit-source-id: 91bf0edcd53b2e054160af34d7c128355c178b26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants