[tool] Migrate BuildAppBundleCommand to modular dependency injection - #192246
Conversation
…endency injection
…via executable.dart
…ldCommand and AndroidGradleBuilder
…oidGradleBuilder in executable.dart
Extract Logger, Terminal, FileSystem, and Platform from ToolContext using object pattern destructuring for diff reduction in build_apk, build_appbundle, and build_aar.
…in Android build commands Ensure AndroidBuilder is required and non-nullable in BuildAarCommand, BuildApkCommand, and BuildAppBundleCommand. Construct effective AndroidGradleBuilder in BuildCommand if not injected. Update tests to remove context.get<AndroidBuilder>() and context.get<Analytics>().
…ate fallback globals Require non-nullable `AndroidBuilder` parameter in `BuildCommand` to eliminate fallback `AndroidGradleBuilder` construction within the command body. Require `Analytics` and `Logger` in `exitWithNoSdkMessage` to remove ambient `globals` fallback references. Simplify test runners across permeable Android build tests.
…separate PRs Narrow scope of PR 14 to `AndroidGradleBuilder` constructor DI, `exitWithNoSdkMessage` refactoring, non-nullable `AndroidBuilder` in `BuildCommand`, and `BuildApkCommand` migration.
Migrate `BuildAppBundleCommand` (`flutter build appbundle`) to modular dependency injection with `ToolContext`, `AndroidContext`, `BuildSystem`, and `AndroidBuilder`, removing direct dependencies on `globals.dart`.
There was a problem hiding this comment.
Code Review
This pull request refactors BuildAppBundleCommand to inject dependencies like AndroidBuilder, AndroidContext, BuildSystem, and ToolContext instead of relying on global variables, and introduces new unit tests. The review feedback suggests refactoring to avoid duplication by moving the toolContext override and the targetFile getter to the base BuildSubCommand class, and recommends accessing _androidContext.androidSdk directly rather than through the testing-annotated getter.
dcharkes
left a comment
There was a problem hiding this comment.
LGTM!
I remember the discussions around globals when I first started contributing and was forced to plumb deps through constructors and function calls instead. But indeed it makes testing much easier. (We do so extensively by mocking the package:hooks_runner as well.)
High level question: Is this just cleanup, or do we have another overarching goal as well?
They've been a thorn in our side for years now, but we've never been able to justify the engineering hours to actually perform the migration work. Agents have made this much easier :) This is "cleanup" in the sense that it's part of a project health initiative to remove this global state, and it should help with some future decoupling work. |
…er#192247) ## Summary Part 14c of the modular dependency injection migration. Migrates `BuildAarCommand` (`flutter build aar`) to modular dependency injection with `ToolContext` and `AndroidContext`, removing direct dependencies on `globals.dart`. Stacked on flutter#192246 Compare: bkonyi/flutter@master...di/14c-build-aar Part of flutter#47161 --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Summary
Part 14b of the modular dependency injection migration.
Migrates
BuildAppBundleCommand(flutter build appbundle) to modular dependency injection withToolContextandAndroidContext, removing direct dependencies onglobals.dart.Stacked on #190776
Compare: bkonyi/flutter@di/14-android-build-and-toolchain...di/14b-build-appbundle
Part of #47161