-
Notifications
You must be signed in to change notification settings - Fork 10k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Consider IStartup alternative #11050
Comments
Yes!! Lets kill anything that is not convention based startup and have 1 way / signature to configure everything. |
I'm a bit confused. How is removing an explicit interface killing anything not convention based startup? Usually in my experience an interface adds structure/convention, rather than kills it... I also don't understand the comment:
@javiercn said in #11001 (comment) that ConfigureTestServices is also going to be no longer required:
It sounds like in 3.0, ConfigureTestServices becomes obsolete as well. |
The convention for ConfigureServices allows multiple signatures. The convention we prefer is |
I get it - basically, you prefer the fluent building to be internal to the Startup object, and the IStartup interface allows fluent external configuration (via escaping an IServiceProvider), which also makes it harder to make consistent mock-able POCO. |
We can break the |
@davidfowl I like this approach better, as it makes it easier to write architecture verification tests - I can then explicitly decouple my Pipeline pre-processing from my Pipeline processor from my Pipeline post-processor - these could all live in different (sets of!) assemblies. This is more or less the goals of more functional frameworks like F# Giraffe. |
@davidfowl breaking IStartup will hurt existing apps upgrading to 3.0. Those apps were still going to work using the old WebHost. |
Then create a IStartup2 interface and deprecate the old IStartup with an
Obsolete warning telling people to forward to IStartup2. A bit klunky, but
better than this ridiculous POCO approach (sorry, can't hide my true
feelings for what a mess that is).
…On Mon, Jun 10, 2019 at 3:45 PM Chris Ross ***@***.***> wrote:
@davidfowl <https://github.com/davidfowl> breaking IStartup will hurt
existing apps upgrading to 3.0. Those apps were still going to work using
the old WebHost.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#11050>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADNH7OGVGGDVZL5FBUQL5DPZ2VMBANCNFSM4HWV5FPA>
.
|
@jzabroski The poco story supports more scenarios. I rather not have something called |
IStartupCore is clever and good. When you say the poco story supports more scenarios, are you thinking of deploying a Startup engine in multiple contexts, e.g. non-MVC, such that the Startup class does not even have to have a reference to MVC? Good in theory, strange in practice: Startup will reference a concrete WebHostBuilder, right? So how do you eliminate the private dependency on Microsoft.ASP.NET? Would you just use C# pre-processor #ifdef commands and compile two versions of your Bootstrap/Startup engine? I ask because this is an interesting user story I mostly hate in existing IoC projects: the whole "extend the IoC container via extension method" thing seems quite amateurish. It started with Ninject and that's been the standard design pattern ever since, and I think it was a mistake that needs to stop. The fundamental problem is that IoC frameworks are tightly coupled to the framework runtime hosting policy and how it handles assembly load contexts, threads and processes. - This is what extension methods SHOULD be doing, not some Ninject.Mvc thing you jam into your bootstrap. I can give you a concrete user story to illustrate my point: How do you have a Controller method that delegates to PLINQ and passes the currently authenticated user to the child threads spawned by PLINQ? Can you give me a concrete user story to demonstrate why a "poco story supports more scenarios" that are actually useful and solve real problems I have? |
What does that have to do with anything?
The Startup class is a bootstrapper, fundamentally it can't really have any dependencies inject into it since it's the one that has to configure more services. That limitation is even more evident in 3.0 as we disallow injecting anything into the startup constructor that wasn't already provided by the hosting (configuration and IHostEnvironment etc). To support injecting services, you can inject into the Configure method directly. That's the main use case that is broken by using the IStartup interface. |
I discovered two nights ago that Principals are dependency injected in ASP.NET Core, so they are bound to a scope, not a thread, so my point is likely irrelevant. Sorry.
An interface is not a dependency; the Startup class is the dependency. Dependency inversion principle: Depend on abstractions, not implementation details. Interfaces do NOT break use cases. They enable use cases. If the goal is to disallow configuring the Microsoft DI plumbing from binding IStartupCore to a Startup class, then add a feature to Microsoft DI to throw exceptions on certain invalid bindings. |
Startup is a bootstrapper not a dependency. It's the composition root for the application level container (well Configure is really). |
Having an interface is orthogonal to the fact it is a bootstrapper. The interface allows a generic constraint on the fluent interface for setting the composition root. You even agree by saying:
Netting it out, here are the correct tasks:
|
I agree there should be an interface as the startup class must adhere to a contract. Using convention is fine for people who don't wish to implement the interface, but it requires you "know" the convention or run the application then look at the resulting exception to learn what the convention is. Even then you don't have exact types, so you probably end up needing to review the documentation. I don't see how providing a strongly typed contract is not ideal. Can someone explain to my why convention is a better approach? And if so why aren't we using convention in ASP.NET core for things like identity, principal, etc. vs contracts (interfaces). As I said before I don't think the two need to be mutually exclusive. |
Yeah, it's a bit like the convention allowed in a C# foreach statement:
There is a convention, but the convention also has a contract: IEnumerator. Hope this parallel design helps explain what I'm looking for. |
I understand why people want for the interface and we’ll add one back. I was just clarifying some misconceptions about why both patterns exist and why IStartup is a bootstrapper/composition root and not a dependency |
Another suggested task (will update my previous comment as well):
@davidfowl It's an interesting side discussion I wish we could just have via instant message. I do not really understand (and thus cannot agree or disagree) with your stance that a Startup class is not a dependency. I agree that it is a bootstrapper/composition root, but at least when deploying code to environments where the host exposes a lot of static state, the Startup is actually the most critical dependency of all:
|
No, I don't think we'll do that.
It's OK to disagree, it's not worth a further discussion. |
@davidfowl @Tratcher we need to find the minimal thing needed in 3.0 as time is running very short. Can we do this in 3.1, since we weren't actually going to remove anything, just obsolete it? |
shrug either way. As you say, it's just guidance. |
So there is no way to add an IStartup instance in net core 3? Why? I don't want to pass a type, I already have an instance I am using, I want to use that. Impossible now in .net core 3? |
Jeff, you can still pass an instance but the contract isn't public. "Why" isn't worthy of discussion according to the team, despite the hundreds of lost hours of productivity this simple thing will cause across developers worldwide. |
How do I pass an instance? I only see the method that takes no parameter, i.e. |
This should be listed as a breaking change in the docs. I came here because the following working code now throws:
public class CustomWebApplicationFactory<TEntryPoint> : WebApplicationFactory<TEntryPoint>
where TEntryPoint : class
{
protected override TestServer CreateServer(IWebHostBuilder builder)
{
builder.UseEnvironment("Testing").UseStartup<TestStartup>();
return base.CreateServer(builder);
}
}
public class TestStartup : Startup
{
public TestStartup(IConfiguration configuration, IWebHostEnvironment webHostEnvironment)
: base(configuration, webHostEnvironment) { }
public override void ConfigureServices(IServiceCollection services)
{
services.AddSingleton(...);
base.ConfigureServices(services);
services.AddSingleton(...);
}
}
public class Startup : StartupBase { ... } I personally prefer the strongly typed class and am not sure why the convention based approach is pushed in this instance when it isn't for |
@SetTrend I appreciate the sentiment but renaming the concept of Startup to
Instead of calling your method Configure? What's the benefit? This feature exists for a small subset of people that want a contract and don't need injection of dependencies in to Configure. To that end, I think something like: public interface IStartupConvention
{
void ConfigureServices(IServiceCollection services, WebHostBuilderContext context);
void Configure(IApplicationBuilder app);
} Seems like it would make sense as a strawman proposal. Some things I would like to make clear:
|
Once I got my head around the configure services and configure app call, I decided to architect a delegate pattern. So I have a service runner class which has a constructor that takes an interface that allows hooking into various parts of the pipeline. The service runner calls the host builder, configure services, etc. then calls the delegate to give it an option to setup additional services. Same for configuring the application and various other bits in the setup pipeline. Whether this should be built into the core framework, that's hard to say. But for now, I don't have any concern about |
On the contrary. Renaming would decrease confusion. "Startup" can be anything and nothing.
Compared to the current implementation, the benefit – again – would be that you wouldn't need a crystal ball to tell what the method is or does, nor do you need external sources to tell what the name of the method must be for the system to work. Interfaces, contracts ... that's what object oriented programming - and robust programming - is about. It's not a game of hide-and-seek.
If dependency injection for |
We'll have to agree to disagree on that one. |
Is the codefix to warn about the use of |
The code fix is the warn about malformed or missing Configure (or malformed ConfigureServices). |
Ideally, the additional parameters would be injected through the constructor, which luckily is not part of the interface. |
Sounds a good idea to me. And that's absolutely what I would suggest striving for, too. However, there is a subtlety that needs to be dealt with: some of the objects initialized in For the sake of this discussion let's assume there are two kinds of services: (a) system provided, and (b) custom provided. System provided services may be injected with the constructor. Custom provided services currently are being initialized in That's the current approach as it is today. Supposed you switch to using the interface as suggested by @davidfowl (and I sincerely hope you do), this would cause a conceptual change:
Then all required services would be available in A sample implementation then would look like this: internal class WebConfiguration : IStartupConvention
{
private readonly IConfiguration _configuration;
private readonly ILogger _logger;
private WebHostBuilderContext _context;
private Options _options;
internal WebConfiguration(IConfiguration configuration, ILogger logger)
{
_configuration = configuration;
_logger = logger;
}
public void ConfigureServices(IServiceCollection services, WebHostBuilderContext context)
{
_context = context;
services
.AddSingleton(_options = _configuration.GetSection("AppSettings").Get<Options>())
.AddControllers()
;
}
public void Configure(IApplicationBuilder app)
{
if (_context.HostingEnvironment.IsDevelopment()) app.UseDeveloperExceptionPage();
DbContext.ConnectionString = Regex.Replace(_options.DbConStr, @"\|DataDirectory\|\\?", Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location) + @"\");
app
.UseHttpsRedirection()
.UseRouting()
.UseAuthorization()
.UseEndpoints(endpoints => endpoints.MapControllers())
;
}
} *) I'd like to stress again that It's a no-go, according to Microsoft naming guidelines. I strongly advice to overhaul the naming. Instead of shoving private fields back and forth in such a class — wouldn't it make sense, be much easier and more streamlined to just put all of it into one single function (defined by interface), i.e. omit the constructor and omit a second function? If required, the programmer then may split that function in whatever sub functions (s)he believes is appropriate. Such interface and corresponding sample class may look like this: interface IWebAppConfiguration
{
void ConfigureApp(IServiceCollection services, IApplicationBuilder app, WebHostBuilderContext context);
} internal class WebAppConfiguration : IWebAppConfiguration
{
public void ConfigureApp(IServiceCollection services, IApplicationBuilder app, WebHostBuilderContext context)
{
Options options;
services
.AddSingleton(options = services.Get<IConfiguration>().GetSection("AppSettings").Get<Options>())
.AddControllers()
;
DbContext.ConnectionString = Regex.Replace(options.DbConStr, @"\|DataDirectory\|\\?", Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location) + @"\");
if (context.HostingEnvironment.IsDevelopment()) app.UseDeveloperExceptionPage();
app
.UseHttpsRedirection()
.UseRouting()
.UseAuthorization()
.UseEndpoints(endpoints => endpoints.MapControllers())
;
}
} |
What parameters would those be? I have a feeling that based on the suggestions made, there's a fundamental disconnect and understanding on how Startup works. One that I have expressed in this thread several times but seems to have gotten lost in the details. Startup cannot support injected arguments that are not already materialized outside of host's dependency injection context. It can't because the container hasn't been built yet. The DI container has 2 phases:
This is why we have 2 methods, and this is why the phases exist. Hopefully that explains why things are split. @SetTrend I'd like to make it clear that there's low to no appetite to change anything as drastically as you have specified it here (in fact what you suggest doesn't even work practically but I rather not get into it):
Let me try to explain the reasoning behind this:
The next big change to the startup experience will solve a bunch of these problems all at once and not introduce new changes without huge benefits. |
You don't consider a number of blog posts written immutable and more important that robustness, testability and program design, do you? This is about the next generation of ASP.NET. SemVer: "MAJOR version when you make incompatible API changes".
What in particular does it increase?
Yes, from bad to good. The current concept is based on runtime-typing and otherwise typeless reflection. That's not a concept, that's a design flaw. If runtime typing was such a great feature, then why would they have invented TypeScript over JavaScript?
So what? Just take a deep breath and improve.
What makes you believe that compile time type checking wouldn't improve programming experience? Did you ask a DevOp manager about this?
lt doesn't keep you from doing this, either.
Gee ... So go on and make it async. The current version isn't async, either. |
Actually, when I think of composing bigger startup classes from smaller pieces, I think of F# Giraffe web framework, which is built on top of ASP.NET but uses F# pipelines to hide much of the ASP.NET Startup glue. It's actually not even about composing bigger startup classes from smaller pieces, it's about describing the pieces as pure computations with no side effects, so that there is a top-down explanation of the API and behavior that can be stepped through without reflection. Instead, Giraffe uses an HttpHandler that is a composition of functions similar to what is provided by IApplicationBuilder (and, in fact, to bootstrap itself, Giraffe uses IApplicationBuilder). Below is an example, originally taken from Scott Hanselman but his blog post example is also featured on the Giraffe github page : open Giraffe.HttpHandlers
open Giraffe.Middleware
let webApp =
choose [
route "/ping" >=> text "pong"
route "/" >=> htmlFile "/pages/index.html" ]
type Startup() =
member __.Configure (app : IApplicationBuilder)
(env : IHostingEnvironment)
(loggerFactory : ILoggerFactory) =
app.UseGiraffe webApp // THIS IS IT. This is basically the whole configuration. And, if you scroll down further on their page, they describe a F# Giraffe program that doesn't even use a Startup class. open System
open Microsoft.AspNetCore.Builder
open Microsoft.AspNetCore.Hosting
open Microsoft.Extensions.Hosting
open Microsoft.Extensions.DependencyInjection
open Giraffe
let webApp =
choose [
route "/ping" >=> text "pong"
route "/" >=> htmlFile "/pages/index.html" ]
let configureApp (app : IApplicationBuilder) =
// Add Giraffe to the ASP.NET Core pipeline
app.UseGiraffe webApp
let configureServices (services : IServiceCollection) =
// Add Giraffe dependencies
services.AddGiraffe() |> ignore
[<EntryPoint>]
let main _ =
Host.CreateDefaultBuilder()
.ConfigureWebHostDefaults(
fun webHostBuilder ->
webHostBuilder
.Configure(configureApp)
.ConfigureServices(configureServices)
|> ignore)
.Build()
.Run()
0 |
@jzabroski I don't see how this fixes the problems I'm talking about but F# syntax is succinct 😄 |
@SetTrend maybe it wasn't clear but I was explaining that the next turn of the crank for startup will tackle all of those problems. |
I think I would need specific examples of the problem, then, because the F# approach does solve several problems with structural composition (the composition is also not free; the trade-off is someone has to stitch together manually the whole pipeline). At the same time, you wrote the framework, so where I see problems you may see "RTFM" :-). For example, one I did not mention above, because I wanted to be brief, is that through using a Hindley-Milner type system, you can write code that manipulates an existential type through a universally quantified function. One particular use case for this technical concept is the question of how you can write open generic controllers that can then be "interpreted" by other parts of the framework, like a Swagger endpoint. In ASP.NET Core Swagger, if you try to |
Moving to Backlog to consider for 6.0 at this point. |
We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process. |
IStartup isn't going to work with generic host which is the default in 3.0. It also conflicts with a few other patterns like ConfigureTestServices.
See #11001
@davidfowl
The text was updated successfully, but these errors were encountered: