Repository navigation
[tool] Migrate DevFS to modular dependency injection - #192836
Conversation
bkonyi
added
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 refactors the flutter_tools codebase to improve dependency injection by explicitly passing required services (such as FileSystem, Logger, Artifacts, Platform, and ProcessManager) through constructors instead of relying on global state. This change affects several classes, including FlutterDevice, ResidentRunner, HotRunner, ColdRunner, TestCommand, and FlutterTestRunner. The review comments highlight opportunities to further optimize these changes by passing existing Config instances instead of defaulting to Config.test() and by reducing state duplication when instantiating OperatingSystemUtils.
3399699 to
84396d7
Compare
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. 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. |
| required ProcessManager processManager, | ||
| required Artifacts artifacts, | ||
| required BuildMode buildMode, | ||
| required ToolContext toolContext, |
There was a problem hiding this comment.
For my understanding:
Interesting there's a ton of properties in here that are all set at the same time.
Is it usually the case that none of these change individually?
I suspect the answer is that none of them changes individually because they were globals so they're likely singletons (except for mocks in tests).
There was a problem hiding this comment.
Yeah, these instances were all singletons before or created early on in the process and passed around as parameters. We'll never replace these instances with new ones.
|
Golden file changes are available for triage from new commit, Click here to view. 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. |
flutter-dashboard
Bot
removed
the
autosubmit
Summary
Part 23a of the modular dependency injection migration.
DevFSandDevFSContentto modular dependency injection, removing ambientdevFSConfigandHttpClientFactorycontext lookups.Config?andDevFSConfig?parameters intoDevFSandDevFSFileContent.Fork diff: bkonyi/flutter@master...di/23a-devfs
Part of [tool] Migrate flutter_tools commands to modular dependency injection #188471