consolidate AssetBundle::entries and AssetBundle::entryKinds into a new type, `AssetBundleEntry` #142029

andrewkolos · 2024-01-23T02:41:45Z

Part of work on #141194

The AssetBundle class contains two members, entries and entryKinds. entries contains asset data indexed by asset key. entryKinds contains the "kinds" of these assets, again indexed by asset key.

Change. Rather than have two separate maps, this PR proposes combining these maps into one by wrapping the asset data and kind into a single data type AssetBundleEntry.

Purpose. In #141194, I am considering associating more information with an asset. In particular, what transformers are meant to be applied to it when copying it to the build output. Rather than adding another map member onto AssetBundle (e.g. entryTransformers), I decided to make things neater by introducing the AssetBundleEntry type.

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.

…tries

andrewkolos · 2024-01-23T07:17:28Z

packages/flutter_tools/lib/src/asset.dart

+  final DevFSContent content;
+  final AssetKind kind;
+
+  Future<List<int>> contentsAsBytes() => content.contentsAsBytes();


DevFSContent::contentsAsBytes is called so often that I decided to expose a convenience getter for it.

I'll leave it up to you, but my personal preference is against these kinds of convenience methods, as they improve the efficiency of writing code at the cost of reading it (I find myself jumping to definition when I'm debugging and see a function like this, fearing it might be a 200 liner with complex error handling).

andrewkolos · 2024-01-23T07:29:38Z

packages/flutter_tools/lib/src/asset.dart

