Repository navigation
feat(web): support --web-content-hash for physical static assets - #191919
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the --web-content-hash flag to Flutter Web builds, enabling compiled entrypoints and assets to include content hashes in their filenames to prevent stale browser caching. The changes update compiler configurations, build targets (Dart2JSTarget, Dart2WasmTarget), release bundling, and templating to dynamically handle hashed filenames and update manifests. Feedback on the changes highlights a duplicate import of dart:typed_data in web.dart and identifies two critical issues in the asset hashing implementation (_hashWebAssets): double-hashing/file accumulation during incremental builds and a missing update to AssetManifest.json.
* Computes SHA-256 for physical assets under assets/ directory * Appends hash to asset filename prior to extension * Rewrites AssetManifest.bin, AssetManifest.bin.json, and FontManifest.json with new file paths * Safely skips caching if --web-content-hash is false * Implements test verifying behavior in web_test.dart Fixes flutter#191915
* Exclude NOTICES and NOTICES.Z from physical renaming to avoid breaking LicensePage * Clean outputDirectory before copying assets to prevent stale asset leaks and Service Worker bloat * Update flutter_assets.d depfile with renamed output paths * Add defensive type guards and sublistView buffer safety to manifest decoders/encoders * Support URI-decoded asset paths for special characters
* Support legacy AssetManifest.json rewriting in _hashWebAssets * Resolve specify_nonobvious_local_variable_types and omit_obvious_local_variable_types in web.dart and web_test.dart * Rebase on updated base branch web-content-hash-phase-1
67fada8 to
7036c34
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces asset hashing for web builds when webContentHash is enabled, renaming assets with their SHA-256 content hash and updating the asset and font manifests accordingly. The review feedback suggests refactoring the hashing process to be asynchronous and streaming file contents to prevent blocking the event loop and avoiding potential Out-Of-Memory (OOM) errors on large assets. Additionally, it recommends preserving unexpected keys in the manifests to prevent data loss.
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
* Decompose relative asset paths using FileSystem.path and join to POSIX manifest paths using p.posix.joinAll per dart-use-path-package. * Exclude all shaders (*.frag and shaders/) from content hashing to prevent runtime 404s in FragmentProgram. * Anchor unhashed asset checks to relative root paths. * Guard against FormatException on unencoded percent signs in asset paths and map bidirectional encodings in renamedAssets. * Preserve unexpected entries and non-standard variant structures in AssetManifest.bin and AssetManifest.json. * Add comprehensive test assertions in web_test.dart covering manifests, DPR variants, shaders, package fonts, and unhashed passthrough.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces web asset hashing when webContentHash is enabled, renaming physical asset files with their content hashes and updating the corresponding manifest files. Comprehensive tests are added to verify this behavior. However, a critical issue was identified where casting elements of compileTargets to Dart2WebTarget in the .any closure will cause a runtime TypeError, and it should instead use WebCompilerConfig.
There was a problem hiding this comment.
Code Review
This pull request implements asset content-hashing for Flutter web builds when the webContentHash configuration is enabled. It renames physical assets with a SHA-256 hash of their content, updates the corresponding asset and font manifests, and updates the build dependency file. Comprehensive tests are added to verify the hashing behavior and manifest updates. Feedback on the changes suggests making the .frag file extension check case-insensitive to ensure shaders with uppercase extensions are also excluded from content-hashing.
- Make .frag extension check case-insensitive in _hashWebAssets to handle uppercase shader extensions. - Remove redundant direct import of dart:convert in web.dart, fixing forbidden_imports_test.dart in CI. - Add test coverage verifying unhashed uppercase .FRAG shader assets.
…paths - Extract Phase 2 asset hashing (hashWebAssets, computeHashedBasename) into lib/src/web/content_hash.dart. - Normalize asset output paths in WebReleaseBundle.build and hashWebAssets for Windows support. - Add unit test assertion verifying AssetManifest.bin.json content rewriting.
…hide --web-content-hash (flutter#193040) ## Description Completes the `--web-content-hash` architecture by adding runtime raw-key asset resolution in `web_ui`, achieving 100% content-hashing coverage across all files in `build/web/assets/` (eliminating manifest and shader exclusions), preserving `AssetManifest.listAssets()` and `AssetMetadata.main`, validating custom `web/flutter_bootstrap.js` / `web/index.html` templates, and un-hiding `--web-content-hash` (`--no-web-content-hash`) in `flutter build web --help`. ### Why this is needed Phase 2 (flutter#191919) and Phase 3 (flutter#192953) renamed physical asset files on disk (`<name>.<hash>.<ext>`), rewrote target values inside `AssetManifest.bin`, `AssetManifest.bin.json`, `AssetManifest.json`, and `FontManifest.json`, and emitted `precache_manifest.json`. Two critical gaps remained before exposing `--web-content-hash` to users: 1. **Cold-cache 404s on raw-key asset requests**: - While `Image.asset` resolves variant paths via `AssetManifest` before calling `rootBundle.load(variant.key)`, direct callers of `rootBundle.load(key)` / `rootBundle.loadString(key)` (`PlatformAssetBundle`) and third-party web plugins (`vector_graphics` / `flutter_svg` `AssetBytesLoader`, `lottie`, `rive`, `video_player_web`, `audioplayers_web`) pass the raw declared key (`assets/data/config.json`) directly to `ui_web.AssetManager.getAssetUrl(String asset)`. - Without runtime key translation in `ui_web.AssetManager.getAssetUrl(asset)`, raw-key asset fetches requested un-hashed paths (`assets/assets/data/config.json`) and 404'd on cold caches. 2. **Un-hashed manifest and shader files under `build/web/assets/`**: - `AssetManifest.*`, `FontManifest.json`, `NOTICES`, `NOTICES.Z`, and `shaders/**` were previously excluded from hashing, which meant a single `"source": "assets/**" -> Cache-Control: public, max-age=31536000, immutable` CDN rule would freeze `AssetManifest.bin.json` and `FontManifest.json` across deployments (`v1 -> v2`). ### What this PR changes 1. **100% `build/web/assets/` Immutability & Clean `AssetManifest.listAssets()` (`packages/flutter_tools/lib/src/web/content_hash.dart`)**: - Removes `_excludedFilenames` and `_excludedDirPrefixes` so every file under `build/web/assets/` (including `shaders/**`, `NOTICES`, `NOTICES.Z`, `FontManifest.json`, `AssetManifest.json`, `AssetManifest.bin`, and `AssetManifest.bin.json`) is content-hashed on disk. - Keeps `AssetManifest.bin` and `AssetManifest.json` strictly limited to user/package manifest entries (storing `Uri.decodeFull` variant paths so `Image.asset` on spaced filenames encodes cleanly to `%2520` rather than triple-encoding to `%252520`), and passes non-manifest SDK files (`NOTICES`, `shaders/ink_sparkle.frag`, `shaders/stretch_effect.frag`, `AssetManifest.bin`, `AssetManifest.json`) via `_flutter.buildConfig.extraAssets` (~200 bytes) alongside `"assetManifest"` and `"fontManifest"`. - Sorts discovered asset paths deterministically before manifest assembly so incremental and clean builds emit identical manifest hashes. - Fails fast (`throwToolExit`) if custom `web/flutter_bootstrap.js` or `web/index.html` templates omit `{{flutter_build_config}}` or contain malformed `_flutter.buildConfig` assignments. 2. **Preserve `AssetMetadata.main` (`packages/flutter/lib/src/services/asset_manifest.dart`)**: - Updates `_AssetManifestBin` (`asset_manifest.dart`) so `AssetMetadata.main` remains `true` for the primary 1.0x asset variant when content hashes are embedded in variant filenames. 3. **Incremental Build `.filecache` Tracking (`packages/flutter_tools/lib/src/build_system/targets/web.dart`)**: - Declares `{OUTPUT_DIR}/*/index.html` and `{OUTPUT_DIR}/flutter_bootstrap.js` in `WebReleaseBundle.inputs` and `WebReleaseBundle.outputs` when `webContentHash` is enabled so `.filecache` records post-injection file hashes. 4. **Synchronous Engine-Private Runtime Key Translation (`web_ui`)**: - Extends `FlutterJS` with `JSBuildConfig` (`assetManifest`, `fontManifest`, `extraAssets`) in `js_loader.dart`. - Updates `fetchFontManifest` in `fonts.dart` to fetch `flutter?.buildConfig?.fontManifest?.toDart ?? 'FontManifest.json'`. - Adds `_loadContentHashedAssetManifest(ui_web.assetManager)` inside `initializeEngineServices()` `Future.wait` in `initialization.dart` to register `assetManifest`, `fontManifest`, `extraAssets`, and decoded `AssetManifest.bin.<hash>.json` mappings in engine-private `setContentHashedAssetMap` / `resolveContentHashedAsset` (keeping the public `dart:ui_web` `AssetManager` API unchanged). - Resolves logical keys and variant paths synchronously inside `ui_web.AssetManager.getAssetUrl(String asset)`. 5. **Un-hide `--web-content-hash` (`packages/flutter_tools/lib/src/web/web_options.dart` & `build_web.dart`)**: - Removes `hide: true` from `WebOptions.webContentHash` and notes that local `canvaskit/**` (`--no-web-resources-cdn`) remains un-hashed (`urlHashed: false` in `precache_manifest.json`). ## Related Issues - Fixes flutter#191915 - Fixes flutter#193031 - Part of flutter#149031 ## Tests - `packages/flutter_tools/test/general.shard/build_system/targets/web_test.dart` - `packages/flutter_tools/test/commands.shard/hermetic/build_web_test.dart` & `args_test.dart` - `packages/flutter/test/services/asset_manifest_test.dart` - `engine/src/flutter/lib/web_ui/test/engine/assets_test.dart` ## Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] All existing and new tests are passing.
Supports content-hashing physical static assets under
assets/and rewrites target path mappings inAssetManifest.bin,AssetManifest.bin.json,AssetManifest.json, andFontManifest.jsonwhen building Flutter Web with--web-content-hash.What this PR accomplishes
build/web/assets/.image.<hash>.png).AssetManifest.bin,AssetManifest.bin.json, andAssetManifest.jsonvariant maps to point to hashed filenames soAssetImagetransparently resolves content-hashed assets at runtime.FontManifest.json.AssetManifest.*,FontManifest.json,NOTICES,NOTICES.Z) and all shaders (*.fragandshaders/) from content-hashing to prevent runtime discovery and CanvasKitFragmentProgram404s.build/web/assets/on incremental rebuilds to prevent stale asset accumulation, and updatesflutter_assets.dfor hermetic build tracking.dart-use-path-packageconventions (fileSystem.pathcontext decomposition andp.posix.joinAllmanifest bridging).Note
Direct runtime resolution for arbitrary
rootBundle.load(...)androotBundle.loadString(...)calls (which pass raw keys to the engine channel without consultingAssetManifest) will be completed in a companion framework task.Part of #191915
Part of #149031