Skip to content

Conversation

@Pranav-yadav
Copy link
Contributor

@Pranav-yadav Pranav-yadav commented Apr 25, 2023

Summary:

This diff adds missing README files for all public RN packages.

Changes:

For all public RN packages:

  • Add Missing READMEs

Update package.json in all RN packages to add:

  • Issues, Bugs urls
  • Keywords and Homepage urls to respective pkgs

Changelog:

[GENERAL][ADDED] - Add missing README files for all public RN packages.
[GENERAL][CHANGED] - Update package.json in all RN packages to add required fields.

Test Plan:

  • yarn lint && yarn flow && yarn test-ci --> should be green

@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 Apr 25, 2023
@analysis-bot
Copy link

analysis-bot commented Apr 25, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,499,710 +0
android hermes armeabi-v7a 7,833,554 +0
android hermes x86 8,977,657 +0
android hermes x86_64 8,834,477 +0
android jsc arm64-v8a 9,064,270 +0
android jsc armeabi-v7a 8,275,566 +0
android jsc x86 9,113,565 +0
android jsc x86_64 9,374,032 +0

Base commit: b0cf746
Branch: main

@Pranav-yadav
Copy link
Contributor Author

Pranav-yadav commented Apr 26, 2023

This PR is stacked on "Update Node.js to v16".

  • TODO: Rebase once that PR is merged. Rebased.

Note: I'll be unavailable due to uni. exams, if everything is okay, then please rebase (try slash rebase command) and merge or suggest and commit changes (edits allowed to maintainers) 👍

@cortinico cortinico requested a review from hoxyq April 27, 2023 10:59
{
"name": "@react-native/codegen-typescript-test",
"version": "0.0.1",
"description": "⚛️ TypeScript related unit test for @react-native/codegen",
Copy link
Contributor Author

@Pranav-yadav Pranav-yadav Apr 27, 2023

Choose a reason for hiding this comment

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

Using symbols like ⚛️ in description is just extra noise for npm and other registries, that too at the start of the descriptions string.
Removing such symbols should make the search easier and should make the packages more accessible.

Same applies for all package.json files.

"license": "MIT",
"repository": {
"type": "git",
"url": "git@github.com:facebook/react-native.git",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

git@ (old) & git:// (new) are more prone to attacks than https://.

Due to the lack of TLS or other cryptography, cloning over git:// might lead to an arbitrary code execution vulnerability, and should therefore be avoided unless you know what you are doing.

If you run git clone git://example.com/project.git, an attacker who controls e.g your router can modify the repo you just cloned, inserting malicious code into it. If you then compile/run the code you just cloned, you will execute the malicious code. Running git clone http://example.com/project.git should be avoided for the same reason.

Running git clone https://example.com/project.git does not suffer from the same problem (unless the attacker can provide a TLS certificate for example.com). Running git clone [[email protected]]:project.git only suffers from this problem if you accept a wrong ssh key fingerprint.

Reference: https://git-scm.com/book/en/v2/Git-on-the-Server-The-Protocols
It is very informative read 😃

@Pranav-yadav
Copy link
Contributor Author

Update: Had some free time, so rebased & resolved the conflicts 😅.

@hoxyq I've left self review above explaining why particular changes are done.
Also about adding more info to respective READMEs, I guess (contributors/maintainers) can iteratively do that as per convenience and shouldn't be a blocker for merging.

cc: @cortinico

@Pranav-yadav Pranav-yadav marked this pull request as ready for review April 27, 2023 17:04
"directory": "packages/react-native"
},
"homepage": "https://reactnative.dev/",
"keywords": ["react", "react-native", "android", "ios", "mobile", "cross-platform", "app-framework", "mobile-development"],
Copy link
Contributor

Choose a reason for hiding this comment

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

@cortinico

Are we okay with these keywords?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PS. I took them from keyword on RN gh repo page :)

"directory": "packages/assets"
},
"license": "MIT",
"homepage": "https://github.com/facebook/react-native/tree/HEAD/packages/assets#readme",
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of #readme suffix in all these links? Do we want to navigate to README file?

Copy link
Contributor Author

@Pranav-yadav Pranav-yadav Apr 27, 2023

Choose a reason for hiding this comment

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

Yup, that's the purpose of "homepage" field in case of packages under monorepo, unless we've specific page for each package on RN/any other website.
PS. If you look closely exact similar links are already used in RN packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, #... is just a "fragment" and won't break urls in any case.

@Pranav-yadav
Copy link
Contributor Author

Pranav-yadav commented Apr 27, 2023

Rebased & addressed suggestions re: jest testing syntax 👍

@Pranav-yadav Pranav-yadav requested a review from hoxyq April 27, 2023 20:37
For all public RN packages:
- Add missing READMEs
- Add issues, bugs urls
- Add keywords and homepage urls to respective pkgs
@facebook-github-bot
Copy link
Contributor

@hoxyq 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 Apr 28, 2023
@facebook-github-bot
Copy link
Contributor

@hoxyq merged this pull request in 14316bd.

@Pranav-yadav Pranav-yadav deleted the update-packageJsons branch April 28, 2023 12:13
jeongshin pushed a commit to jeongshin/react-native that referenced this pull request May 7, 2023
…k#37090)

Summary:
This diff adds _missing_ README files for all public RN packages.

#### Changes:
For all public RN packages:
- Add _Missing_ READMEs

Update package.json in all RN packages to add:
- Issues, Bugs urls
- Keywords and Homepage urls to respective pkgs

## Changelog:

[GENERAL][ADDED] - Add missing README files for all public RN packages.
[GENERAL][CHANGED] - Update package.json in all RN packages to add required fields.

Pull Request resolved: facebook#37090

Test Plan: - `yarn lint && yarn flow && yarn test-ci` --> _should be green_

Reviewed By: cortinico

Differential Revision: D45390861

Pulled By: hoxyq

fbshipit-source-id: 524a92de56a7cb553573d9f54ccf40a998dfd35f
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.

4 participants