-
Notifications
You must be signed in to change notification settings - Fork 707
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
Add synonym to AddMvc
method.
#1089
Comments
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 Only some DI extensions are directly chainable. As you've even shown above, 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:
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:
The convention for adding services, even via a builder, is with the 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:
There's even an entire page dedicated to migration that convers all of the changes that came in 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 The simplified setup for 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. My code was:
Then I added versioning:
MVC 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 that
AddMvc
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.
The text was updated successfully, but these errors were encountered: