Use asset catalog for iOS images#30129
Conversation
Generated by 🚫 dangerJS against 1c7e31ba2ab80466cf837700b1c3d91f382a0412 |
Base commit: 950ea91 |
Base commit: c27e9e6 |
a39cb37 to
072c071
Compare
|
@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 |
|
@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 |
|
@janicduplessis can we rebase + fix the conflicts here? |
050a242 to
f527b5e
Compare
|
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 react-native-community/cli#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. |
|
/rebase |
f527b5e to
58b1917
Compare
|
@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 react-native-community/cli#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
left a comment
There was a problem hiding this comment.
There is a packages/react-native-codegen/react-native-codegen-0.0.5.tgz that needs to go 👍
There was a problem hiding this comment.
This would require a npm publish of this package OR the use of Verdaccio (which we already have for template tests).
There was a problem hiding this comment.
This might also conflict with #34572. Maybe not all template tests use verdaccio currently? Do I need to make some changes for it to pickup the assets package?
Maybe the simplest way would be to just revert the change to the assets package and can land it later as it is not really needed as all it does is rename a function.
There was a problem hiding this comment.
Maybe the simplest way would be to just revert the change to the assets package and can land it later as it is not really needed as all it does is rename a function.
If possible, let's revert it. We're in the process of migrating our repo to be a proper monorepo and let's try to limit the changes to those packages to only the necessary ones 👍
There was a problem hiding this comment.
Ok I reverted the change, let's see how CI goes.
|
@cortinico @cipolleschi Template tests are all passing except the ios release ones, with error |
6a1a87a to
0ccbecb
Compare
|
/rebase |
1 similar comment
|
/rebase |
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: There is currently an error when building in release on iOS when using asset catalogs (experimental feature that is partially merged #30129) This was probably incorrectly migrated from the community cli repo. `.imageset` is actually folders so it needs to be removed with `{recursive: true, force: true}`. I also renamed the variable `files` which is confusing since its folders. ## Changelog: [IOS] [FIXED] - Fix cleanAssetCatalog error Pull Request resolved: #41865 Test Plan: Tested in an app that uses asset catalogs Reviewed By: NickGerleman Differential Revision: D52032258 Pulled By: huntie fbshipit-source-id: 1dc0ca09e0da0d514b03d7d72707bdcaef03301d
|
This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
|
I will one day find time to finish this :) |
dce8343 to
819db88
Compare
320120c to
e60c9cf
Compare
Load packager image assets from a compiled asset catalog instead of loose files in the app bundle. At build time react-native-xcode.sh has the CLI emit imagesets into a staging catalog (--asset-catalog-dest, already in @react-native/community-cli-plugin), compiles it with actool into an RNAssets.bundle inside the app, and the native image loader resolves images by name from that bundle with [UIImage imageNamed:inBundle:]. The feature is opt-in via the RCTUseAssetCatalog Info.plist key, which is the single source of truth: the native loader reads it (a build-time constant, read once) and react-native-xcode.sh reads the same key to decide whether to bundle images into the catalog, so build and runtime cannot disagree on where image assets live. The script owns the catalog end to end, so migrating an app is adding the one Info.plist key: no .xcassets to create and no Xcode project changes. The app's own asset pipeline (Images.xcassets, asset symbol generation, CompileAssetCatalog) is untouched, and apps that have not migrated are unaffected. The catalog path is a single lookup with no filesystem fallback; a mis-bundled asset logs an RCTLogError instead of failing silently. The native side only resolves catalog names for what the CLI actually emits into the catalog (png/jpg/jpeg under main-bundle assets/, mirroring isCatalogAsset), so gif/webp packager assets, sub-bundles and OTA assets outside the main bundle fall through to the existing loader. jpeg is also added to RCTIsImageAssetsPath so jpeg assets are routed to the bundle-asset loaders. rn-tester and private/helloworld opt in via their Info.plists. helloworld's bundle phase now also substitutes REACT_NATIVE_PATH in CONFIG_JSON, which any bundling build requires. Cold image loads in RNTester are ~15x faster (median 47us vs 719us).
e60c9cf to
adf495d
Compare
Summary
Use an asset catalog for images on iOS. At build time,
react-native-xcode.shhas the bundler emit packager image assets into a staging asset catalog (--asset-catalog-dest, already in@react-native/community-cli-plugin), compiles it withactoolinto anRNAssets.bundleinside the app, and the native image loader resolves images by name from that bundle'sAssets.carwith[UIImage imageNamed:inBundle:]instead of reading loose files.Properties worth calling out:
RCTUseAssetCatalogkey in the app's Info.plist, and that key is the single source of truth: the native loader reads it (a build-time constant, read once), andreact-native-xcode.shreads the same key to decide whether to bundle images into the catalog — so the build and the runtime cannot disagree on where image assets live. Because the script owns the catalog end to end (staged in derived files, compiled into the app's resources next tomain.jsbundle), migration is adding one Info.plist key — no.xcassetsto create, no build-phase or project changes.RCTLogError(instead of silently rendering nothing) rather than adding anfsstat to the hot path.assets/…(mirroringisCatalogAsset). Everything else — gif/webp packager assets,.bundlesub-bundles, OTA assets outside the main bundle — falls through to the existing loader. (This also addsjpegtoRCTIsImageAssetsPath, which previously listed onlypng/jpg— an oversight, since.jpegis the same kind of bundled image; it is now treated likepng/jpgregardless of the catalog.)Images.xcassets, asset symbol generation (UIImage(resource:)), andCompileAssetCatalogare untouched: the RN catalog never enters the Xcode project, so there are no build-phase ordering constraints and nothing to disable.Changelog:
[iOS] [Added] - Use asset catalog for ios images
Performance
What was measured: the time to resolve an asset to a
UIImageinsideRCTImageFromLocalAssetURL— a compiled-catalog name lookup vs. the legacyimageNamed:search +imageWithContentsOfFile:on loose files. Decode is deferred in both paths (and costs the same), so this is resolve latency, not end-to-end render time. It matters becauseRCTLocalAssetImageLoaderloads local assets synchronously to avoid flicker, so this time sits on the critical path once per distinct image. Setup: RNTester, Release, new arch, iOS simulator, first load of each distinct image (UIKit caches repeats — repeat loads are ~10 µs in both builds).Raw per-load numbers (final implementation,
RNAssets.bundle), in load order:searchicon¹bottomnavcomponentsicondarkbottomnavplaygroundsiconlightbottomnavapisiconlightbottomnavcomponentsiconlightuie_thumb_normaluie_thumb_selecteduie_comment_normaluie_comment_highlightedverylargeimage²alphahotdog¹ First load in each build pays one-time costs: mapping
Assets.caron the catalog side, ImageIO/framework warm-up + first disk touch on the filesystem side.² Resolve only — the large image's decode cost is unchanged, so its end-to-end win is much smaller than this row suggests.
Every image is faster from the catalog: median 67 µs vs 667 µs (10×; an earlier run of the same benchmark measured 47 µs vs 719 µs, ~15× — run-to-run variance, same order either way). The mechanism: the catalog is a single memory-mapped, indexed archive (one name lookup, no per-image syscalls), while the loose-file path does an
imageNamed:search over several filename permutations plus a per-image file open.The URL→catalog-name derivation on the native side is a single character pass with no regex; benchmarked against a straightforward regex-based implementation of the same transform it measures 4.9 µs vs 11.0 µs per call (2.2×, Debug build, including the shared URL→bundle-path resolution both share), so the name mapping is a negligible part of the lookup.
App thinning: verified that App Store slicing thins the
Assets.carinsideRNAssets.bundleexactly like the app's own catalog. Test: a fixture app with a 1x/2x/3x imageset in both its mainImages.xcassetsand an actool-compiledRNAssets.bundle, archived and exported withthinningfor a 3x device — both cars were sliced to 3x-only. So unused scales are stripped per device (unlike today's loose packager files, which ship every scale to every device).Migration
To opt an app in, add to its
Info.plist:That's the entire migration — no
.xcassetsto create, no build-phase or project changes. Notes:boolValuecheck accepts), so build-setting substitution or fully generated Info.plists (GENERATE_INFOPLIST_FILE) aren't seen by the script and stay on the legacy path. If bundling happens outsidereact-native-xcode.sh(custom CI callingreact-native bundledirectly) while the key is enabled, the native side detects the missingRNAssets.bundleand logs an actionable error instead of silently rendering nothing.@2x(no@3x) is kept for a 3x device by App Store slicing and used at runtime — verified with a thinned export. Only a non-integer-only asset (e.g. an Android-density@1.5xwith no1x/2x/3xvariant, which iOS asset catalogs cannot represent) is unsupported:actooldrops it with anUnknown scale valuewarning in the build log and the runtime logs an error. That's a rare antipattern; acommunity-cli-pluginfollow-up will turn it into a clear build-time error at the source (it has the scale and asset path), rather than this repo re-parsing actool output.react-native-website) and the one-line opt-in inreact-native-community/templatewill be separate PRs, timed with the release that ships this.Test Plan
RNTester and
private/helloworldboth opt in (RCTUseAssetCatalog=YESin their Info.plists — the only change an app needs), so CI covers the catalog path on both app shapes. The new-project template lives inreact-native-community/templateand will get the same one-line opt-in in a follow-up PR there. To test locally with RNTester:RNAssets.bundle/Assets.car; the app contains no loose packager png/jpg files.Unit coverage in
RCTURLUtilsTests(testAssetCatalogNameForURL), asserting the identifiers the CLI generates: scale-suffix stripping (including non-integer@1.5x), folder encoding, illegal-char removal, jpeg vs. non-catalog types (gif), out-of-project-root assets, query strings, non-packager paths, and nil.Mac Catalyst: verified end-to-end by building rn-tester as a Mac Catalyst app — the catalog compiles (via the Catalyst-specific
--platform macosx --ui-framework-family uikitactool invocation) intoRNTester.app/Contents/Resources/RNAssets.bundlewith all packager images and no loose files, and the runtime uses the sameimageNamed:inBundle:mechanism CocoaPods resource bundles use on Catalyst. (rn-tester doesn't ship a Catalyst config, so this isn't in CI, but the build is clean.)