tool: build both Wasm and JS targets for flutter run --wasm#186237
tool: build both Wasm and JS targets for flutter run --wasm#186237shivanshu877 wants to merge 2 commits into
flutter run --wasm#186237Conversation
Fixes flutter#172006. `flutter build web --wasm` already produces both a Wasm and a JS build of the app so the runtime web loader can fall back to JS at runtime on browsers that don't support WasmGC. `flutter run --wasm` only produces the Wasm build, so attempting to run on a non-WasmGC browser (e.g. iOS Safari) fails with: FlutterLoader could not find a build compatible with configuration and environment. Bring the run path in line with build by returning a list of compiler configurations (Wasm + JS when --wasm is set, JS-only otherwise) from ResidentWebRunner._compilerConfigs and passing it through to WebBuilder.buildWeb at both call sites.
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
github-actions
Bot
added
the
tool
![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 modifies ResidentWebRunner to return multiple compiler configurations, enabling a JavaScript fallback when Wasm is enabled for web builds. Feedback recommends refactoring the _compilerConfigs getter to avoid duplicating the JsCompilerConfig instantiation.
| List<WebCompilerConfig> get _compilerConfigs { | ||
| if (debuggingOptions.webUseWasm) { | ||
| return WasmCompilerConfig( | ||
| optimizationLevel: 0, | ||
| stripWasm: false, | ||
| renderer: debuggingOptions.webRenderer, | ||
| ); | ||
| return <WebCompilerConfig>[ | ||
| WasmCompilerConfig( | ||
| optimizationLevel: 0, | ||
| stripWasm: false, | ||
| renderer: debuggingOptions.webRenderer, | ||
| ), | ||
| // JS fallback for browsers that don't support WasmGC. | ||
| JsCompilerConfig.run( | ||
| nativeNullAssertions: debuggingOptions.nativeNullAssertions, | ||
| renderer: debuggingOptions.webRenderer, | ||
| ), | ||
| ]; | ||
| } | ||
| return JsCompilerConfig.run( | ||
| nativeNullAssertions: debuggingOptions.nativeNullAssertions, | ||
| renderer: debuggingOptions.webRenderer, | ||
| ); | ||
| return <WebCompilerConfig>[ | ||
| JsCompilerConfig.run( | ||
| nativeNullAssertions: debuggingOptions.nativeNullAssertions, | ||
| renderer: debuggingOptions.webRenderer, | ||
| ), | ||
| ]; | ||
| } |
There was a problem hiding this comment.
The instantiation of JsCompilerConfig.run is duplicated in both branches of the if statement. Refactoring this to a local variable improves maintainability and ensures consistency between the Wasm fallback and the standalone JS build, adhering to the repository's philosophy of optimizing for readability and avoiding unnecessary duplication.
List<WebCompilerConfig> get _compilerConfigs {
final JsCompilerConfig jsConfig = JsCompilerConfig.run(
nativeNullAssertions: debuggingOptions.nativeNullAssertions,
renderer: debuggingOptions.webRenderer,
);
if (debuggingOptions.webUseWasm) {
return <WebCompilerConfig>[
WasmCompilerConfig(
optimizationLevel: 0,
stripWasm: false,
renderer: debuggingOptions.webRenderer,
),
// JS fallback for browsers that don't support WasmGC.
jsConfig,
];
}
return <WebCompilerConfig>[jsConfig];
}References
- Optimize for readability and avoid unnecessary duplication. (link)
Eliminates the duplicated JsCompilerConfig.run() constructor call in _compilerConfigs by hoisting it into a local variable shared by both the Wasm+JS and JS-only return paths. Addresses gemini-code-assist review on flutter#186237
Per @stuartmorgan-g's feedback, this PR makes a behavioral change (returning a list of compiler configs instead of a single one when --wasm is passed to `flutter run`) and was missing the corresponding tests. Adds: * @VisibleForTesting accessor `debugCompilerConfigs` on ResidentWebRunner that exposes the otherwise-private getter. * Two regression tests: - `--wasm produces both Wasm and JS compiler configs for runtime fallback` — asserts that with `webUseWasm: true` the runner emits exactly two configs, one per CompileTarget. - `no --wasm produces only a JS compiler config` — asserts the default path still emits a single JsCompilerConfig.
50008cc to
72b9654
Compare
|
Closing for a clean conversation timeline. Replaced by #186253 with identical content + the new regression tests (only your authored commits). Sorry for the noise. |
1 participant
Description
Fixes #172006.
flutter build web --wasmalready produces both a Wasm and a JS build of the app, so the Flutter web loader can fall back to JS at runtime on browsers that don't support WasmGC.flutter run --wasmonly produces the Wasm build, so attempting to run on a non-WasmGC browser (e.g. iOS Safari connected via remote debugging) fails with:This PR brings the
flutter run --wasmpath in line withflutter build web --wasmby returning a list of compiler configurations (Wasm + JS when--wasmis set, JS-only otherwise) fromResidentWebRunner._compilerConfigsand passing it through toWebBuilder.buildWebat both invocation sites.Repro (from the original issue)
Pre-launch Checklist
resident_web_runner_test.dart).