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

refactor: mv translateArrayTypeAnnotation (Flow,TS) fns > parsers-primitives.js #35479

Conversation

Pranav-yadav
Copy link
Contributor

Summary

This PR is a subtask of #34872
Moved translateArrayTypeAnnotation (Flow,TS) fns to parsers-primitives.js

  • combined Flow and TS translateArrayTypeAnnotation fn 's into common fn
  • moved it to parsers-primitives.js
  • re-organized imports and exports :)

Changelog

[Internal] [Changed] - Moved translateArrayTypeAnnotation (Flow,TS) fns to parsers-primitives.js

Test Plan

  • ensure 👇 is #00ff00
    yarn lint && yarn flow && yarn test-ci

@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 Nov 25, 2022
@Pranav-yadav Pranav-yadav force-pushed the rf/translateArrayTypeAnnotation branch from 0ecafa6 to 89ee2b0 Compare November 25, 2022 15:23
@Pranav-yadav Pranav-yadav marked this pull request as draft November 25, 2022 15:37
@analysis-bot
Copy link

analysis-bot commented Nov 25, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,102,106 -21
android hermes armeabi-v7a 6,470,990 -31
android hermes x86 7,520,421 -37
android hermes x86_64 7,378,933 -26
android jsc arm64-v8a 8,967,115 -55
android jsc armeabi-v7a 7,698,601 -45
android jsc x86 9,029,922 -48
android jsc x86_64 9,507,549 -57

Base commit: 1b99473
Branch: main

@Pranav-yadav

This comment was marked as outdated.

@Pranav-yadav Pranav-yadav force-pushed the rf/translateArrayTypeAnnotation branch from 89ee2b0 to 5d11c89 Compare November 25, 2022 16:08
@Pranav-yadav Pranav-yadav marked this pull request as ready for review November 25, 2022 16:10
@Pranav-yadav

This comment was marked as outdated.

@Pranav-yadav Pranav-yadav force-pushed the rf/translateArrayTypeAnnotation branch 2 times, most recently from 90421d5 to 1301a1e Compare November 25, 2022 17:05
@pull-bot
Copy link

PR build artifact for 1301a1e 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
Copy link

PR build artifact for 1301a1e 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
Copy link
Contributor

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

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.

Hi @Pranav-yadav, thank you for taking care of this.

I left a comment to simplify the code further. Let me know what do you think.

….js`

- combined `Flow` and `TS` `translateArrayTypeAnnotation` fn 's into common fn
- moved it to `parsers-primitives.js`
- re-organized imports and exports :)
@Pranav-yadav Pranav-yadav force-pushed the rf/translateArrayTypeAnnotation branch from 1301a1e to 28177b8 Compare November 29, 2022 15:35
@Pranav-yadav
Copy link
Contributor Author

Pranav-yadav commented Nov 29, 2022

@cipolleschi Tests are passing. Though, a general Qn for you; yarn lint is not working for me on Win10 m/c; any guess? Logs 👇

Tests
yarn run v1.22.19
$ eslint .

Oops! Something went wrong! :(

ESLint: 8.23.1

E:\Pranav-SEM-7\react-native\react-native\packages\react-native-codegen\src\parsers\flow\modules\index.js:13
import type {
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at Object.compileFunction (node:vm:360:18)
    at wrapSafe (node:internal/modules/cjs/loader:1088:15)
    at Module._compile (node:internal/modules/cjs/loader:1123:27)
    at Module._compile (E:\Pranav-SEM-7\react-native\react-native\node_modules\pirates\lib\index.js:136:24)
    at Module._extensions..js (node:internal/modules/cjs/loader:1213:10)
    at Object.newLoader [as .js] (E:\Pranav-SEM-7\react-native\react-native\node_modules\pirates\lib\index.js:141:7)
    at Module.load (node:internal/modules/cjs/loader:1037:32)
    at Module._load (node:internal/modules/cjs/loader:878:12)
    at Module.require (node:internal/modules/cjs/loader:1061:19)
    at require (node:internal/modules/cjs/helpers:103:18)
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
yarn run v1.22.19
$ flow
No errors!
Done in 1.07s.
$ yarn jest react-native-codegen
yarn run v1.22.19
...
Test Suites: 52 passed, 52 total
Tests:       2383 passed, 2383 total
Snapshots:   775 passed, 775 total
Time:        106.192 s, estimated 118 s
Ran all test suites matching /react-native-codegen/i.
Done in 116.95s.

@analysis-bot
Copy link

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

Base commit: dccb57f
Branch: main

@pull-bot
Copy link

PR build artifact for 28177b8 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
Copy link

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

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.

Uhm... It looks like that yarn lint is working, but it does not accept the import type syntax The CI jobs are green, so I guess that it is ok.
How are you invoking it?

@facebook-github-bot
Copy link
Contributor

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

@Pranav-yadav
Copy link
Contributor Author

Pranav-yadav commented Nov 30, 2022

How are you invoking it?

From git-bash, yarn lint as well as tried eslint .
Also tried from cmd; gives same error 🤔

Click Me

image

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @Pranav-yadav in 5a20bd5.

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

@react-native-bot react-native-bot added the Merged This PR has been merged. label Nov 30, 2022
@Pranav-yadav Pranav-yadav deleted the rf/translateArrayTypeAnnotation branch November 30, 2022 13:10
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
…primitives.js` (facebook#35479)

Summary:
This PR is a subtask of facebook#34872
Moved `translateArrayTypeAnnotation` (Flow,TS) fns to `parsers-primitives.js`
- combined `Flow` and `TS` `translateArrayTypeAnnotation` fn 's into common fn
- moved it to `parsers-primitives.js`
- re-organized imports and exports :)

## Changelog

[Internal] [Changed] - Moved `translateArrayTypeAnnotation` (Flow,TS) fns to `parsers-primitives.js`

Pull Request resolved: facebook#35479

Test Plan:
- ensure 👇 is `#00ff00`
`yarn lint && yarn flow && yarn test-ci`

Reviewed By: cipolleschi

Differential Revision: D41548046

Pulled By: GijsWeterings

fbshipit-source-id: 8fd7214f8b1e669ba42f326f814674eec179fed5
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.

6 participants