Skip to content

Conversation

@andrewkolos
Copy link
Contributor

@andrewkolos andrewkolos commented Dec 6, 2022

Overview

This change introduces a public API for inspecting what assets are available within a Flutter app. Currently, there is no officially-supported way to do this, and some apps--needing this ability--depend on undocumented behavior internal to Flutter.

Example use case

The google_fonts package, when loading a font, first checks if the font is already available in the app. If it is not available, it will load it from fonts.google.com. Code.

Other known use cases:

  • Preemptively searching for available shaders and then warming them up via a call over a MethodChannel.
  • Dynamically listing assets (and maybe loading) all assets that match some criterion (e.g. are within some arbitrry directory).
  • Tests.

Context

Terminology

The asset manifest. The framework, when performing it's first asset load, has to load a file called the asset manifest, which contains information about assets available to the app (e.g. where to find higher-resolution variants of images). This file is created at build time by the tool. It is meant for framework-internal use--but some Flutter apps, including ones internal to google, read the file directly.

Why we should make this change now

#113637 "replaced" the aforementioned asset manifest (hereafter "legacy manifest") with a new one that enables more performant first loads and has a more flexible schema. However, this PR did not remove all references to the legacy manifest file, since doing so would break apps that directly depend on it1. In addition, g3 Flutter does not yet generate the new asset manifest file, so--when loading an asset--the framework needs to be able to fallback to the legacy manifest file in the event that the new one isn't present (see #114913).

Still, we do ultimately want to get rid of the legacy asset manifest (#114913). Rather than having users migrate to reading a new framework-internal file, we can use this opportunity to introduce a dedicated, publicly-documented API for listing what assets are available. Users can instead migrate to this new API. This results in less hacky user code and also gives Flutter maintainers more freedom/flexibility to change the internals of asset loading without worrying about breaking user code.

Design

We add a new class, AssetManifest, that is responsible for loading and parsing the asset manifest file contents from an AssetBundle by calling loadStructuredBinaryData (and loadStructuredData as a fallback in case only the legacy asset manifest file is available). This loading will invoked via static factory methods (e.g. AssetManifest.loadFromAssetBundle). AssetManifest will expose two instance methods, listAssets and getAssetMetadata.

  • Iterable<String> listAssets() will simply enumerate the list of assets in the manifest by their keys. This provides the capability that we know users need.
  • Object? getAssetMetadata(String key) will get the value of the manifest entry corresponding to the asset having the given key. This is meant for use by the framework (i.e. for choosing which image asset variants to load). Since this API is public, the return type is intentionally vague to discourage user code from depending on its shape2.

Drawbacks

We really want the framework be the only one to ever calling getAssetMetadata. While the return type being vague discourages users from depending on the shape of returned data, it doesn't stop them from inspecting it at runtime and depending on hardcoded object keys. However, there's no clear alternative to having this method. Here's one I can think of: have listAssets (which can be moved to AssetBundle if desired) and the existing image resolution logic both independently load the asset manifest file. The parser they use will be have to be identical, since the first parsed result could be cached within AssetBundle. In addition, both will need to support logic for falling back to the legacy asset manifest, which would be a lot of duplicated (if temporary) code.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Footnotes

  1. Normally, it might be fair to perform such a breaking change, because the asset manifest is not a publicly documented/supported concept. However, it's possible that some users, when consulting with Flutter maintainers, were told about the file and that they could use it to accomplish the asset-related behavior they were looking for. As a result, it would be unfair to break the older behavior without a full deprecation cycle.

  2. It is difficult to use a type other than Object? for the map entry value type unless we commit to the idea that values only ever represent list of image variants.

@flutter-dashboard flutter-dashboard bot added framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Dec 6, 2022
@andrewkolos
Copy link
Contributor Author

@jonahwilliams This is still WIP but it can be reviewed from a design/approach standpoint.

@jonahwilliams
Copy link
Contributor

Flutter generally uses a layered architecture. What this means more specifically is that when we have one system (say manifest loading and parsing) that needs to talk to another system (say an image widget/loader), we try to design this system so that these systems communicate via a public API.

There are a number of advantages to this approach:

  • It forces us to document, test, and use our APIs, rather than keeping them hidden.
  • It allows other developers to build on these lower level APIs and provide different solutions than those that come packaged in the SDK.

So given that, I would design the getAssetMetadata API around an opaque type that we try to keep secret. Instead I would provide a well documented and rich object that someone besides us could use to create a different asset selection system.

@jonahwilliams
Copy link
Contributor

aghhh "I would " -> "I wouldn't"

@andrewkolos
Copy link
Contributor Author

Thanks for the detailed response!

So given that, I wouldn't design the getAssetMetadata API around an opaque type that we try to keep secret. Instead I would provide a well documented and rich object that someone besides us could use to create a different asset selection system.

I agree that the exposed manifest data being weakly typed is far from ideal, and providing a stronger concrete type for manifest entry values has notable benefits1.

I've implemented this in my latest commit with the following changes:

  • Introduce another public type, AssetVariant. This is a data object that contains information about an asset variant, namely its key, and its target device pixel ratio. This is very similar to the _AssetVariant which used to be in image_resolution.dart.
  • getAssetMetadata now returns a List.
  • getAssetMetadata is renamed to getAssetVariants. Since we are giving up flexibility in what the asset manifest contains, we may as well make the name of the API more specific.

One tradeoff is that the shape of the asset manifest data would become harder to change.

It allows other developers to build on these lower level APIs and provide different solutions than those that come packaged in the SDK.
...
I would provide a well documented and rich object that someone besides us could use to create a different asset selection system.

Interesting point. Is the idea that custom AssetBundleImageProvider implementations could still use the existing asset manifest in their logic? I can't imagine many such use cases, but I'm sure could be surprised. One example I can think of it's implementing theme-aware image selection--you check for the current theme, search through the list of keys of available assets, and see if there is one that contains the themes name, or something like that.

Footnotes

  1. In addition to enabling custom asset selection, a concrete type would go a long way in making test code more readable and less brittle (there is a lot of indexing into objects with magic strings). It would also maintain the existing readability of image resolution logic.

@flutter-dashboard
Copy link

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@goderbauer
Copy link
Member

(Triage): @andrewkolos Do you still have plans for this PR?

@andrewkolos
Copy link
Contributor Author

@goderbauer Sorry for letting this go stale. This PR will be superseded by #118410, which I will mark as ready-for-review very soon.

@andrewkolos andrewkolos deleted the add-asset-manifest-api branch April 21, 2023 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants