Skip to content

Conversation

@GaryQian
Copy link
Contributor

@GaryQian GaryQian commented Dec 7, 2020

Depends on Engine PR: flutter/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
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
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
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
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
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

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
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
Copy link
Member

xster commented Dec 10, 2020

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