[intl] speed up localization generation and regenerate symbols #102614
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). 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. |
flutter-dashboard
bot
added
a: internationalization
|
waiting to see if this triggers a test somewhere... |
|
Nice, I was tracking this in b/225289302 too but I'll assign it to you then :) |
jonahwilliams
changed the title
| if (writeToFile) { | ||
| final File dateLocalizationsFile = File(path.join('packages', 'flutter_localizations', 'lib', 'src', 'l10n', 'generated_date_localizations.dart')); | ||
| dateLocalizationsFile.writeAsStringSync(buffer.toString()); | ||
| Process.runSync(path.join('bin', 'cache', 'dart-sdk', 'bin', 'dartfmt'), <String>[ |
There was a problem hiding this comment.
This no longer exists
There was a problem hiding this comment.
We may be missing a test here that should have failed when this was removed?
|
Would like some guidance on this. There appear to be no tests, but realy this is all an implementation detail anyway. Love to get your thoughts |
HansMuller
left a comment
HansMuller
left a comment
There was a problem hiding this comment.
This seems OK to me. Even if the performance difference was a wash, this seems like a better way to deal with the Date symbols..
| /// The subset of date symbols supported by the intl package which are also | ||
| /// supported by flutter_localizations.'''); | ||
| buffer.writeln('const Map<String, dynamic> dateSymbols = <String, dynamic> {'); | ||
| buffer.writeln('final Map<String, intl.DateSymbols> dateSymbols = <String, intl.DateSymbols> {'); |
There was a problem hiding this comment.
final implies lazy initialization like late final?
There was a problem hiding this comment.
Yes its lazily initialized, but not in an observable way like late final. I would make these const but intl.DateSymbol cannot be const due to the use of non-final fields
| dateLocalizationsFile.writeAsStringSync(buffer.toString()); | ||
| Process.runSync(path.join('bin', 'cache', 'dart-sdk', 'bin', 'dartfmt'), <String>[ | ||
| '-w', | ||
| final String extension = Platform.isWindows ? '.exe' : ''; |
There was a problem hiding this comment.
Seems odd that this has to be done "by hand"
There was a problem hiding this comment.
We wrote package:process to handle this back in the day
goderbauer
left a comment
goderbauer
left a comment
There was a problem hiding this comment.
Presumably, we have tests in place that cover using the dateSymbols? If those are still passing:
LGTM
| if (writeToFile) { | ||
| final File dateLocalizationsFile = File(path.join('packages', 'flutter_localizations', 'lib', 'src', 'l10n', 'generated_date_localizations.dart')); | ||
| dateLocalizationsFile.writeAsStringSync(buffer.toString()); | ||
| Process.runSync(path.join('bin', 'cache', 'dart-sdk', 'bin', 'dartfmt'), <String>[ |
There was a problem hiding this comment.
We may be missing a test here that should have failed when this was removed?
|
Yeah we don't appear to be running this on CI. Maybe we should, then we could check against a hash or something? |
|
test-exempt: code refactor with no semantic change |
|
This led to some size reductions on small apps, which is surprising IMO. |
6 participants
Cupertino localizations shows up in both the startup CPU and memory profile for customer money. This isn't actually because loading cupertino localizations is that slow, but since cupertino localizations loads first it triggers the parsing of the date symbols table from a const map. Skip this work by instead declaring the date symbols using the intl.DateSymbols class directly.