New Major Version feedback
vonovak opened this issue · 12 comments
Hi!
Let me start by saying thanks for maintaining the library! :) I'm reaching out with regard to #1612 to provide some feedback for the new major version rewrite. I figured it'd be easier to open a new issue to keep discussion more focused.
Firstly, a clarifying question about "Package per font - Only install the fonts you need for smaller bundle sizes" which is listed as one of the benefits of the rewrite.
Does this concern the JS bundle size or the native binary size (because the "default" installation instructions result into copying all icon files into the binary)?
In terms of JS bundle size, I believe there shouldn't be a difference, because metro will bundle only the JS file that are imported in the JS code, so if I import import Icon from 'react-native-vector-icons/Ionicons', then only the JS code backing Ionicons will be bundled.
Secondly, I'd like to provide some feedback on the implementation. Please correct me if I'm wrong somewhere.
To render custom fonts, on both platforms, the font files need to be copied into the native app. This is all that needs to happen on Android, afaik. Then on iOS, the fonts also need to be added to Info.plist.
How font setup happens now
iOS
The copying of font files is currently done by this line in the podspec. The iOS installation instructions currently say that a manual step is needed, but I haven't found it to be necessary. In fact, when I follow the "iOS Setup" I get an error Multiple commands produce '.../iconsdemo.app/FontAwesome6_Regular.ttf'. I tried to improve the installation instructions in #1636.
Then, UIAppFonts need to be added to Info.plist.
Android
The fonts.gradle copies the font files. That's it.
How font setup happens in the new monorepo branch
iOS
The font file copying part is no longer taken care of by Cocoapods but by a custom bash script here.
Note that because the font files are copied into a react-native-vector-icons subfolder in the application bundle, the icon fonts won't render correctly even if they are added in the UIAppFonts entry in Info.plist. This I find unexpected and at odds with how apple covers adding custom fonts.
Icon rendering works in the end thanks to this loadFont() call. However, if standard setup was followed this call wouldn't be necessary. The implementation is necessary for dynamic font loading but for static fonts, the call doesn't need to happen. Let me also note that the promise returned by the call is not awaited so race conditions could happen when icon is rendered before the font is loaded.
The aforementioned bash script calls the getFonts.js node script which reads package.json and looks for all react-native-vector-icons subpackages and collects the font files from them. It appears it should work okay with monorepos 👍 .
Android
The getFonts.js script is called by build.gradle
Question for Android: With the new implementation, are the font files copied to the location documented here?
Summary
- I believe that for static fonts, the recommended approach to set them up should still include editing
info.plist. (For Expo users, this manual setup is not needed when using the Font config plugin) - on iOS, just for reference, this is how the Cocoapod's impl of copying assets looks. Maybe you'll be able to take some of that and not have to call your bash script "a bit of a hack" 🙂. You can also delegate the task of copying font files to Cocoapods, as it used to be, but each font package would need to specify a podspec.
Hi @vonovak.
Thanks for the feedback, apologies for the delay, I've been travelling.
Does this concern the JS bundle size or the native binary size (because the "default" installation instructions result into copying all icon files into the binary)?
That's a terminology misstep on my part. I do mean the native binary size. I'll update that everywhere.
NOTE: Those are the old installation instructions you linked to. See the new branch: https://github.com/oblador/react-native-vector-icons/tree/monorepo?#installation. In the new version, there is a script that copies only the fonts for installed packages into the bundle.
iOS
The key goal was to simplify installation and make it zero-touch. The majority of the project's user issues are caused by installation problems.
Based on some of the documentation you've shared, I think I might be overreaching here.
The script on iOS is mainly to avoid needing the podspec in every module, the fonts are auto generated though so this is not a biggy and we could include one.
Do you think it would be possible to either make use of some expo-font code or use it directly in some way as part of this library to not require users to edit Info.plist
Thanks for the gist, I'll definitely look at improving my hack :)
Android
Question for Android: With the new implementation, are the font files copied to the location documented here?
It doesn't copy them into the directory mentioned there, as that would mean they need to be git ignored or committed. The scripts place them in android/app/build/intermediates/assets/debug/fonts/, the same place they would end up if you added an assets entry to react-native-config.js.
Thanks for the feedback!
Hi @johnf!
I purposely linked to the current installation instructions - at the moment all font files are included in the binary. That changes with the new major version, so that's good.
You mentioned that most issues people have on iOS are around installing - I believe the installation instructions can be much simpler. I made a PR for that: #1636
The script on iOS is mainly to avoid needing the podspec in every module, the fonts are auto generated though so this is not a biggy and we could include one.
If the script that does the copying is solid, then I agree there's no need for podspec (again, just for completeness: how cocoapods do it).
Do you think it would be possible to either make use of some expo-font code or use it directly in some way as part of this library to not require users to edit Info.plist
It'd be nice to not have to change info.plist but given that it's the officially instructed way to add custom fonts, I think the package should stick with it for static fonts (= fonts that are present at build time, which covers the vast majority of how the fonts are used). Expo has a config plugin that adds the font entries to info.plist so no manual step is needed but I don't see an easy way to bring that into this package. But again, if the the docs are better I'm hopeful that the situation will improve.
why rely on changed in `info.plist`
With the current monorepo branch, loadFontWithFileName needs to be called and do some work before the icons can render (and we should await the result here before proceeding - otherwise there's potential for race conditions.). But if we take the path of changing plist file, there's no need to run this extra code and await its result / block the JS thread. I understand the desire to make the library "just work" after installing but it feel it's at the expense of what is right.
There are cases when users might want to load a font family dynamically (without changes to info.plist) - e.g. with OTA updates this can be the case. For that purpose I'm planning to contribute dynamic font loading from this branch. It relies on the presence of Expo, namely expo-font to determine if a font is loaded or not, and loading it. Because the font file can be served by metro bundler when developing locally, there's also some work around that. This still requires some changes inside expo but I'll try to contribute this soon.
Hope this makes sense, and thank you for explaining everything.
@vonovak I've just rolled a new release that moves back to a more static treatment of fonts on iOS
I did try a couple of techniques to try and dynamically edit Info.plist during the build process, but I couldn't quite find the right hook, so it didn't get overridden.
I have provided a script, though so that users can update their Info.plist with a single command which adds any missing fonts.
Thank you for making that change; that sounds like a good way to handle it!
@vonovak Are there any more pull requests coming? I's like to cut a new alpha release soon
@vonovak Thanks for all the help with the dynamic font loading and other improvements.
I'll clean up a couple of things and then push another alpha release this week.
hey @johnf I was wondering if you have a timeline for releasing stuff from the monorepo branch and making it the default?
Are there todo items / tasks that need resolving and would move us closer to that goal?
Also regarding 82b4fda I just wanted to point out that the naming of postscriptName and fontFilename seems inconsistent: it seems that the name part should be Name in both cases.
Thank you! 🙂
@vonovak I'm hoping to release a beta this weekend and then a full version in a couple of weeks.
My main focus is getting the tests working - I'm going to try switching from detox to owl (I've had all sorts of issues with detox, e.g. it just seems to crash the app on ios)
Regarding the naming, you are right. I'm going to switch it back. I think it's the leftover rubyisms in my brain that preferred filename, but FileName seems more correct in JS/TS land
@johnf great to hear that!
With tests - I believe there are better options than Detox for what you're trying to achieve. Detox's advantage is that it's "grey box" - it understands what's going on inside of the app which is useful for real apps. However, if you're trying to just boot a simple "icons gallery app" and take a few screenshots, it's not a real benefit.
Rather, the issue with it is that Detox supports specific React Native versions and doesn't officially work with the new architecture so it might block you from testing the most recent releases.
I'm not too familiar with react native owl so I cannot comment on it but Appium (as much as I don't like it because to me it seems it's trying to do a lot of things but only few work really properly) or Maestro (which I don't have experience with but heard positive comments on) would seems better choices for this purpose compared to Detox.