Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

AddMvc builder calls redundancy #5934

Closed
stevejgordon opened this issue Mar 9, 2017 · 7 comments
Closed

AddMvc builder calls redundancy #5934

stevejgordon opened this issue Mar 9, 2017 · 7 comments
Labels

Comments

@stevejgordon
Copy link
Contributor

This may be more a query than an issue. I've been looking at the source, specifically the AddMvc extension method.

Part of the code looks like this

builder.AddViews();
builder.AddRazorViewEngine();
builder.AddRazorPages();
builder.AddCacheTagHelper();

// +1 order
builder.AddDataAnnotations(); // +1 order

Within AddRazorViewEngine it calls AddViews. Therefore, why do we call AddViews first. Seems redundant? Am I missing a reason, other than this method being a bit more explicit about what it's adding?

In addition, AddDataAnnotations is called within AddViews. Therefore, calling it explicitly later on also seems redundant? It seems that the lines above could actually be reduced to the following, and still perform equally.

builder.AddRazorViewEngine();
builder.AddRazorPages();
builder.AddCacheTagHelper();

Interested to understand the design decisions here?

@rynowak
Copy link
Member

rynowak commented Mar 9, 2017

The AddXyz methods automatically add their dependencies because it's much friendlier that way.

this method being a bit more explicit about what it's adding

That's exactly the reason. Someone would be asking the opposite question if we pared it down to the 'minimal' set of AddXyz calls.


Breaking up the monolith into pieces was complex and sometimes forced us into some inelegant choices. I think I'll still be answering questions on this for the rest of my life 👍

@Eilon Eilon closed this as completed Mar 9, 2017
@stevejgordon
Copy link
Contributor Author

Thanks @rynowak and that makes sense. Appreciate the answer. I'm writing a blog about the AddMvcCore and AddMvc methods so it's good to know the reason.

@Eilon
Copy link
Member

Eilon commented Mar 10, 2017

Send us a link when you post it!

@stevejgordon
Copy link
Contributor Author

@Eilon - Sure, here is the first part - https://www.stevejgordon.co.uk/asp-net-core-mvc-anatomy-addmvccore - Hopefully it's all accurate!

@stevejgordon
Copy link
Contributor Author

And this post features @rynowak's quick feedback! - https://www.stevejgordon.co.uk/asp-net-core-anatomy-part-2-addmvc

@rynowak
Copy link
Member

rynowak commented Mar 17, 2017

These are great posts, love them both!

A little more info about the ApplicationPartManager. It's complicated because of the methods like AddControllersAsServices(). This methods go discover all of the controllers (or tag helpers or view components) and then register them with the service collection. This means that ApplicationPartManager needs to be 'baked' before services are available, which is a significant challenge in our system.

The other complication is that the ApplicationPartManager needs to be customizable in our tests. We have a bunch of test websites for functional testing for MVC and we want to make sure in our tests that the controllers from each website don't leak into the other websites' tests. See https://github.com/aspnet/Mvc/blob/dev/test/Microsoft.AspNetCore.Mvc.FunctionalTests/MvcTestFixture.cs for the juicy bit.

@stevejgordon
Copy link
Contributor Author

Thanks @rynowak - I really appreciate the feedback and additional info. I'll take a look at those extra links.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants