react-native
react-native copied to clipboard
facebook
Use asset catalog for ios images
Summary
Use an asset catalog for images on iOS. See related PR in the CLI for the code that generates it https://github.com/react-native-community/cli/pull/1290.
Changelog:
[iOS] [Added] - Use asset catalog for ios images
Test Plan
See https://github.com/react-native-community/cli/pull/1290
Test on RN Tester
- Add to root package.json and yarn install
"resolutions": {
"@react-native-community/cli-plugin-metro": "janicduplessis/react-native-cli#react-native-community-cli-plugin-metro-v9.1.3-test.0-gitpkg"
},
-
PRODUCTION=1 yarn setup-ios-jscin packages/rn-tester -
In Xcode, product -> scheme -> edit scheme -> change Build Configuration to Release.
-
Build and check that local images are working.
| Warnings | |
|---|---|
| :warning: | :lock: package.json - Changes were made to package.json. This will require a manual import by a Facebook employee. |
Generated by :no_entry_sign: dangerJS against 1c7e31ba2ab80466cf837700b1c3d91f382a0412
| Platform | Engine | Arch | Size (bytes) | Diff |
|---|---|---|---|---|
| ios | - | universal | n/a | -- |
Base commit: 950ea915bee6b3b30107c04bd34990fb390c7268 Branch: main
| Platform | Engine | Arch | Size (bytes) | Diff |
|---|---|---|---|---|
| android | hermes | arm64-v8a | 8,895,039 | +1 |
| android | hermes | armeabi-v7a | 7,943,587 | +2 |
| android | hermes | x86 | 9,292,924 | +1 |
| android | hermes | x86_64 | 9,194,442 | -2 |
| android | jsc | arm64-v8a | 9,482,023 | +0 |
| android | jsc | armeabi-v7a | 8,423,567 | +0 |
| android | jsc | x86 | 9,466,043 | -1 |
| android | jsc | x86_64 | 9,780,275 | +2 |
Base commit: c27e9e647781774b0469fa4f2ca6969784084230 Branch: main
@janicduplessis I haven't dived deep into assets stuff on the CLI side and here, but maybe you have a better overview on this. Now that assets logic is scoped under @react-native/assets package, maybe we could reuse it on the CLI side? We should strive to reuse everything that's possible to avoid these weird syncing situations (this is not the first time).
@thymikee This already uses the shared code in the assets package and updates it (just rename a method since it is now used on iOS too).
@sota000 Any interest on landing this? I've been using this in my fork for a long time and is stable.
It would be great to have a status on this one, there's an outstanding PR in the CLI too.
One think I'd guess would be fixing the conflicts, but yeah also would be great to get some internal 👍/👎 if it's even worth
Ok I rebased this and tested again in RN tester. I added instructions on how to do so in the Test Plan. It does require the CLI patched with https://github.com/react-native-community/cli/pull/1290 to work.
I think the main part that is tricky with this is if the app template is not properly updated or for some reason an older version of the cli is used it will break. For the old cli version case it is actually not that bad since the build will error because of invalid parameter --asset-catalog-dest. If the template project is not properly configured then assets won't show in release mode only.
I think it is still fine to move forward, but might want to make sure to document the change to make sure templates are updated properly. The main 2 things that need updating is to create the RNAssets.xcassets catalog and to reorder build scripts so that JS bundle is built before Copy Bundle Resources. For new projects and automated updates that should be fine, but might not be obvious for someone updating manually.
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
Weird, this PR makes the template tests fail... Basically, the CLI is not able to create a RN app... Is it expected because it requires https://github.com/react-native-community/cli/pull/1290 ?
Oh I see the problem, the template yarn install fails because of error Couldn't find any versions for "@react-native/assets" that matches "1.1.0". Not sure what would be best to handle updating the @react-native/assets package. It is basically just renaming a function since we use it on iOS too now.
We might also have build issue later for ios release build since it does require the cli patch
@cortinico @cipolleschi Template tests are all passing except the ios release ones, with error error: unknown option '--asset-catalog-dest', which is expected since it doesn’t include the updated cli. Maybe the next step would be to get the cli PR merged and published.
@cortinico @cipolleschi Template tests are all passing except the ios release ones, with error
error: unknown option '--asset-catalog-dest', which is expected since it doesn’t include the updated cli. Maybe the next step would be to get the cli PR merged and published.
Sounds like a plan 👍
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@cortinico Cli is updated and template tests are passing now, I don’t think other test failures are related, but will double check.
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
Hi there, the analyze-code job fails due to some linting problems in the JS side. Could you have a look at them and fix them, please?
PR build artifact for 3dc67c896cc23451cf0c558b0e82d575c7e47326 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.
PR build artifact for 3dc67c896cc23451cf0c558b0e82d575c7e47326 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.
@javache Thanks for reviewing this. I've reimplemented the logic to convert from the file url to the asset catalog name in objc and made it so it falls back to using the current bundle url logic if the image doesn't exist in the catalog. It will also log a warning.
I've tested that the Image example works with and without an asset catalog (by not passing the asset catalog arg to the CLI). This also makes sure old way to include assets work too and doesn't log a warning.
PR build artifact for 64f025a331cdb1e0fe9a7a9c065ba7677e48d336 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.
PR build artifact for 64f025a331cdb1e0fe9a7a9c065ba7677e48d336 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.
PR build artifact for 6a1a87adf01a6715271b38b1a126cf8260e91d30 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.
PR build artifact for 6a1a87adf01a6715271b38b1a126cf8260e91d30 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.
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.