Repository navigation
[tool] Migrate Web build subcommands and toolchain to modular dependency injection - #190783
Conversation
github-actions
Bot
added
tool
c7444cf to
8b68044
Compare
8b68044 to
bfbe18c
Compare
github-actions
Bot
removed
a: desktop
![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 refactors several build commands (BuildCommand, BuildBundleCommand, BuildWebCommand) and the WebBuilder class to accept explicit dependencies (such as ToolContext, BuildSystem, and FeatureFlags) instead of relying on global state or context lookups. This allows migrating their respective tests from testUsingContext to testWithoutContext. Feedback suggests typing the terminal parameter, field, and local variable in WebBuilder as Terminal instead of AnsiTerminal to prevent potential runtime TypeErrors when using non-ANSI or mock terminals.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors several build commands, including BuildCommand, BuildBundleCommand, and BuildWebCommand, to explicitly accept ToolContext and FeatureFlags instead of relying on global context. This change allows migrating associated tests from testUsingContext to testWithoutContext. Feedback on the changes suggests avoiding an unnecessary downcast of terminal to AnsiTerminal in build_web.dart to prevent potential runtime errors in tests, and consolidating the duplicated _DefaultFeatureFlags class into a shared location to adhere to the DRY principle.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the build commands (BuildCommand, BuildBundleCommand, and BuildWebCommand) and related utilities to use modular dependency injection via ToolContext and FeatureFlags instead of relying on global variables, enabling hermetic tests to run without context. The review feedback suggests enhancing this refactoring by avoiding hardcoded fallback paths in bundle.dart (using globals.localFileSystem instead) and fully injecting the Config instance into WebBuilder to avoid any fallback to globals.config when resolving the web build directory.
github-actions
Bot
added
engine
…-toolchain # Conflicts: # packages/flutter_tools/lib/src/commands/build.dart # packages/flutter_tools/test/commands.shard/hermetic/build_web_test.dart
… fallback - WebBuilder now has a single constructor with all dependencies required (artifacts, build targets, cache, config, platform, terminal added to the existing parameters). BuildWebCommand builds it from its ToolContext; the non-migrated ResidentWebRunner passes its dependencies explicitly at the call site. - getWebBuildDirectory now requires Config and FileSystem instead of falling back to globals.fs. - Remove the unused test-only webBuilder/buildTargets injection points from BuildWebCommand. - Revert unrelated formatting and renames (web_ui embedding strategy, BuildCommand android builder local, WebBuilder field names, and compile_web_test setup) to reduce the diff.
github-actions
Bot
removed
engine
| fileSystem: globals.fs, | ||
| flutterVersion: globals.flutterVersion, | ||
| analytics: globals.analytics, | ||
| logger: logger, |
There was a problem hiding this comment.
Does WebBuilder need to be migrated to tool context, instead of all the params? Edit: Yes you already have a TODO. sgtm!
flutter-dashboard
Bot
removed
the
autosubmit
…-toolchain # Conflicts: # packages/flutter_tools/test/commands.shard/hermetic/build_web_test.dart
2 participants
Summary
Part 17 of the modular dependency injection migration.
Note
Stacked on #192462 (
di/17a-build-bundle-command), which migratedBuildBundleCommandto modular DI.BuildWebCommand({required BuildSystem buildSystem, required FeatureFlags featureFlags, required ToolContext toolContext, required super.verboseHelp, BuildTargets buildTargets = const BuildTargetsImpl(), WebBuilder? webBuilder})ToolContextdirectly to subcommands, eliminatingcontext.getfallbacks inBuildCommandandWebBuilder.FileSystemintodefaultDepfilePathandgetAssetBuildDirectory.test/commands.shard/hermetic/build_web_test.dartto hermetictestWithoutContext.Part of #188471