Replace primitive injection with IConfiguration #887
Conversation
| _environmentInfo = environmentInfo; | ||
| _defaultDirectory = defaultDirectory; | ||
| _defaultDirectory = configuration["ManagedFileSystem:DefaultDirectory"]; | ||
|
|
||
| // On Linux with mono, this needs to be true or symbolic links are ignored |
There was a problem hiding this comment.
thinking out loud, not necessary to implement in this PR:
if this is for mono only, maybe we should drop that?..
| public static readonly Dictionary<string, string> Configuration = new Dictionary<string, string> | ||
| { | ||
| {"ManagedFileSystem:DefaultDirectory", null}, | ||
| {"ManagedFileSystem:EnableSeparateFileAndDirectoryQueries", "True"} |
There was a problem hiding this comment.
Do we need this option at all? Some old comments said it had to be tweaked for Mono on Linux, but we had it hardcoded to true since we are on .net core. I suggest removing this option.
There was a problem hiding this comment.
Both these options are unnecessary actually. Removed them both.
Program.cs and ApplicationHost are both getting really large, the extra class let's us configure options for ServerImplementations services that only devs can toggle and not users while still providing a centralised place for them. With this implementation, if we want to let the user configure a specific option, it's as simple as removing it from the ConfigurationOptions dictionary and adding it to an external configurations file that also get''s added to the IConfiguration.
This will make it easier to move dependency registration to a system without having to new up all the services first. Moved the primitives to an IConfiguration which is much easier to inject.
5a77f15
to
18ae107
Compare
| .Build(); | ||
| return new ConfigurationBuilder() | ||
| .SetBasePath(appPaths.ConfigurationDirectoryPath) | ||
| .AddJsonFile("logging.json") |
There was a problem hiding this comment.
The config is still called logging.json, you can either create a separate config file for the other settings or rename this one.
There was a problem hiding this comment.
We don't have any other user configurable IConfiguration sources at the moment so keeping it as logging.json still makes sense. We can also add multiple JsonFiles so we can extend that builder when we have another config file to add
There was a problem hiding this comment.
Oh right, you're using the InMemoryCollection 👍
Description
Currently, we create all instances of our registered services manually in ApplicationHost. This is very much an anti pattern for Dependency Injection. The correct way is to create the dependency injection container and register services against it with the specified creation policy (singleton, per instance, per scope etc.). This allows the container to manage the lifetime of the objects that it creates and automatically calls Dispose on services that implement IDisposable.
This commit is just a start to removing the primitives from services that are manually created and registered. We need to do this to eventually get to a point where the container can be responsible for the creation of these services.
I'm following the guidance from the dot net core developer reference -> https://docs.microsoft.com/en-us/aspnet/core/fundamentals/configuration/?view=aspnetcore-2.1