Package development defaults and good practice

[KevinJump]

Moving to Umbraco 9 and .dotnet core changes the behaviors of somethings and moves things around.

For package devs this leads to questions such as ‘where should this go’, and what is the best thing to do when adding X to a site.

Umbraco and .Net are highly flexible when it comes to this, so you can put things almost anywhere, but for the sanity of Developers and users of Umbraco it might make sense to have some ‘conventions’ as to what to do.

This purpose of this discussion is act as a place where we can gather some default suggestions, and maybe formulate some 'best practices' that could then make their way to the Umbraco docs as a guide page.

All of the below are suggested starting points not recommendations (yet)

App_Plugins

Files in App_Plugins should be immutable, in that is should not be something the user changes ? - because most packages are being delivered using a .targets file to copy files into App_Plugins at build time, there is a risk that any changes the user makes will be overwritten at build time, or when a package it updated.

Also the default target file behavior cleaning a build will remove the all the package files in App_Plugins so this will wipe all of a users changes?

files that change/can be changed after a package is installed should be put in another location.

Views

If a package contains views, then traditionally these would be added to the packages App_Plugins folder. With .net core it is probably better not to include files that can change/will change in the App_Plugins folder as this folder is/can be wiped on a clean build. (and files copied to it as part of a build).

Package views example

Umbraco forms, stores its views in /wwwroot/views/partials/forms,

Maybe views should be split out of app_plugins and live in the views folder if the intention is users can alter them after the package has been installed ?

question is how do you deal with updates / upgrades - package dev would need to ensure these files are only overwritten the first time

License files

Umbraco products store their licenses in /umbraco/Licenses and it is considered a good location for third party packages that require license files to also store their license files here.

