More WebHostBuilderContext overloads #1020
Conversation
| @@ -75,6 +91,6 @@ public interface IWebHostBuilder | |||
| /// </summary> | |||
| /// <param name="configureDelegate">The delegate for configuring the <see cref="IConfigurationBuilder" /> that will be used to construct an <see cref="IConfiguration" />.</param> | |||
| /// <returns>The <see cref="IWebHostBuilder"/>.</returns> | |||
| IWebHostBuilder ConfigureConfiguration(Action<WebHostBuilderContext, IConfigurationBuilder> configureDelegate); | |||
| IWebHostBuilder ConfigureAppConfiguration(Action<WebHostBuilderContext, IConfigurationBuilder> configureDelegate); | |||
There was a problem hiding this comment.
@DamianEdwards What do you think about this method? We wanted to keep the Configure part since it matches all the existing methods and is nice in intellisense, and using something other than Configuration as the description part seemed wrong. So breaking it up with a word so that it doesn't seem so circular was our best suggestion so far.
| @@ -33,6 +42,13 @@ public interface IWebHostBuilder | |||
| IWebHostBuilder ConfigureServices(Action<IServiceCollection> configureServices); | |||
There was a problem hiding this comment.
We should mark this as obsolete, same with ConfigureLogging(Action<ILoggerFactory> configureLogging)
There was a problem hiding this comment.
Why? There's nothing wrong with this overload.
There was a problem hiding this comment.
Yeah, I don't think we need to obsolete ConfigureServices, we aren't changing the IServiceCollection interface like ILoggerFactory. Most extension methods on IServiceCollection don't require anything from the Context today, so no reason to force everyone to accept the context that they don't need. You could make an argument for moving it to an extension method instead of being on the main WebHostBuilder though, if it isn't already.
There was a problem hiding this comment.
Yea let's at least move it to the extension.
There was a problem hiding this comment.
Unnecessary breaking change, just leave it.
| @@ -33,6 +42,13 @@ public interface IWebHostBuilder | |||
| IWebHostBuilder ConfigureServices(Action<IServiceCollection> configureServices); | |||
There was a problem hiding this comment.
Why? There's nothing wrong with this overload.
|
|
||
| private IConfiguration _config; | ||
| private WebHostOptions _options; | ||
| private bool _webHostBuilt; | ||
| private Func<WebHostBuilderContext, ILoggerFactory> _createLoggerFactoryDelegate; | ||
| private List<Action<WebHostBuilderContext, IConfigurationBuilder>> _configureConfigurationBuilderDelegates; | ||
| private WebHostBuilderContext _hostingContext; | ||
|
|
||
| public WebHostBuilderContext Context { get { return _hostingContext; } } |
| @@ -241,14 +261,15 @@ private IServiceCollection BuildCommonServices(out AggregateException hostingSta | |||
| var appEnvironment = PlatformServices.Default.Application; | |||
| var contentRootPath = ResolveContentRootPath(_options.ContentRootPath, appEnvironment.ApplicationBasePath); | |||
| var applicationName = _options.ApplicationName ?? appEnvironment.ApplicationName; | |||
| var hostingContext = new WebHostBuilderContext | |||
|
|
|||
| _hostingContext = new WebHostBuilderContext | |||
There was a problem hiding this comment.
_hostingContext should be set in the initial constructor so Context never returns null. You have the initial _config at that point too.
| configure(context, loggerFactory); | ||
| return loggerFactory; | ||
| }); | ||
| hostBuilder.ConfigureLogging(configureLogging); |
There was a problem hiding this comment.
Is this recursive right now?
| /// Some properties of this type will be null whilst it is being built, most noteably the <see cref="ILoggerFactory"/> will | ||
| /// be null inside the <see cref="ConfigureAppConfiguration(Action{WebHostBuilderContext, IConfigurationBuilder})"/> method. | ||
| /// </remarks> | ||
| WebHostBuilderContext Context { get; } |
There was a problem hiding this comment.
@Tratcher @glennc @davidfowl @DamianEdwards
I feel that there is little benefit of having this context on the WebHostBuilder itself since its properties won't be initialized until Build(). We are providing the context in the other methods and extensions now and the WebHostBuilderContext will be resolvable from DI where needed. I plan to remove this, but just wanted to make sure everyone is on the same page first.
| /// <returns>The <see cref="IWebHostBuilder"/>.</returns> | ||
| IWebHostBuilder ConfigureConfiguration(Action<WebHostBuilderContext, IConfigurationBuilder> configureDelegate); | ||
| IWebHostBuilder UseLoggerFactory(Func<WebHostBuilderContext, ILoggerFactory> createLoggerFactory); |
There was a problem hiding this comment.
Pardon but I wanted to sort the members alphabetically so overloads are next to each other.
|
@Tratcher This is ready for a final look. |
JunTaoLuo
force-pushed
the
glennc/allTheContext
branch
from
87589fe to
b26e49b
Compare
|
🆙📅 |
…t and make the context accessible in more locations.
JunTaoLuo
force-pushed
the
glennc/allTheContext
branch
from
79c8a4f to
853b384
Compare
Add Context to WebHostBuilder so that extension methods can get to it, and add some missing overloads.
I renamed
ConfigureConfigurationhere so I can get some more feedback on the name, since everyone seems to hate it but nobody has any better suggestions.