Open voroninp opened 4 months ago
This is intentional and by design. The new approach was outlined in the roadmap 2 years ago. The full gambit is:
services.AddApiVersioning() // Core API Versioning services with support for Minimal APIs
.AddMvc() // API version-aware extensions for MVC Core
.AddApiExplorer() // API version-aware API Explorer extensions
.AddOData() // API versioning extensions for OData
.AddODataApiExplorer(); // API version-aware API Explorer extensions for OData
Existing users have been thrown off a bit because AddMvc
wasn't necessary in the past, but that's because Minimal APIs didn't exist nor were supported.
Only some DI extensions are directly chainable. As you've even shown above, IServiceCollection.AddMvc
doesn't return IServiceCollection
, it returns IMvcBuilder
. AddMvcCore
returns IMvcCoreBuilder
. IApiVersioningBuilder
works in a similar fashion. All of these builders provide IServiceCollection Services { get; }
.
If you wanted to make one big chain, you still can with:
builder.Services.AddApiVersioning().AddMvc().Services.AddSwaggerGen();
There were several motivations behind this change:
services.AddVersionedApiExplorer()
before services.AddApiVersioning()
services.AddApiVersioning().AddApiExplorer()
vs services.AddVersionedApiExplorer()
IApiVersioningBuilder
In addition to the roadmap, this information has been conveyed in:
I've gone to great lengths to try and make it concise, but still obvious. There are only two scenarios that seem to consistently hang people up:
IApiVersioningBuilder.AddMvc
with controllers, which I don't have a better solution for.AddMvc
adds the full MVC stack versus MVC Core (which is silly for APIs so I thought it was obvious; in hindsight perhaps AddMvcCore
would have been better)The convention for adding services, even via a builder, is with the AddXXX
prefix. You can see that even IApiVersioningBuilder.AddApiExplorer has to call through to IServicesCollection.AddMvcCore → IMvcCoreBuilder →
IMvcCoreBuilder.AddApiExplorer.
At this point, I'm not really inclined to change things. There hasn't been an outcry from the community. I'm still willing to entertain the idea however. I will leave the issue open and if there is a significant upvote, I'll consider it. Aliases are nice, but they also have the risk of causing confusion and changing them will result in a breaking change - at some point (there's no point in having both - forever).
Then I'd propose to emphasize in wiki and examples that these methods are not the standard one from the framework.
In the wiki, there is already the text:
.AddMvc(); // ← brings in MVC Core; unnecessary for Minimal APIs
There's even an entire page dedicated to migration that convers all of the changes that came in 6.0
that should help people moving from Microsoft.AspNetCore.Mvc.Versioning
to Asp.Versioning.Mvc
.
Perhaps more text in the examples. The examples are setup to boil things down to just the bare minimum API Versioning configuration with comments and options highlighted for the common knobs that someone might want to turn. The example itself is kind of the documentation. IMHO, it should be obvious that everything shown is working and required. Turning a knob is one thing, but removing a knob, such as removing the call to .AddMvc()
, should be expected to break things.
The simplified setup for WebApplication
removes the necessity for AddMvcCore
and AddMvc
. That should also be a signal that it's different. It would probably be more obvious if they were still side-by-side:
builder.Services.AddMvcCore();
builder.Services.AddApiVersioning().AddMvc();
This is actually the case with OData, which requires:
builder.Services.AddOData();
builder.Services.AddApiVersioning().AddAddOData();
Let me explain how it was for me.
The last time I used your package was before minimal APIs.
For minimal APIs to work you also do not need to call standard AddMvc()
or AddMvcCore()
. So when I read that comment, my thought was: "Thank you, I know." 😂
My code was:
services.AddMvc().AddApplicationPart(...);
Then I added versioning:
services.AddMvc().AddApplicationPart(...);
services.AddApiVersioning();
MVC added? Check. Versioning added? Check.
;-)
Yep, I get the surprise and there was no great way to fully advertise that. Behavioral changes always concern me the most because they are the least obvious and hardest to find. Unfortunately, they aren't always fully avoidable.
It's been 2+ years, so I could update the announcement banner to reflect a migration notice and like to the appropriate wiki topic instead of just an announcement. Would that have - perhaps - made things slightly more obvious? That's pretty quick and easy change.
I guess so.
But you know the curse of knowledge. :-) It's always hard to predict how those who are unaware can be surprised:-)
POLA is part of every decision. Unfortunately, it's just not always easy to advertise changes.
Background and Motivation
Extension method which operate on
IServiceCollection
type are usually chainable. When I saw examples it was not immeidately obvious thatAddMvc
is not the same from Asp.Net Core:vs
It took me a while to understand why versioning was not working for controllers.
Proposed API
My proposal is to add alternative method named
WithMvcSupport()
which, IMO, will cause less confusion and will be more descriptive. (AlsoWithApiExplorerSupport
)Usage Examples
Risks
If this proposal is accepted, the interface will epand, so is the need to support it. Maybe old methods can be deprecated with time.