From e10e9a90afb6846420da63ca59329a74efd4dcae Mon Sep 17 00:00:00 2001 From: Kate Lovett Date: Tue, 23 Apr 2024 11:40:29 -0500 Subject: [PATCH] Update icon tree shaker to allow system font fallback (#147202) Fixes https://github.com/flutter/flutter/issues/147189 This allows const `IconData` to fallback to the system font if `fontFamily` is not provided. A similar non-fatal error occurs when IconData specifies a font that is not included in the manifest, so I modeled after that error message: https://github.com/flutter/flutter/blob/b4121a1867a93e030377b3509147e7fdbf6517d5/packages/flutter_tools/lib/src/build_system/targets/icon_tree_shaker.dart#L122 --- .../targets/icon_tree_shaker.dart | 16 ++- .../targets/icon_tree_shaker_test.dart | 100 ++++++++++++++++++ 2 files changed, 113 insertions(+), 3 deletions(-) diff --git a/packages/flutter_tools/lib/src/build_system/targets/icon_tree_shaker.dart b/packages/flutter_tools/lib/src/build_system/targets/icon_tree_shaker.dart index 916d1b3f592..d8848d91225 100644 --- a/packages/flutter_tools/lib/src/build_system/targets/icon_tree_shaker.dart +++ b/packages/flutter_tools/lib/src/build_system/targets/icon_tree_shaker.dart @@ -327,15 +327,25 @@ class IconTreeShaker { final Object? package = iconDataMap['fontPackage']; final Object? fontFamily = iconDataMap['fontFamily']; final Object? codePoint = iconDataMap['codePoint']; - if ((package ?? '') is! String || // Null is ok here. - fontFamily is! String || + if ((package ?? '') is! String || + (fontFamily ?? '') is! String || codePoint is! num) { throw IconTreeShakerException._( 'Invalid ConstFinder result. Expected "fontPackage" to be a String, ' '"fontFamily" to be a String, and "codePoint" to be an int, ' 'got: $iconDataMap.'); } - final String family = fontFamily; + if (fontFamily == null) { + _logger.printTrace( + 'Expected to find fontFamily for constant IconData with codepoint: ' + '$codePoint, but found fontFamily: $fontFamily. This usually means ' + 'you are relying on the system font. Alternatively, font families in ' + 'an IconData class can be provided in the assets section of your ' + 'pubspec.yaml, or you are missing "uses-material-design: true".', + ); + continue; + } + final String family = fontFamily as String; final String key = package == null ? family : 'packages/$package/$family'; diff --git a/packages/flutter_tools/test/general.shard/build_system/targets/icon_tree_shaker_test.dart b/packages/flutter_tools/test/general.shard/build_system/targets/icon_tree_shaker_test.dart index cb493e7cec7..e4dc7f1ec9b 100644 --- a/packages/flutter_tools/test/general.shard/build_system/targets/icon_tree_shaker_test.dart +++ b/packages/flutter_tools/test/general.shard/build_system/targets/icon_tree_shaker_test.dart @@ -561,6 +561,90 @@ void main() { expect(processManager, hasNoRemainingExpectations); }); + testWithoutContext('Allow system font fallback when fontFamily is null', () async { + final Environment environment = createEnvironment({ + kIconTreeShakerFlag: 'true', + kBuildMode: 'release', + }); + final File appDill = environment.buildDir.childFile('app.dill') + ..createSync(recursive: true); + + // Valid manifest, just not using it. + fontManifestContent = DevFSStringContent(validFontManifestJson); + + final IconTreeShaker iconTreeShaker = IconTreeShaker( + environment, + fontManifestContent, + logger: logger, + processManager: processManager, + fileSystem: fileSystem, + artifacts: artifacts, + targetPlatform: TargetPlatform.android, + ); + + addConstFinderInvocation(appDill.path, stdout: emptyConstFinderResult); + // Does not throw + await iconTreeShaker.subsetFont( + input: fileSystem.file(inputPath), + outputPath: outputPath, + relativePath: relativePath, + ); + + expect( + logger.traceText, + contains( + 'Expected to find fontFamily for constant IconData with codepoint: ' + '59470, but found fontFamily: null. This usually means ' + 'you are relying on the system font. Alternatively, font families in ' + 'an IconData class can be provided in the assets section of your ' + 'pubspec.yaml, or you are missing "uses-material-design: true".\n' + ), + ); + expect(processManager, hasNoRemainingExpectations); + }); + + testWithoutContext('Allow system font fallback when fontFamily is null and manifest is empty', () async { + final Environment environment = createEnvironment({ + kIconTreeShakerFlag: 'true', + kBuildMode: 'release', + }); + final File appDill = environment.buildDir.childFile('app.dill') + ..createSync(recursive: true); + + // Nothing in font manifest + fontManifestContent = DevFSStringContent(emptyFontManifestJson); + + final IconTreeShaker iconTreeShaker = IconTreeShaker( + environment, + fontManifestContent, + logger: logger, + processManager: processManager, + fileSystem: fileSystem, + artifacts: artifacts, + targetPlatform: TargetPlatform.android, + ); + + addConstFinderInvocation(appDill.path, stdout: emptyConstFinderResult); + // Does not throw + await iconTreeShaker.subsetFont( + input: fileSystem.file(inputPath), + outputPath: outputPath, + relativePath: relativePath, + ); + + expect( + logger.traceText, + contains( + 'Expected to find fontFamily for constant IconData with codepoint: ' + '59470, but found fontFamily: null. This usually means ' + 'you are relying on the system font. Alternatively, font families in ' + 'an IconData class can be provided in the assets section of your ' + 'pubspec.yaml, or you are missing "uses-material-design: true".\n' + ), + ); + expect(processManager, hasNoRemainingExpectations); + }); + testWithoutContext('ConstFinder non-zero exit', () async { final Environment environment = createEnvironment({ kIconTreeShakerFlag: 'true', @@ -609,6 +693,20 @@ const String validConstFinderResult = ''' } '''; +const String emptyConstFinderResult = ''' +{ + "constantInstances": [ + { + "codePoint": 59470, + "fontFamily": null, + "fontPackage": null, + "matchTextDirection": false + } + ], + "nonConstantLocations": [] +} +'''; + const String constFinderResultWithInvalid = ''' { "constantInstances": [ @@ -668,3 +766,5 @@ const String invalidFontManifestJson = ''' ] } '''; + +const String emptyFontManifestJson = '[]';