[web] Make asset manifest immediately available to app (no network request) #130419

andrewkolos · 2023-07-12T16:06:23Z

Fixes #124883. See #128456 for more history.

Context

The asset manifest is a file generated by the tool at build time that lists available assets and their variants. The framework loads this file when resolving assets (e.g. from an Image.asset call) (It's also meant to be pre-loaded when the app first loads, but this is currently broken, see #130455).

However, should this load operation fail, assets will fail to load unless the end-user of the app re-opens it. This is far more likely to happen in Flutter web, since file loads must happen over a potentially unreliable network where requests can fail or be intercepted.

Fix

At build time, we take the contents of the asset manifest and include it as a constant in the code we generate for web app's entry point. This way, the asset manifest is available without making a network request specifically for it.

We do this for both the flutter run -d <browser> and flutter build web workflows. Arguably, it would be sufficient to do this only for flutter build web (to keep things simpler); but issues like #124883 would still be confusing for developers, and they would be rightfully concerned that the issue would still affect end users.

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.

packages/flutter_tools/lib/src/web/file_generators/main_dart.dart

packages/flutter/lib/src/services/_asset_manifest_contents_io.dart

andrewkolos · 2023-07-13T20:38:06Z

@jonahwilliams I found a problem with this approach. It works for flutter run since we build the AssetBundle before generating the entry point (we could even pass the contents directly to the entry point generating code to avoid using a dart global here).

However, this doesn't work for flutter build web because 1) the web entry point is generated completely independently of asset bundle generation and, more importantly, 2) the asset bundle is built after the web entry point is generated. To be more specific, the WebReleaseBundle target builds the AssetBundle (via copyAssets), and the entry point through this target's dependency chain.

There might be a way to rework these targets to make these work, but if it turns out to be difficult, I may end up just generating another JSON asset manifest, as much I would not like to.

jonahwilliams · 2023-07-13T20:40:22Z

Just change the dependency chain?

andrewkolos · 2023-07-13T20:42:35Z

Just change the dependency chain?

Yeah that's what I am going to try. In hindsight, I didn't mean to sound so pessimistic lol

andrewkolos · 2023-07-18T23:28:05Z

I tried changing the dependency chain for web builds to build the asset bundle before compiling any code (so we can inject the asset manifest into code before compiling) (see db5bbc3). However, this didn't work because icon tree shaking (which is part of copyAssets) depends on compilation so it can read app.dill to figure out what icons it can treeshake. Trying to build the asset bundle first via copyAssets will result in an error akin to this:

error

fyi: web_asset_bundle is the name of a new target I added that comes first in the Target chain.

Target web_asset_bundle failed: IconTreeShakerException: Expected to find kernel file at /Users/andrewkolos/Documents/GitHub/flutter_issue_repros/simple_asset_issues/.dart_tool/flutter_build/5aacfc7995e86607c01b55086eae64eb/app.dill, but no file found.

To disable icon tree shaking, pass --no-tree-shake-icons to the requested flutter build command
#0      IconTreeShaker._getIconData (package:flutter_tools/src/build_system/targets/icon_tree_shaker.dart:102:7)
#1      IconTreeShaker.subsetFont (package:flutter_tools/src/build_system/targets/icon_tree_shaker.dart:180:36)
<asynchronous suspension>
#2      copyAssets.<anonymous closure> (package:flutter_tools/src/build_system/targets/assets.dart:128:25)
<asynchronous suspension>
#3      Future.wait.<anonymous closure> (dart:async/future.dart:523:21)
<asynchronous suspension>
#4      copyAssets (package:flutter_tools/src/build_system/targets/assets.dart:106:3)
<asynchronous suspension>
#5      WebAssetBundleTarget.build (package:flutter_tools/src/build_system/targets/web.dart:52:29)
<asynchronous suspension>
#6      _BuildInstance._invokeInternal (package:flutter_tools/src/build_system/build_system.dart:853:9)
<asynchronous suspension>
#7      Future.wait.<anonymous closure> (dart:async/future.dart:523:21)
<asynchronous suspension>
#8      _BuildInstance.invokeTarget (package:flutter_tools/src/build_system/build_system.dart:791:32)
<asynchronous suspension>
#9      Future.wait.<anonymous closure> (dart:async/future.dart:523:21)
<asynchronous suspension>
#10     _BuildInstance.invokeTarget (package:flutter_tools/src/build_system/build_system.dart:791:32)
<asynchronous suspension>
#11     Future.wait.<anonymous closure> (dart:async/future.dart:523:21)
<asynchronous suspension>
#12     _BuildInstance.invokeTarget (package:flutter_tools/src/build_system/build_system.dart:791:32)
<asynchronous suspension>
#13     Future.wait.<anonymous closure> (dart:async/future.dart:523:21)
<asynchronous suspension>
#14     _BuildInstance.invokeTarget (package:flutter_tools/src/build_system/build_system.dart:791:32)
<asynchronous suspension>
#15     FlutterBuildSystem.build (package:flutter_tools/src/build_system/build_system.dart:620:16)
<asynchronous suspension>
#16     WebBuilder.buildWeb (package:flutter_tools/src/web/compile.dart:89:34)
<asynchronous suspension>
#17     BuildWebCommand.runCommand (package:flutter_tools/src/commands/build_web.dart:198:5)
<asynchronous suspension>
#18     FlutterCommand.run.<anonymous closure> (package:flutter_tools/src/runner/flutter_command.dart:1322:27)
<asynchronous suspension>
#19     AppContext.run.<anonymous closure> (package:flutter_tools/src/base/context.dart:150:19)
<asynchronous suspension>
#20     CommandRunner.runCommand (package:args/command_runner.dart:212:13)
<asynchronous suspension>
#21     FlutterCommandRunner.runCommand.<anonymous closure> (package:flutter_tools/src/runner/flutter_command_runner.dart:339:9)
<asynchronous suspension>
#22     AppContext.run.<anonymous closure> (package:flutter_tools/src/base/context.dart:150:19)
<asynchronous suspension>
#23     FlutterCommandRunner.runCommand (package:flutter_tools/src/runner/flutter_command_runner.dart:285:5)
<asynchronous suspension>
#24     run.<anonymous closure>.<anonymous closure> (package:flutter_tools/runner.dart:115:9)
<asynchronous suspension>
#25     AppContext.run.<anonymous closure> (package:flutter_tools/src/base/context.dart:150:19)
<asynchronous suspension>
#26     main (package:flutter_tools/executable.dart:90:3)
<asynchronous suspension>

To make the idea of injecting the asset manifest into the dart code we compile work, we would have to do one of the following:

do compilation twice (once to satisfy icon tree shaking functionality, once after we inject the asset manifest into the code) or
build the asset bundle twice (first time with tree shaking disabled) the generate the asset manifest so that we have it before compilation, or
refactor copyAssets to not actually build an asset bundle, but to accept one built by something else for copying. Then, we could build the asset manifest separately while not having copyAssets redundantly build the asset bundle again (this would probably be a significant refactor).

None of these solutions seem great.

Alternatively, another thing we can do to support the flutter build web case is inject the asset manifest contents into the service worker that is generated as one of the final steps. This is what the current state of this PR does. However, I don't like this solution, because it adds a lot of complexity, and I am not sure it the (lack of) severity of the parent issue justifies it.

Yet another (already mentioned) alternative is to generate another version of the asset manifest that isn't a .bin file and have flutter web apps instead request that. This solves the parent issue, but doesn't solve the issue of all asset loads depending on the request for the asset manifest succeeding (imo this is probably rare for production apps, especially once we fix #130455 since that basically will give browsers two opportunities to download the asset manifest).

At this point I am considering closing this PR since my work on this has far over-exceeded the amount of time I allotted myself to work on this, but I will discuss with @jonahwilliams before then.

andrewkolos · 2023-07-19T08:06:21Z

Another option I've just now realized is that I could just try the service worker thing (I still need to test this with IDM). Developers with IDM will still run into the issue when they flutter run -d chrome, but 1) it's very reasonable to ask/expect them to configure their IDM to stop hijacking requests and 2) still solves the issues for end-users without too much additional code complexity.

jonahwilliams · 2023-07-19T16:32:09Z

Ahh yeah, the asset bundle depending on the results of compilation for tree shaking is unfortunate. I think building the bundle twice is probably the most reasonable way to do it. By default I don't think copy assets (the helper function) will write any data, so you could do that in the same target that generates the entrypoint

andrewkolos · 2023-07-26T16:24:21Z

Marking this as ready for review. I wanted to get Google testing unstuck first, but I had no success.

edit: oops, forgot to replace the WIP text in the description. Will do that and then request review from the web team.

jonahwilliams · 2023-07-26T16:27:18Z

packages/flutter/lib/src/painting/_load_asset_manifest_io.dart

This comment seems out of date

Addressed in commit fix documented [sic.] of loadAssetManifest in _load_asset_manifest_io.dart

packages/flutter/lib/src/painting/_load_asset_manifest_web.dart

packages/flutter_tools/lib/src/build_system/targets/web.dart

jonahwilliams · 2023-07-26T16:30:23Z

packages/flutter_tools/lib/src/build_system/targets/web.dart

nit: remove?

Do you mean all four of these lines? Don't we need to call build to generate the asset manifest?

deferredComponentsEnabled

Oh yeah, I think we can remove this since deferred components assets are written to the asset manifest the same way any other asset is. Addressed with commit simplify call to assetBundle.build

packages/flutter_tools/lib/src/build_system/targets/web.dart

…y point

This reverts commit e45151778e87bdf0cef178d62eb8057c3998f4d9.

… instead

* Move load_asset_manifest to services * Restore image_resolution.dart * Conditionally loadAssetManifest from bundle. * Simplify js-interop, and avoid having two copies of the manifest in memory. * Prevent web version from calling AssetManifest.loadFromAssetBundle(bundle); (It's a recursive call now)

christopherfujino · 2023-08-10T20:36:45Z

@andrewkolos has this PR been superceded by #131382 ?

github-actions bot added tool Affects the "flutter" command-line tool. See also t: labels. framework flutter/packages/flutter repository. See also f: labels. labels Jul 12, 2023

andrewkolos added a: assets Packaging, accessing, or using assets fyi-web For the attention of Web platform team labels Jul 12, 2023

jonahwilliams reviewed Jul 13, 2023

View reviewed changes

packages/flutter_tools/lib/src/web/file_generators/main_dart.dart Outdated Show resolved Hide resolved

jonahwilliams reviewed Jul 13, 2023

View reviewed changes

packages/flutter/lib/src/services/_asset_manifest_contents_io.dart Outdated Show resolved Hide resolved

github-actions bot removed the framework flutter/packages/flutter repository. See also f: labels. label Jul 17, 2023

andrewkolos changed the title ~~[web] Store asset manifest in JS global to avoid depending on an HTTP request when loading assets~~ [web] Make asset manifest immediately available to app (no network request) Jul 17, 2023

github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Jul 18, 2023

andrewkolos force-pushed the fix-asset-manifest-web branch 2 times, most recently from 33aa21d to db5bbc3 Compare July 18, 2023 21:47

andrewkolos force-pushed the fix-asset-manifest-web branch 4 times, most recently from 13b655b to 163d23f Compare July 24, 2023 22:04

andrewkolos marked this pull request as ready for review July 26, 2023 16:23

andrewkolos requested a review from jonahwilliams July 26, 2023 16:23

jonahwilliams reviewed Jul 26, 2023

View reviewed changes

packages/flutter/lib/src/painting/_load_asset_manifest_web.dart Outdated Show resolved Hide resolved

jonahwilliams reviewed Jul 26, 2023

View reviewed changes

packages/flutter_tools/lib/src/build_system/targets/web.dart Outdated Show resolved Hide resolved

jonahwilliams reviewed Jul 26, 2023

View reviewed changes

packages/flutter_tools/lib/src/build_system/targets/web.dart Outdated Show resolved Hide resolved

jonahwilliams reviewed Jul 26, 2023

View reviewed changes

packages/flutter_tools/lib/src/build_system/targets/web.dart Outdated Show resolved Hide resolved

andrewkolos and others added 25 commits August 1, 2023 12:47

(broken) try building asset bundle before generating & compiling entr…

2b972c6

…y point

revert everything

084e06a

working (hopefully) implementation utilizing service worker code gen

194a1c8

use asset manifest from output dir instead of build dir

bce44b3

Revert "use asset manifest from output dir instead of build dir"

8ed31cd

This reverts commit e45151778e87bdf0cef178d62eb8057c3998f4d9.

declare asset manifest as input to WebServiceWorker

8bfb6e2

(experiment) fix tests

414e478

restore avoiding request when running from resident runner

c1e2cbd

unify signatures of assetManifestContents

e9dec1c

avoid base64 encoding

f790787

fix variable name

9338531

undo service worker stuff, build asset bundle twice instead

6015225

reduce global spaghetti

b3882b9

remove added build target, have entry point target build asset bundle…

1cf8110

… instead

attempt to fix google testing

b135879

move from 'js' to 'js_interop' to satisfy conditional_srcs_processor

33a0bff

nonchange to unstuck google testing

a2bf2ac

fix documented of loadAssetManifest in _load_asset_manifest_io.dart

46fd0f1

+readability to construction of assetManifestByteData

68253a4

remove outdated addition to outputs of WebReleaseBundle

547eec0

simplify call to assetBundle.build

e33f3c0

remove extra input

e79d2c0

lints

5cf0260

fix cyclic dependency

d7d61e3

andrewkolos force-pushed the fix-asset-manifest-web branch from 404a406 to d7d61e3 Compare August 1, 2023 19:47

fix infinite recursion

07fe469

andrewkolos closed this Aug 14, 2023

andrewkolos deleted the fix-asset-manifest-web branch April 29, 2024 18:17

[web] Make asset manifest immediately available to app (no network request) #130419

[web] Make asset manifest immediately available to app (no network request) #130419

Uh oh!

Conversation

andrewkolos commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Fix

Pre-launch Checklist

Uh oh!

Uh oh!

Uh oh!

andrewkolos commented Jul 13, 2023

Uh oh!

jonahwilliams commented Jul 13, 2023

Uh oh!

andrewkolos commented Jul 13, 2023

Uh oh!

andrewkolos commented Jul 18, 2023

Uh oh!

andrewkolos commented Jul 19, 2023

Uh oh!

jonahwilliams commented Jul 19, 2023

Uh oh!

andrewkolos commented Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams Jul 26, 2023

Choose a reason for hiding this comment

Uh oh!

andrewkolos Jul 26, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonahwilliams Jul 26, 2023

Choose a reason for hiding this comment

Uh oh!

andrewkolos Jul 26, 2023

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Jul 26, 2023

Choose a reason for hiding this comment

Uh oh!

andrewkolos Jul 26, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

christopherfujino commented Aug 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

andrewkolos commented Jul 12, 2023 •

edited

Loading

andrewkolos commented Jul 26, 2023 •

edited

Loading