Skip to content

Add DynamicFeature system channel - #71879

Merged
GaryQian merged 7 commits into
flutter:masterfrom
GaryQian:splitaotchannel
Dec 12, 2020
Merged

GaryQian merged 7 commits into
flutter:masterfrom
GaryQian:splitaotchannel

Conversation

@GaryQian

@GaryQian GaryQian commented Dec 7, 2020 •

Copy link
Copy Markdown
Contributor

Depends on Engine PR: flutter-team-archive/engine#22833

Adds a new DynamicFeatureChannel method channel to SystemChannels.

Wrapper to expose methods coming in future PR.

@GaryQian GaryQian added platform-android Android applications specifically engine flutter/engine related. See also e: labels. labels Dec 7, 2020
@flutter-dashboard flutter-dashboard Bot added the framework flutter/packages/flutter repository. See also f: labels. label Dec 7, 2020
@google-cla google-cla Bot added the cla: yes label Dec 7, 2020

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Member

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.

@GaryQian GaryQian changed the title [WIP] Add DynamicFeatures system channel Add DynamicFeatures system channel Dec 8, 2020
@GaryQian GaryQian changed the title Add DynamicFeatures system channel Add DynamicFeature system channel Dec 10, 2020
@GaryQian
GaryQian marked this pull request as ready for review December 10, 2020 07:05
@flutter-dashboard

Copy link
Copy Markdown

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.

Copy link
Copy Markdown
Member

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?

@xster

xster commented Dec 10, 2020

Copy link
Copy Markdown
Member

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

engine flutter/engine related. See also e: labels. framework flutter/packages/flutter repository. See also f: labels. platform-android Android applications specifically

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants