-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Add DynamicFeature system channel #71879
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
Conversation
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.
Should this be an enum that also exists on the framework side?
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 haven't fully decided on this. On one hand, we could fully define this in an enum, but custom implementers of DynamicFeatureManager may wish to have a custom and/or far more detailed set of states. Maybe show percentage downloaded, etc. An enum on just the framework side for the default implementation could exist for convenience, but I don't want to restrict this particular channel to just these states, as it has potential for more, and could be useful to the more advanced users we are targeting with this feature.
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.
Sure, you can decide. One thing to keep in mind, we'll want to bring this to iOS (at least the asset part) at some point https://developer.apple.com/library/archive/documentation/FileManagement/Conceptual/On_Demand_Resources_Guide/index.html.
Ideally the user should just have to touch the dart facing parts (yaml, asset files, deferred instructions in dart) and never touch anything platform specific or have to read platform specific values.
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'll defer adding enums to a later PR/decision.
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.
Please use standard method codec. I think having json elsewhere is a bit of a mistake but is generally less efficient, doesn't allow bytes transport and we shouldn't encourage the divergence more by using it more.
|
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. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
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.
You wanna cross-reference the dart API that does the same thing on deferred libraries?
|
LGTM |
ab5dab5 to
0022c48
Compare
Depends on Engine PR: flutter/engine#22833
Adds a new DynamicFeatureChannel method channel to
SystemChannels.Wrapper to expose methods coming in future PR.