-
-
Notifications
You must be signed in to change notification settings - Fork 153
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for deferred components #576
base: main
Are you sure you want to change the base?
Add support for deferred components #576
Conversation
}); | ||
|
||
// Merge the deferred assets with the main assets. | ||
return [...config.pubspec.flutter.assets, ...deferredAssets]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't [...?config.pubspec.flutter.deferredAssets?.assets]
do the same thing as above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah exactly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry. For a moment I thought it was the same but it is not. I need to revert it.
The fact is we may have many Deferret Components, with many assets each (a list of lists).
So before spreading the list of deferredComponents, it is necessary to spread the assets from each deferredComponent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you may reduce components' assets into one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like reduce((list, assets) => list + assets)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ianmaciel Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here it is :)
Also we need a test to make sure it works well |
Hi @AlexV525, I pushed fixups for this pr, if this is okay please let me know so can rebase squashing those fixups. |
Changes are good but still missing tests. You don't need to squash them as we'll squash them to commit eventually. |
859d10e
to
564f01b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #576 +/- ##
==========================================
+ Coverage 97.64% 98.06% +0.41%
==========================================
Files 24 23 -1
Lines 891 929 +38
==========================================
+ Hits 870 911 +41
+ Misses 21 18 -3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RSLGTM
/// Merge the deferred assets with the main assets. | ||
List<Object> _buildAssetsList(Config config) { | ||
// We may have several deferred components, with a list of assets for each. | ||
// So before spreading the list of deferred components, we need to spread | ||
// the list of assets for each deferred component. | ||
final deferredComponents = config.pubspec.flutter.deferredComponents ?? []; | ||
return deferredComponents.fold<List<Object>>( | ||
config.pubspec.flutter.assets, | ||
(list, deferredComponent) => list + (deferredComponent.assets ?? []), | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: For better readability maybe:
/// Merge the deferred assets with the main assets. | |
List<Object> _buildAssetsList(Config config) { | |
// We may have several deferred components, with a list of assets for each. | |
// So before spreading the list of deferred components, we need to spread | |
// the list of assets for each deferred component. | |
final deferredComponents = config.pubspec.flutter.deferredComponents ?? []; | |
return deferredComponents.fold<List<Object>>( | |
config.pubspec.flutter.assets, | |
(list, deferredComponent) => list + (deferredComponent.assets ?? []), | |
); | |
} | |
/// Build assets from the main list and the deferred components. | |
List<Object> _buildFlutterAssetsList(Flutter flutter) { | |
final flutterAssets = flutter.assets; | |
// We may have several deferred components, with a list of assets for each. | |
// So before spreading the list of deferred components, we need to spread | |
// the list of assets for each deferred component. | |
final deferredComponents = flutter.deferredComponents ?? []; | |
return deferredComponents.fold( | |
flutterAssets, | |
(list, component) => list + (component.assets ?? []), | |
); | |
} |
What does this change?
This PR include support for Flutter Deferred Components.
Deferred components allow developers to the app into multiple apk to reduce its size. This can be used to optimize the initial download and download components only on necessary but it is also mandatory for apk with more than 200MB.
This library initially looks for assets listed on
flutter.assets
ofpubspec.yaml
, with this change the library will continue looking for the assets lists underflutter.assets
but will merge with the list of assets included onflutter.deferred-components.$
. This will be merged in a single list.Ideally in the future assets should have different classname, so users will easily know when dealing with deferred components. This wasn't implemented at this moment because it would requeire a significative changes.
Fixes #577 🎯
Type of change
Please delete options that are not relevant.
Checklist:
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
melos run test
)melos run format
to automatically apply formatting)