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

Rename assets to assets-registry #34572

Closed
wants to merge 2 commits into from

Conversation

fortmarek
Copy link
Contributor

Summary

Part of the monorepo RFC

Changelog

[General] [Changed] - Rename assets to assets-registry

Test Plan

@fortmarek fortmarek requested a review from hramos as a code owner September 2, 2022 09:09
@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Shopify Partner: Shopify Partner labels Sep 2, 2022
@github-actions
Copy link

github-actions bot commented Sep 2, 2022

Warnings
⚠️ 🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

Generated by 🚫 dangerJS against 23d1e08

@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Sep 2, 2022
@facebook-github-bot
Copy link
Contributor

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

@analysis-bot
Copy link

analysis-bot commented Sep 2, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,103,533 +0
android hermes armeabi-v7a 6,471,857 +0
android hermes x86 7,521,168 +0
android hermes x86_64 7,380,054 +0
android jsc arm64-v8a 8,968,359 +0
android jsc armeabi-v7a 7,699,467 +0
android jsc x86 9,030,658 +0
android jsc x86_64 9,508,690 +0

Base commit: 8b8f4f3
Branch: main

@fortmarek fortmarek force-pushed the rename/packager-assets branch from e14bb07 to 55063f3 Compare September 2, 2022 09:38
@facebook-github-bot
Copy link
Contributor

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

@analysis-bot
Copy link

analysis-bot commented Sep 2, 2022

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

Base commit: 8b8f4f3
Branch: main

@fortmarek fortmarek force-pushed the rename/packager-assets branch 2 times, most recently from 0d20b68 to d351910 Compare September 12, 2022 09:21
@kelset kelset added the Tech: Monorepo For PRs that are related to the monorepo infra label Sep 12, 2022
@cortinico cortinico mentioned this pull request Sep 14, 2022
11 tasks
@@ -2,7 +2,7 @@ load("@fbsource//tools/build_defs/third_party:yarn_defs.bzl", "yarn_workspace")
load("@fbsource//xplat/js:JS_DEFS.bzl", "rn_library")

rn_library(
name = "assets",
Copy link
Contributor

Choose a reason for hiding this comment

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

Commenting here but valid for the whole PR:

Let's undo the BUCK update and the path change. It will make easier for us to import and merge this change 👍

I've also posted an update on the monorepo effort there:

facebook-github-bot pushed a commit that referenced this pull request Sep 22, 2022
Summary:
This PR adds [verdaccio](https://github.com/verdaccio/verdaccio) to release packages in the `packages` directory during the E2E test on CI.

The rationale behind this is the following:
- Firstly, we wanted to push the [monorepo RFC](react-native-community/discussions-and-proposals#480). We hit an issue when renaming the packages to follow the same convention caused by the e2e test using the template to fail. This is because the template installs packages from the live version of npm – and if you just rename a package in a given PR without releasing it, the package understandably can't be installed since it's not published, yet – as you can see [here](https://app.circleci.com/pipelines/github/facebook/react-native/15286/workflows/149df51f-f59b-4eb3-b92c-20c513111f04/jobs/282135?invite=true#step-108-283).
- Secondly, the current e2e test on `main` does not actually test the latest code of the packages in the `packages` directory as it simply downloads the latest versions from npm. This creates a divide between what's tested and what users should expect when using nightlies or when a new minor is released.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[Internal] [Changed] - Use verdaccio for template e2e test

Pull Request resolved: #34577

Test Plan: `test_js` CI check should pass. Additionally, I have temporarily updated the [PR](#34572) renaming `assets` to `assets-registry` to include the verdaccio changes – the `test_js` passes there, additionally proving merging this PR will unblock us with the rename PRs.

Reviewed By: cipolleschi

Differential Revision: D39723048

Pulled By: cortinico

fbshipit-source-id: aeff3811967360740df3b3dbf1df50e506fb72d8
@fortmarek fortmarek force-pushed the rename/packager-assets branch from d351910 to dcbe802 Compare September 28, 2022 12:32
@fortmarek fortmarek requested a review from cortinico September 28, 2022 12:33
@fortmarek
Copy link
Contributor Author

I've updated the PR, so it includes only the version bump and package renaming 🤞

@cortinico
Copy link
Contributor

@fortmarek can you rebase and fix the merge conflicts?

@pull-bot
Copy link

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

@cortinico
Copy link
Contributor

Thanks for the rebase @fortmarek
@hoxyq is looking into merging those PRs by adapting our internal infra to let them work fine 👍

@hoxyq hoxyq force-pushed the rename/packager-assets branch from e4533d6 to eceb227 Compare November 25, 2022 09:39
@pull-bot
Copy link

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

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

@hoxyq hoxyq force-pushed the rename/packager-assets branch from eceb227 to 23d1e08 Compare November 25, 2022 21:47
@pull-bot
Copy link

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

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @fortmarek in 3c5a829.

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
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
This PR adds [verdaccio](https://github.com/verdaccio/verdaccio) to release packages in the `packages` directory during the E2E test on CI.

The rationale behind this is the following:
- Firstly, we wanted to push the [monorepo RFC](react-native-community/discussions-and-proposals#480). We hit an issue when renaming the packages to follow the same convention caused by the e2e test using the template to fail. This is because the template installs packages from the live version of npm – and if you just rename a package in a given PR without releasing it, the package understandably can't be installed since it's not published, yet – as you can see [here](https://app.circleci.com/pipelines/github/facebook/react-native/15286/workflows/149df51f-f59b-4eb3-b92c-20c513111f04/jobs/282135?invite=true#step-108-283).
- Secondly, the current e2e test on `main` does not actually test the latest code of the packages in the `packages` directory as it simply downloads the latest versions from npm. This creates a divide between what's tested and what users should expect when using nightlies or when a new minor is released.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[Internal] [Changed] - Use verdaccio for template e2e test

Pull Request resolved: facebook#34577

Test Plan: `test_js` CI check should pass. Additionally, I have temporarily updated the [PR](facebook#34572) renaming `assets` to `assets-registry` to include the verdaccio changes – the `test_js` passes there, additionally proving merging this PR will unblock us with the rename PRs.

Reviewed By: cipolleschi

Differential Revision: D39723048

Pulled By: cortinico

fbshipit-source-id: aeff3811967360740df3b3dbf1df50e506fb72d8
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
Part of the [monorepo RFC](https://github.com/react-native-community/discussions-and-proposals/blob/04671deebe4ef2af782e281bbd912a8597c5af96/proposals/0006-react-native-monorepo.md)

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[General] [Changed] - Rename assets to assets-registry

Pull Request resolved: facebook#34572

Reviewed By: cortinico

Differential Revision: D39235817

Pulled By: hoxyq

fbshipit-source-id: ff4d4a7ff980d3fc6e28b83ad3b36250eba79fc3
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. p: Shopify Partner: Shopify Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Tech: Monorepo For PRs that are related to the monorepo infra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants