Repository navigation
Reduce devtools extension build size via safe engine-asset pruning - #10063
NemesisDevX wants to merge 8 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
`flutter build web` copies every built web engine artifact into build/web/canvaskit, so the packaged extension/devtools/build output is ~44 MB even though a given build never loads most of those files at runtime (flutter#9897). This change makes build_and_copy prune what is provably or explicitly unneeded from the copied destination (source build output is untouched): - .js.symbols debug files are always removed; the bootstrap loader never fetches them. - skwasm/wimp renderer files are removed only when the build output contains no .wasm application artifact at its root, which proves the wasm renderer cannot be selected independently of any runtime configuration. - A new opt-in --prune-cdn-canvaskit flag removes the entire canvaskit/ directory for extensions whose owners can assert the default CDN configuration applies (no custom bootstrap, index.html initialization or runtime config overrides). It is rejected when combined with --no-web-resources-cdn. The default remains conservative: whether a build loads CanvasKit from the CDN depends on mutable runtime configuration that cannot be proven statically, so full pruning requires explicit opt-in. Measured on the example standalone extension (flutter build web): default CDN build 44 -> 23 MB (or 7 MB with --prune-cdn-canvaskit), offline 23 MB, --tree-shake-icons 21 MB. Assisted-by: Devin AI
0402666 to
8ed122f
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces size-reduction features for DevTools extensions by pruning unused web engine resources during the build process. It adds new flags (--tree-shake-icons, --web-resources-cdn, and --prune-cdn-canvaskit) to the build_and_copy command, implements the pruning logic in _build_and_copy.dart, and includes comprehensive unit tests. Feedback on the changes suggests optimizing the pruneCopiedBuildOutput method to avoid redundant directory listings and file deletions when pruneCdnCanvasKit is enabled.
Assisted-by: GPT-6
Assisted-by: GPT-6
dart format reflows the root-level .wasm artifact check introduced in 0360371; no behavior change. Assisted-by: Devin AI
|
@kenzieschmoll WDYT of this feature? |
kenzieschmoll
left a comment
kenzieschmoll
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Reducing published extension size is worth doing, and --no-web-resources-cdn is useful on its own so extensions built with the default template can load CanvasKit offline. Pruning only the copied destination and keeping the full canvaskit/ prune opt-in are the right defaults.
Before landing, we need to fix the reasoning for the skwasm prune, remove several no-op tests, and document the new flags in the README. Details inline.
| /// * `skwasm`/`wimp` renderer files are only reachable when the output | ||
| /// contains a `.wasm` application artifact at the build root, which is | ||
| /// emitted only for `dart2wasm` builds (as `main.dart.wasm`, possibly | ||
| /// content-hashed). Its absence proves — independently of any mutable | ||
| /// bootstrap configuration — that the wasm renderer cannot be | ||
| /// selected, so those variants are removed. |
There was a problem hiding this comment.
[CONCERN] The absence of a root-level .wasm file does not prove the skwasm renderer isn't used. flutter build web supports --dart-define=FLUTTER_WEB_USE_SKIA=false --dart-define=FLUTTER_WEB_USE_SKWASM=true for dart2js builds (see WebRendererMode.fromDartDefines in flutter_tools), which emits "compileTarget":"dart2js","renderer":"skwasm" without main.dart.wasm. The flutter.js loader picks loadCanvasKit or loadSkwasm based on build.renderer, not whether a .wasm file exists.
Pruning is only safe today because build_and_copy runs flutter build web --release without --wasm or dart-defines, so the output is always dart2js + canvaskit.
To support building extensions with wasm in the future, please check the renderer values in _flutter.buildConfig (emitted into flutter_bootstrap.js): only remove the skwasm/wimp files when no build uses the skwasm renderer, and keep everything if the config cannot be read. Please also add a test for a build config containing both dart2wasm + skwasm and dart2js + canvaskit builds (as emitted by flutter build web --wasm), and update the doc comment, CHANGELOG entry, and PR description to match.
There was a problem hiding this comment.
Fixed in d3e06ea. The .wasm-artifact heuristic is removed: pruneCopiedBuildOutput now statically inspects every _flutter.buildConfig = {...} literal in flutter_bootstrap.js and index.html and removes the skwasm*/wimp* variants only when no builds entry declares "renderer": "skwasm". Missing, malformed, dynamically-assigned, or builds-mutating configurations all fail closed and keep the variants (plain loader reads of the config are ignored).
Follow-ups in 4d227b9, 5584801, and 9ace743: adversarial audits found the inspection still failed open when the config object was mutated through an alias (const config = _flutter.buildConfig; config.builds.push(...)), Object.assign/destructuring/computed keys/for...of entry writes, value-position escapes (call args, spread, ??, ternary), explicit _flutter.loader.load({config: ...}) overrides, _flutter[...]/global[...] computed access, quoted builds/buildConfig/renderer key strings used as variables, .renderer writes including prototype pollution, and non-string or case-variant renderer values. All of these now fail closed. Notably, DevTools' own build/web/flutter_bootstrap.js appends custom bootstrap code that selects the renderer at runtime via _flutter.loader.load({config: rendererConfig}) — the inspection now keeps engine variants for exactly this pattern while stock flutter build web output (whose load() call only passes serviceWorkerSettings) still prunes.
New tests cover: dart2js+CanvasKit (prunes, real emitted config incl. the {} dry-run entry and skwasm entries in wasmHashes), dart2js+Skwasm via dart-defines with no root .wasm artifact (keeps — your exact case), the flutter build web --wasm mixed dart2wasm/skwasm + dart2js/canvaskit config (keeps), missing/malformed/dynamic/reassigned/mutated configs (keep), every mutation/escape/override vector above (keep), index.html-inlined configs, read-only loader controls (let i = _flutter.buildConfig + for...of i.builds, === comparison), and union behavior when the two files disagree. Verified end-to-end on fresh flutter build web output: 43 MB -> 23 MB default, 43 MB -> 7 MB with --prune-local-canvaskit, SHA-256 file-tree comparison shows only intended removals. Doc comment, CHANGELOG, and PR description updated to match.
| group('retains assets for configuration that cannot be proven safe', () { | ||
| for (final (description, contents) in [ | ||
| ( | ||
| 'a config object reassigned after the emitted statement', | ||
| '_flutter.buildConfig = {"engineRevision":"abc123","builds":[]};\n' | ||
| '_flutter.buildConfig = runtimeOffline;\n' | ||
| '_flutter.loader.load({});\n', | ||
| ), | ||
| ( | ||
| 'a mutated build configuration', | ||
| '_flutter.buildConfig = {"engineRevision":"abc123","builds":[]};\n' | ||
| '_flutter.buildConfig.useLocalCanvasKit = true;\n' | ||
| '_flutter.loader.load({});\n', | ||
| ), | ||
| ( | ||
| 'an escaped configuration option key', | ||
| '_flutter.buildConfig = {"engineRevision":"abc123","builds":[]};\n' | ||
| r'_flutter.loader.load({conf\u0069g: {canvasKitBaseUrl: "./"}});' | ||
| '\n', | ||
| ), | ||
| ( | ||
| 'a getter configuration option', | ||
| '_flutter.buildConfig = {"engineRevision":"abc123","builds":[]};\n' | ||
| '_flutter.loader.load({' | ||
| 'get config() { return window.runtimeOptions; }});\n', | ||
| ), | ||
| ( | ||
| 'a dynamically appended wasm build entry', | ||
| '_flutter.buildConfig = {"engineRevision":"abc123","builds":' | ||
| '[{"compileTarget":"dart2js","renderer":"canvaskit"}]};\n' | ||
| '_flutter.buildConfig.builds.push(' | ||
| '{"compileTarget":"dart2wasm"});\n' | ||
| '_flutter.loader.load({});\n', | ||
| ), | ||
| ]) { | ||
| test(description, () { | ||
| File( | ||
| p.join(buildDir.path, 'flutter_bootstrap.js'), | ||
| ).writeAsStringSync(contents); | ||
| // A wasm artifact exists, so the wasm renderer is reachable. | ||
| File( | ||
| p.join(buildDir.path, 'main.dart.wasm'), | ||
| ).writeAsStringSync('// wasm'); | ||
|
|
||
| BuildExtensionCommand.pruneCopiedBuildOutput( | ||
| buildDir.path, | ||
| pruneCdnCanvasKit: false, | ||
| ); | ||
|
|
||
| expectSymbolsRemoved(); | ||
| expectRuntimeAssetsKept(); | ||
| expect( | ||
| File( | ||
| p.join(buildDir.path, 'canvaskit', 'skwasm.wasm'), | ||
| ).existsSync(), | ||
| isTrue, | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| test('a canvasKitBaseUrl in index.html initialization', () { | ||
| File(p.join(buildDir.path, 'index.html')).writeAsStringSync( | ||
| '<script>_flutter.loader.load({config: {' | ||
| 'canvasKitBaseUrl: "canvaskit/"}});</script>', | ||
| ); | ||
|
|
||
| BuildExtensionCommand.pruneCopiedBuildOutput( | ||
| buildDir.path, | ||
| pruneCdnCanvasKit: false, | ||
| ); | ||
|
|
||
| expectRuntimeAssetsKept(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[CONCERN] pruneCopiedBuildOutput never reads flutter_bootstrap.js or index.html, so varying their contents has no effect. The five bootstrap tests only pass because they create main.dart.wasm (already covered by default keeps wasm renderer when main.dart.wasm exists), and the index.html test duplicates default keeps all runtime CanvasKit variants.
Please remove this group so it doesn't imply the implementation inspects the bootstrap config.
There was a problem hiding this comment.
Removed. Those fixtures varied flutter_bootstrap.js/index.html contents that the implementation never read, so they proved nothing. They are replaced by cases that drive the new _flutter.buildConfig inspection and assert on the resulting file tree — e.g. a dart2js+skwasm config must keep skwasm.wasm, a dart2js+canvaskit config must remove it — so each test fails if its production behavior is broken.
| test('works without a bootstrap file', () { | ||
| // The pruning decision never depends on flutter_bootstrap.js. | ||
| BuildExtensionCommand.pruneCopiedBuildOutput( | ||
| buildDir.path, | ||
| pruneCdnCanvasKit: false, | ||
| ); | ||
|
|
||
| expectSymbolsRemoved(); | ||
| expectWasmRendererRemoved(); | ||
| expectRuntimeAssetsKept(); | ||
| }); |
There was a problem hiding this comment.
[NIT] The fake build output never includes a bootstrap file, making this identical to default keeps all runtime CanvasKit variants. Please remove it.
There was a problem hiding this comment.
Replaced with a meaningful fail-closed case: with no _flutter.buildConfig present in either flutter_bootstrap.js or index.html, the skwasm/wimp variants are kept — which is the required behavior when the configuration cannot be read.
| ..addFlag( | ||
| _pruneCdnCanvasKitKey, | ||
| help: | ||
| 'Delete the bundled canvaskit/ directory from the copied ' | ||
| 'extension build output. Only use this when the app is known to ' | ||
| 'load all web engine resources from the CDN at runtime: the ' | ||
| 'default `flutter build web` configuration with no custom ' | ||
| 'bootstrap, no custom index.html initialization, and no runtime ' | ||
| 'configuration that could select local resources. Cannot be ' | ||
| 'combined with --no-web-resources-cdn.', | ||
| ); |
There was a problem hiding this comment.
[CONCERN] Please document the three new flags under "Publish your package with a DevTools extension" in packages/devtools_extensions/README.md, including when --prune-cdn-canvaskit is safe to use (not just in --help).
[NIT] This name sounds like it prunes CDN assets. Consider --prune-local-canvaskit.
There was a problem hiding this comment.
Done in d3e06ea. Renamed to --prune-local-canvaskit (it removes local assets, not CDN ones) and documented --tree-shake-icons, --web-resources-cdn/--no-web-resources-cdn, and the pruning flag under "Publish your package with a DevTools extension", including when full local CanvasKit removal is safe (default flutter build web CDN configuration, no custom bootstrap/index.html init or runtime overrides) versus when it is not.
| /// Callers must ensure the app uses the default `flutter build web` | ||
| /// CDN configuration with no custom bootstrap, `index.html` | ||
| /// initialization, or runtime configuration overrides. | ||
| static void pruneCopiedBuildOutput( |
There was a problem hiding this comment.
[NIT] Please add @visibleForTesting since this is only public for testing, and consider extracting named constants for 'canvaskit', '.js.symbols', 'skwasm', and 'wimp'.
There was a problem hiding this comment.
Added @visibleForTesting to pruneCopiedBuildOutput and extracted _canvaskitDirName, _jsSymbolsSuffix, _skwasmAssetPrefix, _wimpAssetPrefix, and _skwasmRendererName.
| static void _log(String message) => | ||
| stdout.writeln('[build_and_copy] $message'); |
There was a problem hiding this comment.
[NIT] '[build_and_copy]' duplicates name. Please share a constant for both.
There was a problem hiding this comment.
Extracted _commandName ('build_and_copy'), now shared by the name getter and the [build_and_copy] log prefix.
| @@ -0,0 +1,334 @@ | |||
| // Copyright 2025 The Flutter Authors | |||
There was a problem hiding this comment.
[NIT] Use 2026 for the copyright year in new files.
There was a problem hiding this comment.
Updated the new test file's copyright to 2026.
| * Reduce extension build output size by pruning web engine resources that are | ||
| never requested at runtime. `.js.symbols` debug files are always removed, | ||
| and `skwasm`/`wimp` renderer files are removed when the build output | ||
| contains no root-level `.wasm` application artifact (so the wasm | ||
| renderer is not selected). A new opt-in `--prune-cdn-canvaskit` flag | ||
| additionally removes the entire local `canvaskit/` directory (~36 MB) for | ||
| builds that are known to load all engine resources from the CDN at | ||
| runtime; it cannot be combined with `--no-web-resources-cdn`. | ||
| * Add a `--tree-shake-icons` flag to the `build_and_copy` command that allows | ||
| the compiler to tree-shake icon fonts (disabled by default for backwards | ||
| compatibility). Passing it can shrink `MaterialIcons-Regular.otf` by ~99%. | ||
| * Add a `--web-resources-cdn` / `--no-web-resources-cdn` flag to the | ||
| `build_and_copy` command for building extensions that do not load CanvasKit | ||
| from the CDN (e.g. for offline use). |
There was a problem hiding this comment.
[NIT] Please format entries as single unwrapped lines to match the rest of the file, and move the detailed reasoning from the first entry to the doc comment.
There was a problem hiding this comment.
Changelog entries are now single unwrapped lines matching the file's style, and the detailed reasoning moved into the pruneCopiedBuildOutput doc comment.
The previous heuristic treated the absence of a root-level .wasm artifact as proof that the skwasm renderer is unreachable, but dart2js builds can select Skwasm through FLUTTER_WEB_USE_SKIA/SKWASM dart-defines without emitting a .wasm artifact. Parse the renderer values of every _flutter.buildConfig literal emitted into flutter_bootstrap.js and index.html, and only remove the skwasm*/wimp* variants when no build declares the skwasm renderer. The variants are kept whenever the configuration is missing, malformed, dynamic, or mutates builds, since the loader's selection cannot be proven statically in those cases. Also rename --prune-cdn-canvaskit to --prune-local-canvaskit (it removes local assets, not CDN ones), document the build_and_copy flags in the README, replace the bootstrap tests that varied unread file contents with cases that exercise the new inspection, and apply review cleanups (visibleForTesting, shared constants, changelog style). Assisted-by: Devin AI
889a780 to
d3e06ea
Compare
The buildConfig inspection only checked each _flutter.buildConfig occurrence in isolation, so mutations through an alias (const config = _flutter.buildConfig; config.builds.push(...)), destructuring, Object.assign/Reflect calls, computed ['builds'] access, or for-of entry writes escaped detection and could prune skwasm/wimp assets that a mutated configuration still needs. Scan each inspected file for builds-mutation syntax after blanking the parsed literals: every .builds access must be a read-only for-of iteration whose body does not write to the loop variable, and computed keys, builds/buildConfig object keys, destructuring, Object.*/Reflect.* mutations, dynamic execution, and computed config access all fail closed. The token now also matches optional-chained, whitespace-separated, and globalThis/self-style prefixed forms, and a comma after a non-assignment reference is treated as an escape. Verified against a real flutter_bootstrap.js: the loader's let i = _flutter.buildConfig alias and its read-only for (const u of i.builds) iteration still allow pruning, so the measured 43 MB -> 23 MB default output is unchanged. Adds nine regression tests covering the alias, Object.assign, destructuring, computed-member, for-of entry-write, defineProperty, and globalThis vectors plus a read-only for-of control. Assisted-by: Devin AI
The escape check only flagged a comma after a non-assignment _flutter.buildConfig reference, but the object leaks the same way through call arguments (f(_flutter.buildConfig)), array/object literals, spread, ternaries, and ?? — any position where the reference itself becomes a value. Widen the pattern to flag `,`, `)`, `]`, `}`, `:`, `&`, `|`, and `?` not followed by `.` (i.e. ?? but not ?.), keeping `.prop`/`?.prop` reads and `;` statement ends as the only safe continuations. Verified against real flutter_bootstrap.js output: the loader's ?.wasmHashes read, `let i = _flutter.buildConfig` alias, string literal mention, and `;`-terminated statements all still pass, so standard CanvasKit builds keep pruning (fresh standalone_extension build/web output: skwasm/wimp removed). Stale example outputs built with an older SDK loader that calls i.builds.find() were already kept before this change and still are — the for-of-only rule is unchanged. Adds four tests: call-arg, spread, ?? escapes kept; === comparison still prunes. Assisted-by: Devin AI
An adversarial audit of the buildConfig inspection found six more
fail-open vectors on real and synthetic inputs:
- Custom bootstrap code appended to flutter_bootstrap.js can call
_flutter.loader.load({config: ...}) with runtime-derived renderer
selection — devtools_app's own build output does exactly this
(shouldUseSkwasm() -> config override). The options object is now
brace-matched and scanned for config/renderer/builds/buildConfig
keys; the stock loader's serviceWorkerSettings-only call still
passes, so standard builds keep pruning.
- _flutter[expr] computed member access bypassed the buildConfig
token entirely; any bracket access on _flutter now fails closed.
- global['_flutter']-style computed global access was not covered;
`global` joins the computed-access pattern.
- Quoted 'builds'/'buildConfig'/'renderer' key strings anywhere in
the file could smuggle watched properties through variables
(const k = 'builds'; config[k].push(...)); they now fail closed.
- .renderer writes (entry.renderer = 'skwasm', including prototype
pollution like Object.prototype.renderer) are now flagged; reads
in the loader's for-of loop are unaffected.
- {renderer: ...} object keys are watched alongside builds/
buildConfig, and builds entries with a present-but-non-string
renderer fail closed; renderer comparison is case-insensitive.
Verified against real flutter_bootstrap.js output: stock builds
(standalone_extension, foo_devtools_extension, devtools_app source
build/web) still prune skwasm/wimp while devtools_app's own custom
bootstrap — which genuinely selects the renderer at runtime — now
correctly keeps them. Adds seven regression tests (six failed before
this change). End-to-end build_and_copy on fresh flutter build web
output: 43 MB -> 23 MB default, 43 MB -> 7 MB with
--prune-local-canvaskit; SHA-256 tree comparison shows only the
intended engine-variant removals and zero content changes.
Assisted-by: Devin AI
Fixes #9897
flutter build webcopies every built web engine artifact intobuild/web/canvaskit, so the packagedextension/devtools/buildoutput is ~44 MB even though a given extension never loads most of those files at runtime.This PR makes
build_and_copyprune what is provably or explicitly unneeded from the copied destination only (the sourcebuild/weboutput is untouched):.js.symbolsdebug files are always removed — the bootstrap loader never fetches them.skwasm*/wimp*renderer variants are removed only when every_flutter.buildConfigliteral found influtter_bootstrap.jsorindex.htmlprovably contains nobuildsentry with"renderer": "skwasm". The loader selects engine resources from that configuration, and Skwasm can be selected bydart2wasmbuilds (flutter build web --wasm) and bydart2jsbuilds that opt in via--dart-define=FLUTTER_WEB_USE_SKIA=false --dart-define=FLUTTER_WEB_USE_SKWASM=true— neither requires a root-level.wasmartifact. When no readable configuration exists, or any discovered configuration is malformed, dynamically assigned, or could be mutated at runtime — through reassignment,buildsaccess, aliases, destructuring,Object.assign/Reflect, computed keys,for...ofentry writes, or dynamic execution — the variants are kept (fail closed). The proof covers only the two inspected files.--prune-local-canvaskitflag removes the entirecanvaskit/directory for extensions whose owners can assert the default CDN configuration applies (no custom bootstrap,index.htmlinitialization, or runtime configuration overrides). It is rejected when combined with--no-web-resources-cdn.Full pruning is deliberately opt-in: whether a build loads CanvasKit from the CDN depends on mutable runtime configuration (bootstrap reassignment/mutation,
load({config: ...}),index.htmlinitialization) that cannot be proven statically, so the default stays conservative in every mode. Theskwasm/wimpcheck is narrower: those files are only fetched when abuildsentry selects theskwasmrenderer, which the emitted configuration declares statically.Measured on the example
standalone_extension(freshflutter build web, head9ace743):--prune-local-canvaskit--tree-shake-icons--no-web-resources-cdnTesting on head
9ace743:dart testbuild_and_copy 27/27 (config-driven cases: dart2js+CanvasKit prunes; dart2js+Skwasm with no.wasmartifact,--wasmmixed builds, missing/malformed/dynamic/reassigned/mutated configs, alias/Object.assign/destructured/computed/for...of-entry mutations ofbuilds,index.html-inlined configs all keep; plus the rejected flag combination and a read-onlyfor...ofloader control),dart testvalidate,flutter testapi 12/12,dart analyze/dart format/git diff --checkclean,dt presubmitpassed, fresh end-to-endbuild_and_copyruns reproduce 43 MB -> 23 MB default and 43 MB -> 7 MB opt-in with SHA-256-verified file trees (only intended engine variants removed, zero content changes), and realflutter_bootstrap.jsoutputs verify stock builds prune while DevTools' own custom bootstrap correctly keeps with the realflutter_bootstrap.js(verified the loader'slet i = _flutter.buildConfigalias + read-onlyfor...of i.buildsstill allows pruning). Not run: browser-hostedtest/web/suite.Follow-up commits (addressing review feedback)
0360371— avoids the redundant recursive listing and per-file deletions when the opt-in flag is set (one listing to sum sizes, then delete the directory).cf0fe41,2bb30c0— changelog wording anddart formatfix-ups.d3e06ea— replaces the.wasm-artifact heuristic with_flutter.buildConfigrenderer inspection (fail-closed), renames the flag to--prune-local-canvaskit, documents the flags in the README, replaces the no-op bootstrap tests with config-driven cases, and applies review cleanups (@visibleForTesting, shared constants, copyright year, changelog style).4d227b9— extends the inspection to fail closed on aliased, destructured,Object.assign/Reflect, computed-key, andfor...ofentry mutations ofbuilds, and hardens the token match (optional chaining, whitespace,globalThis-style prefixes).5584801— treats every value-position_flutter.buildConfigreference (call argument, array/object literal, spread, ternary,??) as an escape that fails closed;.prop/?.propreads and;statement ends remain the only safe continuations.9ace743— fails closed on_flutter.loader.load({...})calls carryingconfig/renderer/buildsoverrides (DevTools' ownflutter_bootstrap.jsdoes runtime renderer selection this way), any_flutter[...]/global[...]computed access, quoted watched-key strings,.rendererwrites (incl. prototype pollution),{renderer: ...}keys, and non-string/case-variantrenderervalues.Parts of this PR were assisted by AI review tooling; all changes were reviewed and verified before landing.
Pre-launch Checklist
General checklist
///).Issues checklist
contributions-welcomeorgood-first-issuelabel. I understand this means my PR might take longer to be reviewed.Tests checklist
AI-tooling checklist
Feature-change checklist
release-notes-not-requiredlabel or left a comment requesting the label be added.