Use asset catalog for ios images #30129

janicduplessis · 2020-10-07T21:17:13Z

Summary

Use an asset catalog for images on iOS. See related PR in the CLI for the code that generates it react-native-community/cli#1290.

Changelog:

[iOS] [Added] - Use asset catalog for ios images

Test Plan

See react-native-community/cli#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-jsc in packages/rn-tester
In Xcode, product -> scheme -> edit scheme -> change Build Configuration to Release.
Build and check that local images are working.

pull-bot · 2020-10-07T21:22:41Z

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

Generated by 🚫 dangerJS against 1c7e31b

analysis-bot · 2020-10-07T21:38:41Z

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

Base commit: 950ea91
Branch: main

analysis-bot · 2020-10-07T21:38:52Z

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: c27e9e6
Branch: main

thymikee · 2020-11-18T12:22:08Z

@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).

janicduplessis · 2022-01-04T22:10:05Z

@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).

janicduplessis · 2022-01-04T22:10:43Z

@sota000 Any interest on landing this? I've been using this in my fork for a long time and is stable.

grabbou · 2022-02-15T12:55:20Z

It would be great to have a status on this one, there's an outstanding PR in the CLI too.

kelset · 2022-02-15T14:42:17Z

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

cortinico · 2022-10-04T09:48:23Z

@janicduplessis can we rebase + fix the conflicts here?

janicduplessis · 2022-10-04T18:56:33Z

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.

cipolleschi · 2022-10-05T09:14:42Z

/rebase

facebook-github-bot · 2022-10-05T09:33:34Z

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

cipolleschi · 2022-10-05T15:03:46Z

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 ?

janicduplessis · 2022-10-05T15:30:27Z

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.

janicduplessis · 2022-10-05T15:31:40Z

We might also have build issue later for ios release build since it does require the cli patch

cortinico

There is a packages/react-native-codegen/react-native-codegen-0.0.5.tgz that needs to go 👍

cortinico · 2022-10-05T17:32:04Z

packages/assets/package.json

@@ -1,6 +1,6 @@
 {
  "name": "@react-native/assets",
-  "version": "1.0.0",
+  "version": "1.1.0",


This would require a npm publish of this package OR the use of Verdaccio (which we already have for template tests).

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.

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 👍

Ok I reverted the change, let's see how CI goes.

janicduplessis · 2022-10-22T01:09:50Z

@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.

pull-bot · 2022-10-22T01:37:32Z

PR build artifact for 64f025a 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 · 2022-10-22T01:39:16Z

PR build artifact for 64f025a 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 · 2022-10-22T02:25:58Z

PR build artifact for 6a1a87a 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 · 2022-10-22T02:26:12Z

PR build artifact for 6a1a87a 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 · 2022-11-10T13:58:12Z

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

javache · 2022-11-10T14:53:28Z

React/Base/RCTUtils.m

+  // Remove extension
+  path = [path stringByDeletingPathExtension];
+
+  // Remove scale suffix
+  path = [RCTAssetURLScaleRegex() stringByReplacingMatchesInString:path
+                                                           options:0
+                                                             range:NSMakeRange(0, [path length])
+                                                      withTemplate:@""];
+
+  path = [path lowercaseString];
+
+  // Encode folder structure in file name
+  path = [path stringByReplacingOccurrencesOfString:@"/" withString:@"_"];
+
+  // Remove illegal chars
+  path = [RCTAssetURLCharactersRegex() stringByReplacingMatchesInString:path
+                                                                options:0
+                                                                  range:NSMakeRange(0, [path length])
+                                                           withTemplate:@""];
+
+  // Remove "assets_" prefix
+  if ([path hasPrefix:@"assets_"]) {
+    path = [path substringFromIndex:@"assets_".length];
+  }


This is a really important code path that will be used by any image loaded in RN. Can we optimize it further?

E.g. rather than mutating the string multiple times, could you have a single regular expression that captures in a subgroup the relevant information so we don't have to write it multiple times, like this https://regex101.com/r/6Lqxoc/1

javache · 2022-11-10T14:56:12Z

React/Base/RCTUtils.m

+NSString *__nullable RCTAssetCatalogNameForURL(NSURL *__nullable URL)
+{
+  NSString *path = RCTBundlePathForURL(URL);
+  // Packager assets always start with assets/
+  if (path == nil || ![path hasPrefix:@"assets/"]) {
+    return nil;
+  }
+
+  // Remove extension
+  path = [path stringByDeletingPathExtension];
+
+  // Remove scale suffix
+  path = [RCTAssetURLScaleRegex() stringByReplacingMatchesInString:path
+                                                           options:0
+                                                             range:NSMakeRange(0, [path length])
+                                                      withTemplate:@""];
+
+  path = [path lowercaseString];
+
+  // Encode folder structure in file name
+  path = [path stringByReplacingOccurrencesOfString:@"/" withString:@"_"];
+
+  // Remove illegal chars
+  path = [RCTAssetURLCharactersRegex() stringByReplacingMatchesInString:path
+                                                                options:0
+                                                                  range:NSMakeRange(0, [path length])
+                                                           withTemplate:@""];
+
+  // Remove "assets_" prefix
+  if ([path hasPrefix:@"assets_"]) {
+    path = [path substringFromIndex:@"assets_".length];
+  }


Is there any documentation by Apple on how asset catalog assets are named that we could reference here as a comment?

This logic is based on how we name the assets when we create the catalog in the rn cli (https://github.com/react-native-community/cli/blob/main/packages/cli-plugin-metro/src/commands/bundle/assetPathUtils.ts#L67). The logic is the same as we use for android, which i know has a lot of limitations about how assets have to be named.

javache · 2022-11-10T14:57:38Z

React/Base/RCTUtils.m

+    regex = [NSRegularExpression regularExpressionWithPattern:@"[^a-z0-9_]" options:0 error:nil];
+  });
+  return regex;
+}


