[google_fonts] Add google_fonts_lite file to allow tree-shaking of the other huge files#11433
[google_fonts] Add google_fonts_lite file to allow tree-shaking of the other huge files#11433TheCarpetMerchant wants to merge 3 commits intoflutter:mainfrom
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Code Review
This pull request introduces a 'lite' version of the Google Fonts generator to support tree-shaking by creating a GoogleFontsLite class. Key changes include updating the generator to process a new template, making the GoogleFontsFile constructor const, and adding the google_fonts_lite.tmpl file. Review feedback recommends fixing a typo in the template comments, declaring the GoogleFontsLite class as abstract with a static const map, using display names for font keys, and implementing safer error handling in the getFont method to replace the current null-assertion operator.
| // That is used to allow tree-shaking to remove all of the _parts files. | ||
| // If you only call GoogleFontsLite.fontsMap or GoogleFontsLite.loadFont(), | ||
| // the code in the GoogleFonts class and its parts classes is never called. | ||
| // WHat is called is googleFontsTextStyle, which attempts to load a font if not already loaded. |
| class GoogleFontsLite { | ||
| static Map<String, Map<GoogleFontsVariant, GoogleFontsFile>> fontsMap = { | ||
| {{#fontEntries}} | ||
| '{{fontFamily}}': { | ||
| {{#fontUrls}} | ||
| const GoogleFontsVariant(fontWeight: FontWeight.w{{variantWeight}}, fontStyle: FontStyle.{{variantStyle}},) : const GoogleFontsFile('{{hash}}', {{length}},), | ||
| {{/fontUrls}} | ||
| }, | ||
| {{/fontEntries}} | ||
| }; |
There was a problem hiding this comment.
Consider making GoogleFontsLite an abstract class to prevent instantiation, as it only contains static members. Additionally, the fontsMap should be declared as static const for better efficiency. Using fontFamilyDisplay (the actual font name with spaces) as the map key is also recommended for better compatibility with font fallback and consistency with the main GoogleFonts API.
abstract class GoogleFontsLite {
static const Map<String, Map<GoogleFontsVariant, GoogleFontsFile>> fontsMap = {
{{#fontEntries}}
'{{fontFamilyDisplay}}': {
{{#fontUrls}}
const GoogleFontsVariant(fontWeight: FontWeight.w{{variantWeight}}, fontStyle: FontStyle.{{variantStyle}},) : const GoogleFontsFile('{{hash}}', {{length}},),
{{/fontUrls}}
},
{{/fontEntries}}
};
| static TextStyle getFont(String fontFamily) { | ||
| return googleFontsTextStyle(fontFamily: fontFamily, fonts: fontsMap[fontFamily]!); | ||
| } |
There was a problem hiding this comment.
The getFont method uses the null-assertion operator !, which will cause a runtime crash if an unknown font family is requested. It is safer to check for the existence of the font and throw a more descriptive error.
static TextStyle getFont(String fontFamily) {
final fonts = fontsMap[fontFamily];
if (fonts == null) {
throw ArgumentError('No font family with name $fontFamily was found.');
}
return googleFontsTextStyle(fontFamily: fontFamily, fonts: fonts);
}
Adds a
google_fonts_lite.dartgenerated file, which contains a map offontFamilyto fonts variants and agetFontfunction that simply forwards the call to the internalgoogleFontsTextStylefunction which does the downloading and loading of the font.List which issues are fixed by this PR. You must list at least one issue.
flutter/flutter#184337
I have not done documentation, testing etc because this PR is for discussion purposes. I'd like approval of the concept before doing the paperwork.
The idea is to provide a lite version of the
GoogleFontsclass. This lite version does not depend on any of the other generated code, which is absolutely huge and takes up a massive amount of space (8MB on android for v8.0.0). By using this lite version only, the compiler is able to tree-shake all that additionnal code while you retain the ability to invoke any font from the package viagetFont.For me this brings the package's footprint from 8MB/target platform to less than a 1MB/target when building a release apk.