microsoft
Prefer passing specific services to constructor instead of `IServiceProvider`
We have multiple places where we pass IServiceProvider to constructors. Usually, it's better to make the dependencies very explicit (helps with testability and code clarity as to what exactly a specific component needs).
There might be cases where the service isn't available yet during the constructor, and in these cases we should clearly document that the service is intentionally retrieved later via IServiceProvider because it's not available yet in the constructor.
Today, at least the following classes have IServiceProvider in constrcutor. I didn't validate yet which is valid usage, if any, and which needs to be updated).
- RetryExecutionFilterFactory
- HotReloadTestHostTestFrameworkInvoker
- RetryFailedTestsPipeServer
- RetryLifecycleCallbacks
- RetryDataConsumer
- MSBuildConsumer
- RetryOrchestrator
I remember I've looked into this before, when designing the platform, because I had the same objection. The problems here are that we
- don't have any IoC container that would populate the dependencies to the constructors, nor can we have it in AoT, unless we write a clone of StrongInject or some similar source generated IoC container. This forces very awkward manual passing of dependencies to child dependencies, and cannot be done fully because there are places that have their own factories, which will still resolve the dependencies from the service provider (unless we want to flow all possible dependencies via constructors).
- we don't have AutoFixture in tests, so any change to the constructor will likely require many unit tests to change.
No flame, but "always" based on what? I would also prefer explicit dependencies, but it felt really annoying to flow IClock via 10 constructors only to consume it in child dependency. It also goes against the idea of inversion of control, because the classes that are above need to be intimately aware of the dependencies that all it's children have to be able to construct them. Unlike when DI container is used, where only the container is aware of all the possible dependencies, and classes higher up in the construction have no idea about their dependencies construction.
Any learnings from #7074 ? In case someone tries to implement this in other PR.