+@immutable
+final class AssetBundleEntry {
+  const AssetBundleEntry(this.content, {
+    this.kind = AssetKind.regular,


AssetKind.regular was used as a default in a few different places, so I decided to make it a default here.

can we at least add a dartdoc to AssetKind.regular--I don't personally know what a "regular" asset is.

Actually, the more I think about this, the more I worry we introduce bugs where we accidentally let this fall back AssetKind.regular. Can you convince me otherwise?

The problem is that a "regular" asset is simply what it isn't—a font, shader, or model. I can document that, but I don't know if negative definitions are particularly helpful.

Actually, the more I think about this, the more I worry we introduce bugs where we accidentally let this fall back AssetKind.regular. Can you convince me otherwise?

No, to be honest. Parameters with default values always risk this as a tradeoff for less noise throughout the rest of the code. I'm happy to make it required. I originally opted not to for the initial PR review since it would have made the diff more intimidating.

The problem is that a "regular" asset is simply what it isn't—a font, shader, or model. I can document that, but I don't know if negative definitions are particularly helpful.

Interesting, so it basically means "just copy, don't do any pre-processing"?

also, what's a model?

Interesting, so it basically means "just copy, don't do any pre-processing"?

Yeah, my hope is that the docstring for AssetBundleEntry alludes-to/implies this.

As an example, you can see this stuff in devfs.dart.

also, what's a model?

A file containing data for a 3D model, I believe.

packages/flutter_tools/lib/src/asset.dart

packages/flutter_tools/test/general.shard/asset_bundle_test.dart

christopherfujino

LGTM, my only real concern is whether allowing AssetBundleEntry(kind: ...) to be optional is a footgun.

christopherfujino · 2024-01-23T20:08:19Z

Also, this is a great refactor!

…into a new type, `AssetBundleEntry` (flutter/flutter#142029)

flutter/flutter@5b673c2...19b06f4 2024-01-24 [email protected] Roll Flutter Engine from 7c592dbfa032 to ba3a70ce7722 (2 revisions) (flutter/flutter#142130) 2024-01-24 [email protected] Roll Flutter Engine from 700235c85d4a to 7c592dbfa032 (2 revisions) (flutter/flutter#142108) 2024-01-24 [email protected] Roll Flutter Engine from da96f9de3f69 to 700235c85d4a (1 revision) (flutter/flutter#142100) 2024-01-24 [email protected] Roll Flutter Engine from 220416cfa6ca to da96f9de3f69 (5 revisions) (flutter/flutter#142098) 2024-01-24 [email protected] Don't change the height of the Textfield's labelStyle when it focused. (flutter/flutter#141943) 2024-01-24 [email protected] Ignore a leak. (flutter/flutter#141737) 2024-01-24 [email protected] [github actions] FINALLY fix the mysterious token error in github actions (flutter/flutter#142058) 2024-01-24 [email protected] Revert "Add abifilters to our gradle templates" (flutter/flutter#142089) 2024-01-23 [email protected] Roll Flutter Engine from 0b9b538bd458 to 220416cfa6ca (2 revisions) (flutter/flutter#142079) 2024-01-23 [email protected] Add abifilters to our gradle templates (flutter/flutter#135529) 2024-01-23 [email protected] Roll Flutter Engine from 11a16d83ab6c to 0b9b538bd458 (2 revisions) (flutter/flutter#142076) 2024-01-23 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 4.2.0 to 4.3.0 (flutter/flutter#142075) 2024-01-23 [email protected] consolidate AssetBundle::entries and AssetBundle::entryKinds into a new type, `AssetBundleEntry` (flutter/flutter#142029) 2024-01-23 [email protected] Roll Flutter Engine from 57d6b518f920 to 11a16d83ab6c (4 revisions) (flutter/flutter#142071) 2024-01-23 [email protected] Roll Flutter Engine from 994054143fc3 to 57d6b518f920 (3 revisions) (flutter/flutter#142067) 2024-01-23 [email protected] [flutter_tools] fix language_version_test and enable shuffle (flutter/flutter#142009) 2024-01-23 [email protected] Merge flutter_goldens_client into flutter_goldens (flutter/flutter#141900) 2024-01-23 [email protected] Allow overriding the native assets yaml file in the resident runner. (flutter/flutter#142016) 2024-01-23 [email protected] Roll Flutter Engine from b229878c57f5 to 994054143fc3 (1 revision) (flutter/flutter#142059) 2024-01-23 [email protected] Update material banner maxScaleFactor to meet GAR requirement (flutter/flutter#142015) 2024-01-23 [email protected] Add tooltip for the clear button on `SearchAnchor`'s search view (flutter/flutter#141804) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

consolidate AssetBundle::entries and AssetBundle::entryKinds

c1107a9

github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jan 23, 2024

andrewkolos added 4 commits January 22, 2024 18:59

try fix test

140841c

improve docstrings of AssetBundle::entries and ::deferredComponentsEn…

db584ce

…tries

fix more tests

9d6c5cc

lint

7c9d5e0

andrewkolos commented Jan 23, 2024

View reviewed changes

andrewkolos marked this pull request as ready for review January 23, 2024 07:33

andrewkolos requested a review from christopherfujino January 23, 2024 18:44

andrewkolos changed the title ~~consolidate AssetBundle::entries and AssetBundle::entryKinds into a new type AssetBundleEntry~~ consolidate AssetBundle::entries and AssetBundle::entryKinds into a new type, AssetBundleEntry Jan 23, 2024

christopherfujino reviewed Jan 23, 2024

View reviewed changes

packages/flutter_tools/lib/src/asset.dart Show resolved Hide resolved

christopherfujino reviewed Jan 23, 2024

View reviewed changes

packages/flutter_tools/lib/src/asset.dart Outdated Show resolved Hide resolved

christopherfujino reviewed Jan 23, 2024

View reviewed changes

packages/flutter_tools/lib/src/asset.dart Show resolved Hide resolved

christopherfujino reviewed Jan 23, 2024

View reviewed changes

packages/flutter_tools/test/general.shard/asset_bundle_test.dart Outdated Show resolved Hide resolved

christopherfujino approved these changes Jan 23, 2024

View reviewed changes

andrewkolos added 2 commits January 23, 2024 12:33

make kind parameter on AssetBundleEntry() required

f3565e4

nit

4e9f388

andrewkolos added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 23, 2024

auto-submit bot merged commit cbe0cea into flutter:master Jan 23, 2024

engine-flutter-autoroll mentioned this pull request Jan 24, 2024

Roll Flutter from 5b673c28e4ac to 19b06f4ee9a0 (21 revisions) flutter/packages#5968

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 24, 2024

cbe0cea consolidate AssetBundle::entries and AssetBundle::entryKinds …

45e48b7

…into a new type, `AssetBundleEntry` (flutter/flutter#142029)

huycozy mentioned this pull request Feb 15, 2024

[flutter_tools] The asset bundle is not rebuilt if an asset file is edited between flutter test commands #143513

Closed

andrewkolos deleted the consolidate-entries-and-entry-kinds branch April 29, 2024 18:18

consolidate AssetBundle::entries and AssetBundle::entryKinds into a new type, AssetBundleEntry #142029

consolidate AssetBundle::entries and AssetBundle::entryKinds into a new type, AssetBundleEntry #142029

Uh oh!

Conversation

andrewkolos commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewkolos Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

christopherfujino commented Jan 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

consolidate AssetBundle::entries and AssetBundle::entryKinds into a new type, `AssetBundleEntry` #142029

consolidate AssetBundle::entries and AssetBundle::entryKinds into a new type, `AssetBundleEntry` #142029

andrewkolos commented Jan 23, 2024 •

edited

Loading

christopherfujino Jan 23, 2024 •

edited

Loading

andrewkolos Jan 23, 2024 •

edited

Loading