Instead of a regular expression, could you use a NSCharacterSet for this?

javache · 2022-11-10T14:58:57Z

React/Base/RCTUtils.m

+      return image;
+    } else {
+      RCTLogWarn(
+          @"Image %@ not found in the asset catalog. Make sure your app template is updated correctly.", catalogName);


Is there a link to documentation we could provide here?

I'm worried that this warns and then just continues executing, so the warning will likely be ignored.

I think we could provide a link to the release notes where it explains this change. However this doesn’t exists yet so I guess it could be added later?

cipolleschi · 2022-11-17T13:54:43Z

@janicduplessis, there are a couple of comments by @javache that should be addressed. Could you take care of it, please? 🙏

janicduplessis · 2022-11-17T16:30:23Z

Yes, haven’t had the time to address those yet. I should be able to in the next week or so.

cipolleschi · 2023-08-04T09:50:13Z

/rebase

cipolleschi · 2023-08-04T09:52:06Z

/rebase

facebook-github-bot · 2023-08-04T09:59:02Z

@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

Summary: There is currently an error when building in release on iOS when using asset catalogs (experimental feature that is partially merged facebook#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: facebook#41865 Test Plan: Tested in an app that uses asset catalogs Reviewed By: NickGerleman Differential Revision: D52032258 Pulled By: huntie fbshipit-source-id: 1dc0ca09e0da0d514b03d7d72707bdcaef03301d

github-actions · 2024-02-01T05:01:26Z

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.

janicduplessis · 2024-02-03T15:00:50Z

I will one day find time to finish this :)

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. Contributor A React Native contributor. labels Oct 7, 2020

react-native-bot added Platform: iOS iOS applications. Type: Enhancement A new feature or enhancement of an existing feature. labels Oct 7, 2020

janicduplessis mentioned this pull request Oct 7, 2020

Use asset catalog for ios images react-native-community/cli#1290

Merged

janicduplessis force-pushed the assets-catalog branch from a39cb37 to 072c071 Compare October 10, 2020 19:30

kelset added the Needs: React Native Team Attention label Feb 15, 2022

janicduplessis marked this pull request as ready for review October 3, 2022 23:47

janicduplessis requested a review from hramos as a code owner October 3, 2022 23:47

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 Oct 4, 2022

janicduplessis force-pushed the assets-catalog branch 2 times, most recently from 050a242 to f527b5e Compare October 4, 2022 18:49

github-actions bot force-pushed the assets-catalog branch from f527b5e to 58b1917 Compare October 5, 2022 09:16

cortinico requested changes Oct 5, 2022

View reviewed changes

janicduplessis force-pushed the assets-catalog branch from b7608e4 to 64f025a Compare October 22, 2022 01:03

github-actions bot force-pushed the assets-catalog branch from 64f025a to 3894a46 Compare October 22, 2022 01:53

janicduplessis force-pushed the assets-catalog branch from 3894a46 to 6a1a87a Compare October 22, 2022 01:54

javache reviewed Nov 10, 2022

View reviewed changes

janicduplessis added 2 commits April 22, 2023 14:48

Use asset catalog for ios images

a4e0f09

Implement catalog conversion natively and add fallback

0ccbecb

janicduplessis force-pushed the assets-catalog branch from 6a1a87a to 0ccbecb Compare April 22, 2023 06:49

Merge branch 'main' into assets-catalog

3820159

janicduplessis mentioned this pull request Dec 8, 2023

Fix cleanAssetCatalog error #41865

Closed

github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Feb 1, 2024

github-actions bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Feb 4, 2024

cortinico added the Never gets stale Prevent those issues and PRs from getting stale label Feb 4, 2024

Use asset catalog for ios images #30129

Are you sure you want to change the base?

Use asset catalog for ios images #30129

Conversation

janicduplessis commented Oct 7, 2020 • edited by cipolleschi Loading

Summary

Changelog:

Test Plan

Test on RN Tester

pull-bot commented Oct 7, 2020 • edited Loading

analysis-bot commented Oct 7, 2020 • edited Loading

analysis-bot commented Oct 7, 2020 • edited Loading

thymikee commented Nov 18, 2020

janicduplessis commented Jan 4, 2022

janicduplessis commented Jan 4, 2022

grabbou commented Feb 15, 2022

kelset commented Feb 15, 2022

cortinico commented Oct 4, 2022

janicduplessis commented Oct 4, 2022

cipolleschi commented Oct 5, 2022

facebook-github-bot commented Oct 5, 2022

cipolleschi commented Oct 5, 2022

janicduplessis commented Oct 5, 2022

janicduplessis commented Oct 5, 2022

cortinico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janicduplessis commented Oct 22, 2022

pull-bot commented Oct 22, 2022

pull-bot commented Oct 22, 2022

pull-bot commented Oct 22, 2022

pull-bot commented Oct 22, 2022

facebook-github-bot commented Nov 10, 2022

javache Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javache Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipolleschi commented Nov 17, 2022

janicduplessis commented Nov 17, 2022

cipolleschi commented Aug 4, 2023

cipolleschi commented Aug 4, 2023

facebook-github-bot commented Aug 4, 2023

github-actions bot commented Feb 1, 2024

janicduplessis commented Feb 3, 2024

janicduplessis commented Oct 7, 2020 •

edited by cipolleschi

Loading

pull-bot commented Oct 7, 2020 •

edited

Loading

analysis-bot commented Oct 7, 2020 •

edited

Loading

analysis-bot commented Oct 7, 2020 •

edited

Loading

javache Nov 10, 2022 •

edited

Loading

javache Nov 10, 2022 •

edited

Loading