Use asset catalog for ios images #1290
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thymikee
reviewed
Oct 7, 2020
thymikee
reviewed
Oct 7, 2020
| await Promise.all( | ||
| imageSet.files.map(async (file) => { | ||
| const dest = path.join(imageSet.basePath, file.name); | ||
| await fs.copyFile(file.src, dest); |
There was a problem hiding this comment.
we have a copyFiles utility, maybe worth reuse it?
There was a problem hiding this comment.
copyFiles copies all files in a directory so I don't think it could work here where we just want to copy specific files.
|
Interesting! Looking forward to adding some tests :) |
|
@thymikee Thanks for having a look! Going to work on the RN part of the PR and come back to make the improvements you suggested. |
janicduplessis
force-pushed
the
assets-catalog
branch
from
41f8708
to
b6262e7
Compare
|
Thank you @janicduplessis for sending this over! Been following this discussion on Discord as well, this looks really exciting. Waiting for the updates on the RN side. |
janicduplessis
force-pushed
the
assets-catalog
branch
from
b6262e7
to
d76a53e
Compare
|
cc @adamTrz |
adamTrz
approved these changes
Oct 14, 2022
There was a problem hiding this comment.
Lgtm!
@janicduplessis - if I read conversation from facebook/react-native#30129 correctly, this PR should be merged first?
Correct 👍 |
|
Hi everyone! Any update on this PR? :D |
thymikee
pushed a commit
that referenced
this pull request
Oct 17, 2022
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Oct 19, 2022
Summary: A super small PR to bump CLI to latest available of the 9.x stream, to make available in main the asset image fix (react-native-community/cli#1290) and to be able to cherry pick it back in 0.70 branch. ## 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] - bump CLI to 9.2.1 Pull Request resolved: #35022 Test Plan: CI is green Reviewed By: dmytrorykun Differential Revision: D40507344 Pulled By: huntie fbshipit-source-id: 7c3753e9df154eb5835f021cdfe1b476499afb9a
dmytrorykun
pushed a commit
to facebook/react-native
that referenced
this pull request
Oct 19, 2022
Summary: A super small PR to bump CLI to latest available of the 9.x stream, to make available in main the asset image fix (react-native-community/cli#1290) and to be able to cherry pick it back in 0.70 branch. <!-- 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] - bump CLI to 9.2.1 Pull Request resolved: #35022 Test Plan: CI is green Reviewed By: dmytrorykun Differential Revision: D40507344 Pulled By: huntie fbshipit-source-id: 7c3753e9df154eb5835f021cdfe1b476499afb9a
brentvatne
pushed a commit
to expo/react-native
that referenced
this pull request
Oct 28, 2022
Summary: A super small PR to bump CLI to latest available of the 9.x stream, to make available in main the asset image fix (react-native-community/cli#1290) and to be able to cherry pick it back in 0.70 branch. <!-- 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] - bump CLI to 9.2.1 Pull Request resolved: facebook#35022 Test Plan: CI is green Reviewed By: dmytrorykun Differential Revision: D40507344 Pulled By: huntie fbshipit-source-id: 7c3753e9df154eb5835f021cdfe1b476499afb9a
3 tasks
OlimpiaZurek
pushed a commit
to OlimpiaZurek/react-native
that referenced
this pull request
May 22, 2023
Summary: A super small PR to bump CLI to latest available of the 9.x stream, to make available in main the asset image fix (react-native-community/cli#1290) and to be able to cherry pick it back in 0.70 branch. ## 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] - bump CLI to 9.2.1 Pull Request resolved: facebook#35022 Test Plan: CI is green Reviewed By: dmytrorykun Differential Revision: D40507344 Pulled By: huntie fbshipit-source-id: 7c3753e9df154eb5835f021cdfe1b476499afb9a
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Generates an asset catalog for images on iOS. This allows Xcode to compile it and enables app thinning for RN assets. App thinning will re-bundle the app for each phone so it only includes asset with the right scale.
Presentation about features of asset catalog: https://developer.apple.com/videos/play/wwdc2018/227/
RN PR: facebook/react-native#30129
TODO: Add tests
The approach used is to add assets to a new asset catalog (RNAssets.xcassets). Assets inside the catalog should be gitignored, but the catalog is not. Assets are then added to the catalog at build time, by copying the assets files over and generating a Contents.json file containing info about the asset files. It generates the following directory structure:
Since the asset catalog is included in the project, it will be compiled to the app Assets.car file which also includes app icons and other image assets in the project.
Asset names need to be unique. To do so we can reuse the method that generates identifiers for android. It uses the full path and replaces
/with_.Asset catalog only works for image files (png, jpeg) so other type of assets will be copied to the bundle as they were before.
Test Plan:
App size
App thinning using iPhone 11
78 png assets, most
@2x+@3x, some@1x. Optimized using imageOptim.Before
bundle: 244kb (741kb on disk)
with app thinning: same
After
bundle: 572kb (same on disk)
with app thinning: 223kb (same on disk)
Savings: -8.6% (-70% size on disk)
Perf
I made a very non scientific perf measurement of the time it takes to load images by using the following code in RCTLocalAssetImageLoader:
Before:
After:
One thing that is interesting is that all assets seems to be only loaded once from the file system. Before we could see multiple calls over 1ms, while with asset catalog only the first load is over 1ms.