The default .gitignore for Umbraco templates will include this file in a repo (while ignoring most of the rest of the Umbraco folder.

Note :

• The folder does not exist on default installations. So if a package needs it then it will have to create it.
• For case sensitive folders (linux) note lowercase “u” uppercase “L” - /umbraco/Licenses

[KevinJump]

Also

Settings

packages should not write to appsettings.json directly. the nature of settings in .Net core means that settings can come from 5 different locations so writing to this file can override/cause unexpected behavior for users who are using these other locations.

Possible options ??

• require minimal config, using sensible defaults in code, so most users don't need to configure
• store app specific settings in the DB (be aware of how deployment of a site might not include these settings)
• when config is required show users/document the changes needed to the file (via a popup in the UI?)
• store settings in a different location on disk ? (but this looses a lot of the .net core flexibility on settings)*

* in theory there is a way to add your own config file into the appsettings config chain, but currently this might not be possible without altering the startup.cs file, which doesn't make it a good pattern for package development.

[p-m-j]

in theory there is a way to add your own config file into the appsettings config chain, but currently this might not be possible without altering the startup.cs file, which doesn't make it a good pattern for package development.

You can do this with IHostingStartup but requires an environment variable to be set which also isn't ideal.

We may be able to add an extension point to the host builder setup in a similar fashion to IComposer type finding.

[KevinJump]

yeah i think an extension point is either missing or we just haven't found it yet. i know Warren took a quick look - but i don't think he got much further.

[JasonElkin]

I've only consumed, not created so far, but don't Razor Class Libraries effectively solve the problems mentioned with the App_Plugins and Views approaches?

[KevinJump]

yeah i don't know 🤷 , i suppose it depends if the views are not meant to be altered, then that's fine (but so is app_plugins) its really what do you do if you think your users will/want to alter the views ? then they have to be somewhere for them to do it.

[NikRimington]

This raises questions for things like email templates and the like, all of which are razor views. If they were in the Views directory, IIRC they would be compiled by default now (is that right??).

But, that aside maybe views should be put in /views/plugins/ << I know it's a new path but we don't know what types of views packages might be including, they might be more than just partials and it makes sense from a naming convention to me.

Also, could a similar folder structure for config exist so that it breaks config away from app_plugins for the same reasoning?

[KevinJump]

yeah i wonder if a new path convention is a good idea, because it will at least separate these things out from people's sites so they can easily tell what is and isn't custom / imported code

[JasonElkin]

compiled by default

And that's my problem. I don't think I should be compiling (or seeing) views that I'm not supposed to edit. I'm already frustrated by the warnings from Umbraco's views because I want to use Nullable Reference Types in my project.

they have to be somewhere for them to do it.

What if, by default, views were in an RCL but the package also shipped the overriding views as something like view.cshtml.example that could simply be renamed to override the RCL?

[KevinJump]

What if, by default, views were in an RCL but the package also shipped the overriding views as something like view.cshtml.example that could simply be renamed to override the RCL?

I am not 100% up on this (most of my packages don't have views), but it sounds interesting, I might have a play. I can see a case for the core views being pre-compiled, but again not sure of the implications. #hacktoberfest

I actually think one of the issues you have with the nullable warnings is they are coming from the "template" files which are used when people create new macros in the backend so aren't "live" as it where - without going off topic too much they could be fixed in core, or potentially renamed so they are not .cshtml files when they are not actually used (i would think fixing is a better solution - probably something for that issue in the core).

[nathanwoulfe]

I've been fiddling with my .targets this morning, and have a workable, if not entirely elegant solution for preserving local changes on rebuild/clean. It's reliant on the fact that a locally modified file will have LastWriteTime > CreationTime, since a file installed from the package will have a creation time from when the local copy was created, but write time will be the time the file was last edited before packaging (shout if that's incorrect, my tests so far hold true).

On rebuild, I'm now only copying files where the corresponding local copy hasn't been modified. To do that, I've added a task to my .targets file:

  <UsingTask
    TaskName="FindUnmodifiedEmailTemplates"
    TaskFactory="CodeTaskFactory"
    AssemblyFile="$(MSBuildToolsPath)\Microsoft.Build.Tasks.v4.0.dll" >
    <ParameterGroup>
      <SourcePath ParameterType="System.String" Required="true" />
      <TargetPath ParameterType="System.String" Required="true" />
      <FilesToCopy ParameterType="System.String[]" Output="true" />
    </ParameterGroup>
    <Task>
      <Using Namespace="System.IO" />
      <Using Namespace="System.Linq" />
      <Code Type="Fragment" Language="cs">
        <![CDATA[          
          var targetInfo = new DirectoryInfo(TargetPath);          
          var sourceInfo = new DirectoryInfo(SourcePath);
          
          if (targetInfo.Exists && sourceInfo.Exists) {          
            var targetFiles = targetInfo.GetFiles();    
            var sourceFiles = sourceInfo.GetFiles();

            var modifiedLocal = targetFiles.Where(file => file.LastWriteTime > file.CreationTime).Select(file => file.Name).ToArray();   
            FilesToCopy = sourceFiles.Where(file => !modifiedLocal.Contains(file.Name)).Select(file => file.FullName).ToArray();     
          }
        ]]>
      </Code>
    </Task>
  </UsingTask>

Then call the task:

    <FindUnmodifiedEmailTemplates SourcePath="$(MSBuildThisFileDirectory)..\Content\Views\Partials\WorkflowEmails"
                                  TargetPath="$(MSBuildProjectDirectory)\Views\Partials\WorkflowEmails">
      <Output TaskParameter="FilesToCopy" ItemName="ViewsToCopy" />
    </FindUnmodifiedEmailTemplates>

The output is the array of non-modified filepaths, which can then be copied:

    <Copy
      SourceFiles="@(ViewsToCopy)"
      DestinationFolder="$(MSBuildProjectDirectory)\Views\Partials\WorkflowEmails"
      SkipUnchangedFiles="true" />

Is it perfect? Hells no, but it works (and needs some clean up). I'm doing similar in clean, but the custom task outputs an array of file paths representing the unmodified views.

So far, it seems like this will let me put views anywhere, since I can be pretty specific about which folder to touch during clean/rebuild

As for files in app_plugins, currently I'm planning on deleting /Backoffice and /Lang directories, plus the manifest. Anything else will be left as-is, which for me is only the .lic and .key files (which may end up moving, if there's an accepted convention for where these best live).

[KevinJump]

It's reliant on the fact that a locally modified file will have LastWriteTime > CreationTime, since a file installed from the package will have a creation time from when the local copy was created, but write time will be the time the file was last edited before packaging (shout if that's incorrect, my tests so far hold true).

What happens on Package upgrades ? (e.g user installs package, edits file, you update package some time later, new package has later date than user file)

[nathanwoulfe]

On update, I'm only going to replace files where they haven't been modified, so if user has updated an email template, and I update the same view in a later release, my changes don't overwrite the local changes.

I couldn't see any other way to manage it - it's either overwrite with the new file and kill the local updates, or keep local at the expense of package updates. Neither is perfect.

[p-m-j]

I think a razor class library is probably the way to go unless there’s a very specific reason it doesn’t work for your use case.

e.g starter kits but those should probably be dotnet templates

[KevinJump]

i think packages do have a use case to ship editable views. @nathanwoulfe's example is good for this, email templates that work in one way default and then the user can edit.

are we sure that if the code contains a razor class library, it can be overridden (easily) by the end user adding a template (so the defaults would be in a class library, and could be overridden?)

[p-m-j]

My understanding is local files take precedence over those in an RCL but I haven’t put it to the test.

edit: and even with this, it may not be suitable for everyone, just another tool in the box.

edit 2: Works good for razor views, not so much for static files aspnet/Announcements#443

[p-m-j]

Spent some time (more than I intended) hacking on a proof of concept for an RCL with razor view and static file overriding downstream.

It's definitely not as smooth as it could be, but with a bit of work on a custom task I think we could make things easier.

https://github.com/p-m-j/umbraco_plugin_rcl_poc

[skttl]

I'm doing a package where I want the users to be able to add API-keys through the backoffice. Where should I save these? Is the umbracoKeyValue table suitable for this? I would prefer a solution, where I can have Deploy be able to pick up changes (even if I should write my own handler for that).

Previously I have exploited dictionary keys for this, but that feels wrong 🙂

[AndyButland]

umbracoKeyValue is suitable I would say (have been looking at doing that this week for a package). But it's not something Deploy currently handles, as values in here will often be environment specific (e.g. migrations).

[ronaldbarendse]

Most (if not all) of these are now added to the documentation, see PRs umbraco/UmbracoDocs#3829 (original, closed) & umbraco/UmbracoDocs#5156 (updated, merged) and the final page on https://docs.umbraco.com/umbraco-cms/extending/packages/good-practice-and-defaults.