Removing explicit namespaces for UmbracoViewPage in templates

[callumbwhyte]

When creating templates in the backoffice the editor adds a bunch of @using's that ultimately don't appear to be needed.

This might be a legacy thing where IDE's wouldn't pick up references in the Views/web.config and so without those lines you'd have no intellisense.

With V9 / netcore these references are included in the _ViewStart.cshtml, which most modern IDE's (Visual Studio, VS Code, Rider) do a good job of picking up. I see no reason why these lines should be there, and we can instead encourage devs to keep their views cleaner.

This PR removes the explicit Umbraco.Cms.Web.Common.PublishedModels and Umbraco.Cms.Web.Common.Views references from templates created via the CMS. The former is not needed, as there's already the @using ContentModels = ... alias.

If I've overlooked something here please do tell me!

[umbrabot]

Hi there @callumbwhyte, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[matthewcare]

Hey @callumbwhyte
I opened a PR already which allows for configurable default view content, as I also disliked removing a bunch of default content that wasn't needed
#11317
Is it worth me updating to include your changes here as well?

[bergmania]

Hi @callumbwhyte ..

These namespaces are required right? The only reason they do not show up as required, is because the are also present in the _ViewImports from the dotnet new templates?

So if you are just installing Umbraco into a plain empty solution these would be required?

[callumbwhyte]

Hey @bergmania,

Great point, I definitely hadn't considered the scenario where someone might not install with the templates...

This raises some further questions for me though... Is installing without templates a supported example? I assume someone would have to manually setup their Startup.cs accordingly to get the project to run?

Following your same argument that these users would have to add explicit references for their models, the same goes for Umbraco.Extensions, tag helpers, etc, whenever they're needed (some of which don't get easily picked up by intellisense). The whole experience feels a little clumsy...

As my proposed change is for users of the backoffice template editor, what if we added some logic to ensure a standard _ViewImports exists whenever a template is created via the backoffice? This way they can have more-or-less the same experience as with the dotnet templates?

Let me know what you think!

[bergmania]

This raises some further questions for me though... Is installing without templates a supported example? I assume someone would have to manually setup their Startup.cs accordingly to get the project to run?

I'm not sure if it is officially supported but it is definitely possible and I have heard of some doing it this way.
Correct you will have to configure Startup.cs your selves, but this is the normal/expected pattern when using packages in ASP.NET Core.

Following your same argument that these users would have to add explicit references for their models, the same goes for Umbraco.Extensions, tag helpers, etc, whenever they're needed (some of which don't get easily picked up by intellisense). The whole experience feels a little clumsy...

I totally agree, that's why we ship the templates :)
But I think there could be scenarios where you have an existing project ASP.NET Core and wanna add Umbraco. In these cases I see no other way then doing it manually.

As my proposed change is for users of the backoffice template editor, what if we added some logic to ensure a standard _ViewImports exists whenever a template is created via the backoffice? This way they can have more-or-less the same experience as with the dotnet templates?

That could be an option, but also a bit magically that the _ViewImports are created in such an operation IMO.

Alternatively - which is a bit complicated due to GetDefaultFileContent is static - But if we supported some TemplateSettings using the IOptions Pattern. Then we could have some default TemplateSettings.DefaultViewImports, with the value as you suggest. By using the IOptions Pattern this default value could be changed in code.

To make this in a good way, we need to move GetDefaultFileContent into an instance method. Obsolete some ctors, injection the needed IOptions in new ctors, and register the ViewHelper into the DI container.

[matthewcare]

@bergmania

To make this in a good way, we need to move GetDefaultFileContent into an instance method. Obsolete some ctors, injection the needed IOptions in new ctors, and register the ViewHelper into the DI container.

As mentioned, PR #11317 does exactly that.

[bergmania]

@bergmania

To make this in a good way, we need to move GetDefaultFileContent into an instance method. Obsolete some ctors, injection the needed IOptions in new ctors, and register the ViewHelper into the DI container.

As mentioned, PR #11317 does exactly that.

Cool.. Sounds good, then I think that PR can be updated to the suggestion from this PR, and all is happy :)

[matthewcare]

@bergmania

I'm not sure it's really needed. Shipping with two view content providers, one for if the template is used, and one for if it isn't, or adding more template configuration options seems like adding bloat and more to code maintain for no real benefit.

My PR already allows a user to override the default view content should they wish, and the current default works for both templated solutions, and direct installs.

Happy to reconsider if it's something that you think will be useful though.

[bergmania]

@matthewcare, my idea was to just ship with one. And that single one could be optimal for when using the template. But now with this PR, it is possible to override, so I think that is okay for the few that do not use the template.

So my suggestion was simply just to do this in your PR:

+           content.AppendLine("@using Umbraco.Cms.Web.Common.PublishedModels;");
+           content.Append("@inherits Umbraco.Cms.Web.Common.Views.UmbracoViewPage");
-           content.Append("@inherits UmbracoViewPage");

[matthewcare]

@bergmania

I think I'll leave @callumbwhyte to update this PR to avoid expanding the scope of mine.

With the further dicussions here, you'd realisitcally be looking at a view template which generates the following

@inherits UmbracoViewPage<GeneratedModel>
@{
	Layout = null;
}

As @using ContentModels = Umbraco.Cms.Web.Common.PublishedModels; isn't going to be needed (included in the view imports file), which will result in updating more tests, and probably introducing a couple more.

If we're removing namespaces, then we should also be looking at partial view / macro generation.
https://github.com/umbraco/Umbraco-CMS/blob/v9/contrib/src/Umbraco.Infrastructure/Services/Implement/FileService.cs#L36

[nathanwoulfe]

Hey @callumbwhyte where's this one at? #11317 was merged a while back, does that impact on your changes here?

[emmagarland]

Hi @callumbwhyte!

Just going through some of the longer running PRs, and noticed this one (it's been a while, so there are now some conflicts sorry!)

Did #11317 affect this PR, or did you need anything else to progress this one?

Thanks,

Emma 🙂

[nul800sebastiaan]

I'll close this one for now with the conclusion that #11317 does the trick for now. I feel like we need a bit more of a discussion first as to what needs to be generated before we look at a PR again so feel free to raise it at https://github.com/umbraco/Umbraco-CMS/discussions and then we can come to a consensus. Thanks all for being involved so far! 👍