Umbraco-cms: Simplify service registration and use MSDI abstractions throughout.

[JimBobSquarePants]

I don't know how to phrase this better but drop the tight coupling with LightInject like a hot stone for the .NET Core implementation.

This was discussed at great length pre V8.

https://issues.umbraco.org/issue/U4-11427

There was even an attempted PR. Unfortunately V8 was launched before it could be completed. In my opinion it also did not go far enough to simplify registration of services.

https://github.com/umbraco/Umbraco-CMS/pull/3955

The current DI implementation hurts extensibility through it's complexity. Package development for V8 still undeniably suffers for a lack of ecosystem. There were slack channels created by package developers specifically to discuss navigating this complexity.

Using a custom LightInjectContainer is never a good idea; neither is running multiple containers.

CC/ @lars-erik

P.S _I'm absolutely putting my hand up here to help design a more simplified approach._

[lars-erik]

I'm sorry to say I haven't been able to look much at the core port yet. Hopefully I suddenly will.
In any case, I wholeheartedly support the notion that Umbraco should rely on MS.DI abstractions alone for the core port.
I don't mind whether the current composition abstraction stays on top, as long as it exposes the same possibilities. It will at least keep Umbraco code resilient from future container changes. It may well be it deserves an overhaul to better align with MS.DI tho.
Whether the default container or lightinject is used behind the scenes ootb I don't care, but it has to work with -, and support changing to any container supporting MS.DI.
WFIW, I insist. 🤣

[lars-erik]

I do believe this would mostly still just work:
https://github.com/lars-erik/our.umbraco.containers/blob/master/Our.Umbraco.Containers.MS.DI/ContainerAdapter.cs
Got tests as well:
https://github.com/lars-erik/our.umbraco.containers/tree/master/Our.Umbraco.Containers.MS.DI.Tests

(tho I dunno if I ran them before last commit. 😇)

[JimBobSquarePants]

I say kill IRegister and IFactory.

[lars-erik]

Not sure about IFactory, but IRegister can certainly become IServiceCollection. 👍
I'd expect the MS.DI factory abstraction would be accessible through a prop or method on IFactory tho.

[JimBobSquarePants]

At no point should there need an explicit concrete reference to ServiceCollection, only IServiceCollection. IFactory exists to service a container which shouldn't ever be required either.

IMO all you should ever care about is consuming IServiceCollection, and IConfiguration and adding extension methods to the former or using an IUmbracoBuilder implementation or suchlike to allow simplified registration of certain types.

``` c#
// Startup.cs ConfigureServices
IUmbracoBuilder builder = services.AddUmbraco(this.IConfiguration);

// Startup.cs Configure
app.UseUmbraco(...);
```

[lars-erik]

Abstraction miss fixed. 👼
All I'm saying is that registered factory methods should have access to an IFactory.
I believe MS.DI provides one, so by all means, it could be it.

(A bit sluggish on the APIs atm. Still mostly hacking framework stuff with V8.)

[JimBobSquarePants]

All I'm saying is that registered factory methods should have access to an IFactory.

I don't follow you here. The IFactory is used to define a conforming container. You don't need or want to be referencing a container implementation anywhere in the codebase. Any custom container (_Not recommended by MS_) implementation is wired up at ConfigureServices the final caller.

Perhaps you mean IServiceScopeFactory? This can be injected any time (but should be considered a tool of last resort).

[Shazwazza]

Hi all! Glad this has been resurrected. It's was an item on the original RFC for netcore to look into once we were a little further on with development. There's still a long way to go but prob a good time to make some decisions and come up with a design for how we want this to all work. As you both know there's all sorts of things to think about for this refactor so it's great that you were both very much involved with the orig discussions :)

I'm all for simplification and removing LI and don't think anyone would object to that 😄 so just need to figure out where/how to start with this and what the end goal is. The main thing to keep in mind is that the Umbraco.Core project isn't meant to have any dependencies (clean architecture) and contain only our own abstractions. This is where things like IFactory, etc... are defined. So a big decision would need to be made:

1) Do we keep this guideline, don't take any dependencies in Umbraco.Core, keep/modify our own abstractions
1) Do we break this guideline and takes a dependency on Microsoft.Extensions.DependencyInjection in Umbraco.Core for it's DI abstractions
1) Do we keep this guideline, don't take any dependencies in Umbraco.Core, and move anything that needs a reference to MSDI abstractions to the Umbraco.Infrastructure project (I'm not sure if this is at all possible FYI, just thinking out loud

I believe Lars's previous work and suggestion is to keep option #1 and then implement the abstractions with MSDI. I think this would also mean a lot less code to change. Maybe there's a 4rd/5th/etc... option too?

I guess the main goals here would be to:

• Hopefully not take any dependencies in Umbraco.Core
• Remove LI
• Try to modify as little as possible especially currently used public APIs like the composer/components and collections

What do you think?

And how do you think it can be tackled so we can review/approve in small iterations so we don't end up with someone spending a huge amount of effort for it to be left out? Perhaps once a direction is decided (or needs to be proved) a small POC can be made (if possible) to help showcase the intention.

Cheers!

[JimBobSquarePants]

The main thing to keep in mind is that the Umbraco.Core project isn't meant to have any dependencies (clean architecture) and contain only our own abstractions.

I do not understand this sentiment. You're already taking dependencies on all the System.*. Having a dependency doesn't make things unclean.

That said....

Looking at the codebase (Infrastructure) it looks like you are creating your own container instance and then passing that instance around via IFactory. You should never have to do that. The only time a container should be created is at the topmost caller level (_Web App creation_) or during tests. Any service registration should be done via IServiceCollection. Everything else should be blissfully unaware.

Try to modify as little as possible especially currently used public APIs like the composer/components and collections

This is where you will never win me over. It's so overly complicated and unnecessary. There are already abstractions available for registration via IServiceCollection. Any syntactic sugar can be provided as extension methods upon that abstraction.

[Shazwazza]

Hi @JimBobSquarePants,

I'm wondering if it would be easiest to do a zoom call about all this stuff since there's probably a lot of questions/answers/concerns/reasons underpinning the current state of things and where we want to go. Would you and @lars-erik be available some time next (any) week preferably either Mon or Thur (since those are my late days but happy to do any other day too!)?

Regarding the 'Clean Architecture', that's just referring to the direction the codebase has been restructured (still ongoing), see https://blog.cleancoder.com/uncle-bob/2012/08/13/the-clean-architecture.html where the general rule is that the Domain/Core has no external dependencies. In this case though - in my personal opinion - I don't think that referencing Microsoft.Extensions.DependencyInjection is necessarily regarded as an 'external dependency'.

Try to modify as little as possible especially currently used public APIs like the composer/components and collections

Let me try to re-phrase this since I don't think the way I worded that was very clear - also why I'd love to have a call about all this stuff :) What I'm trying to say is that one goal of this release is to reduce the friction for Umbraco developers to make the transition from Umbraco 8 to the Umbraco netcore version and as such we are trying to maintain (where possible) the public APIs and patterns that exist in Umbraco 8. For developers to interact with Umbraco's startup, container configuration, plugins, collection builders etc... we have our own IComposer/IComponent which we need to keep. If changes to these types of things are necessary that is fine but we want to minimize the changes where possible. So it's totally fine we make changes but the goal is to minimize the transition for Umbraco devs from v8 to the netcore version.

I'm sure you'll have a ton of questions as to 'why' if you are perusing the code, there are reasons for everything, both good and bad and things are in flux. I just think a call to do a recap of the current state would be easiest, else we'll be writing responses this long or longer for quite some time 😂

We're totally open for suggestions and if possible would love to get to a point to see a proof of concept for desired changes.
Cheers!

[JimBobSquarePants]

Hey @Shazwazza happy to make time for a chat. Just concerned the conversation will be lost to the public.

Regarding IComponent.... That's a worrying API for several reasons.

• It passes a concrete Composer instance which tightly couples implementation to that type.
• Composer has a public method CreateFactory which returns the container!

You should never, ever have to return the container instance anywhere in your API.

This is why I get worried when I see a major goal repeated that V8 API compatibility.

[lars-erik]

Happy to make time as well. Mostly free, but unsure how my free time matches aussie timezone.

Here's a slide from my UK Fest 2019 talk on wishes for DI in Umbraco. Note the package dev guidance. 👼

(All slides here: https://1drv.ms/p/s!AnYHs3nuLdwBlKxbsi6BCj42REN7vw?e=uhB4SZ)

[Shazwazza]

Hey @JimBobSquarePants + @lars-erik , sorry for the delay, was off Friday to get a tooth implant so I can stop looking like a hillbilly 😂 (long story!)

Just to recap on some stuff:

Just concerned the conversation will be lost to the public.

We had a plan to do an RFC for this exact phase at some point so we might as well start that now with the help of you both and whoever else might like to join. My proposal is to have a first call soon just to go through questions/concerns/current/future, I'll take notes and post them back here and get an RFC draft started and you can help fill in any info, etc... if you want.

Regarding changes and cleanup to the code - We're all for it! It's not about "V8 API compatibility" 1:1 or anything like that, it's just about maintaining similar concepts. If people need to change code that's fine but it will be much friendlier if that can be minimized and perhaps even done with a find/replace.

What do you think about next week for timing? If you can do Monday Aug 31 I would propose a 30 min chat (longer if we want it) anytime between Sydney 19:00-23:00 (CPH 11:00-15:00) onwards. I can make that same time frame work from Mon-Thur next week.

https://www.timeanddate.com/worldclock/meetingtime.html?day=31&month=8&year=2020&p1=240&p2=69&iv=0

Let me know what you think :)

[lars-erik]

@Shazwazza the timeframe works for me, although I might be 10-15 min late in order to fetch lunch and make eating sounds for the first 20 min. 👼

[JimBobSquarePants]

@Shazwazza That works for me, looking forward to it!

[Shazwazza]

@lars-erik + @JimBobSquarePants

Sorry should have posted this on Friday but better late than never :) Here's the meeting invite details for tonight (morning for you!). If anything has changed and you guys can't make it, no problemo and we can reschedule.

Topic: Umbraco netcore MSDI chat
Time: Aug 31, 2020 07:00 PM Canberra, Melbourne, Sydney

Join Zoom Meeting
https://us02web.zoom.us/j/88446112341

Meeting ID: 884 4611 2341
One tap mobile
[phone],,88446112341# US (Houston)
[phone],,88446112341# US (New York)

Dial by your location
[phone] US (Houston)
[phone] US (New York)
[phone] US (San Jose)
[phone] US (Tacoma)
[phone] US (Germantown)
[phone] US (Chicago)
Meeting ID: 884 4611 2341
Find your local number: https://us02web.zoom.us/u/kcxhWnBIKx

[Shazwazza]

@lars-erik + @JimBobSquarePants we'll reschedule to Thur if James can make it. Have pinged you on Twitter and we can setup some cal invites. Cheers!

[rustybox]

and whoever else might like to join

I'd love to sit in on this, is this open to all?

[Shazwazza]

Hi @rustybox yep for sure, here's the zoom information for tomorrow:

Shannon Deminick is inviting you to a scheduled Zoom meeting.

Topic: Umbraco netcore MSDI chat
Time: Sep 3, 2020 07:00 PM Canberra, Melbourne, Sydney

Join Zoom Meeting
https://us02web.zoom.us/j/85617048741

Meeting ID: 856 1704 8741
One tap mobile
[phone],,85617048741# US (Houston)
[phone],,85617048741# US (New York)

Dial by your location
[phone] US (Houston)
[phone] US (New York)
[phone] US (San Jose)
[phone] US (Tacoma)
[phone] US (Germantown)
[phone] US (Chicago)
Meeting ID: 856 1704 8741
Find your local number: https://us02web.zoom.us/u/kfJyWYds7

[Shazwazza]

https://us02web.zoom.us/j/88418227838?pwd=SFZjOFQxRE0vN3NLM1djWVhQcWM2UT09

[Shazwazza]

https://us02web.zoom.us/j/82035419325?pwd=T3JXL1hXWC9DZXNFSW5kRm14WWJtQT09

[Shazwazza]

Thanks!!! community members for joining and helping move this along, that was fantastic, much appreciated for your time ❤️

Here's the raw notes (will type them up later in a nicer format)

---

Umbraco netcore + MSDI

• We know we want to:

  • Remove LightInject
  • Simplify
  • Cleanup
  • Use MSDI!

• We know we need to:

  • Keep the same concepts as what Umbraco currently has: IComposer, IComponent, plugins, Collection Builder, "Unique" registrations
    
    
    
    • Some of these things are 'interesting' because they don't work 1:1 with how MSDI container works
      
      
      
      
      
      • IFileSystem - named service? It's using Activator/reflection? It's possible to use MS's new 'Activator' (can't remember what it's called... ActivatorUtilities?)... can we use DI?
      
      
      
    
    
    • ASPNETCORE - has 'replace' method, could be used in place of "Unique"?
    
    

• Main questions
  1) Do we use MSDI abstractions for the DI abstraction layer
  * Use extension methods to keep our existing API methods for ease of upgrade/usage
  1) Do we continue to use our own DI abstractions and MSDI is the default shipped implementation

• Notes:

  • Have discussed internally and taking on MSDI dependencies in the "Core" project = OK
  • DI Abstractions package only in Core

• Current state...

  • Current
  • Startup
  • More...

• RFC

• Other

  • IFileSystem refactor - it is coupled too strongly to physical file system, its not async!
    
    
    
    • We would need a way to 'fetch' the file and not rely on static file handler - this allows for authorization
    
    

• Improvements:

  • Clear direction on what we're changing
  • Naming - use correct naming so that people know exactly what they are doing
  • Instead of relying on 'magic' like "RegisterUnique" would be like "SetAppCaches"
  • Provide really simple ways to set specific services
  • Current conforming container removed - everything is then extensions on IServiceContainer ... or IUmbracoBuilder type of thing
  • IComposer is really just for plugins
  • IUmbracoBuilder used for end user developers for adding/replacing
  • Current ... WHY? (This causes more problems than it's worth), Views - we need it here in v8 but in netcore we don't
  • IFileSystem - named service? It's using Activator/reflection? It's possible to use MS's new 'Activator' (can't remember what it's called... ActivatorUtilities?)... can we use DI? How do we really want
    
    
    
    • There doesn't need to be multiple IFileSystem ... there is just a single IFileSystem which is injected into Css, Script, Media 'providers', if people want to replace these, then they use factory methods for these service
    
    
    • ShadowFileSystem ... how does this affect what we want ... could be done with DI Interceptors, but that's not in MSDI
    
    

• How?

  • will be done in a different feature branch
  • IComposer is simplified, it's really just a wrapper on IServiceContainer
  • Umbraco cannot rely on any 'special' container features, it needs to work with MSDI's default implementation
  • what type of phases could/should done to get to where we are?
    
    
    
    • Could start by renaming methods on IRegister to equivalent method names on IServiceContainer, that would allow transitioning in an easier way
    
    
    • POC - a first go to just see what happens, and list questions/concerns, etc... - this can be a PR for comments - whether it gets merged doesn't matter but it is a first.
    
    
  • Get rid of Current, is it easy/doable? At the end of this process, let's check!

[lars-erik]

Indeed that was awesome! We were way more aligned than I expected, and that makes me super happy. :)

Just thought I'd elaborate on the steps I suggest for renaming the IRegister methods.

Most of the methods and extensions on IRegister boil down to the concrete method Register(Type serviceType, Type implementingType, Lifetime lifetime). (https://github.com/umbraco/Umbraco-CMS/blob/edca4af7f43eb586ba91810729569f7a9a3fcce7/src/Umbraco.Infrastructure/Composing/LightInject/LightInjectContainer.cs#L158)
That method pretty much mirrors a private extension method in MSDI:
https://github.com/dotnet/runtime/blob/1789775d0722bd2344fc026e9111d2a49adb0da0/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ServiceCollectionServiceExtensions.cs#L650

So I think a good way forward would be to create a new static class a'la TempMsdiExtensions and move the LightInjectContainer.Register method to an extension method with the same signature, except IRegister as the instance argument. It can well be renamed to Add to match the private MSDI one. After that the remaining Register methods on IRegister should be possible to move into extension methods all pointing to that initial Add method, and mirroring the respective extensions in ServiceCollectionServiceExtensions. The final step being just replacing the using directive around Umbraco with the MSDI abstractions namespace instead.

While glancing over that interface now, I'm also reminded about the RegisterFor<> concept we introduced. It lets you register unique dependencies for given services. I think it amongst other things was indended for fixing IFileSystem. You could say .RegisterFor<IFileSystem, IMediaService>(typeof(StorageFileSystem)). 👼
However RegisterFor, like "uniques", do some magic on the container build step. AFAIK. there's nothing supporting that in MSDI, so we need to mind those as well.

Looking forward to see what @rustybox comes up with! 🤘

[JimBobSquarePants]

I created services.AddServiceFor in my NamedServices experiments years ago. As far as I recall it "Just worked :tm:"

The signature looks like this.

c# public static IServiceCollection AddServiceFor<TService, TImplementation, TTarget>( this IServiceCollection serviceCollection, ServiceLifetime serviceLifetime, ServiceLifetime targetLifetime) where TImplementation : class where TTarget : class

[Shazwazza]

IMO if IFileSystem gets in the way of the initial POC I would suggest ignoring RegisterFor and how it is currently put together and we can come up with a simpler approach like we discussed instead of trying to shoe-horn in something that doesn't feel quite right.

[rustybox]

RuntimeTests.Boot_Core_Runtime passed -- Was a bit concerned I might have lost my mind before I saw that message.

[lars-erik]

@Shazwazza

IMO if IFileSystem gets in the way of the initial POC I would suggest ignoring RegisterFor and how it is currently put together and we can come up with a simpler approach like we discussed instead of trying to shoe-horn in something that doesn't feel quite right.

I think RegisterFor is pretty much what we talked about. I think usage for filesystems were left as an intention. ;)
Both issues has to be addressed never the less, but I totally agree it can be postponed for now.

@rustybox

RuntimeTests.Boot_Core_Runtime passed -- Was a bit concerned I might have lost my mind before I saw that message.

🤪🤘👍 (GJ)

[rustybox]

Any ideas why I might get the following during Setup in
Umbraco.Core.Persistence.DbConnectionExtensions.IsAvailable for all subclasses of UmbracoIntegrationTest

"Connection Timeout Expired. The timeout period elapsed during the post-login phase. The connection could have timed out while waiting for server to complete the login process and respond; Or it could have timed out while attempting to create multiple active connections. The duration spent while attempting to connect to this server was - [Pre-Login] initialization=1; handshake=0; [Login] initialization=0; authentication=0; [Post-Login] complete=14046;

Edit: The good news (if you can call it that) is that I get the same exception with head @ 775360e which is odd as they ran fine before I made any changes( with the exception of 5 tests that failed)

[JimBobSquarePants]

Triggered by this conversation I had a look at the composition docs.

It's all so complicated!

All those attributes and separate classes are too much. There's so much scope for simplification here it's incredible.

Here's how I would attack composition.

Rather than the various [DisableComposer] & [Disable] attributes and myriad of inherited composers there should be a single registration pipeline where individuals can choose what items to register items based upon the hosting environment.

A simplified pipeline would be this:

• IUmbracoBuilder should use the type finder to get all instances of IComposer from all assemblies.
• Once all instances have been found. on UmbracoBuilder.Build() (_note Build() is specific to the implementation and can never be called via the interface_) would simply create and loop through each found instance calling a single ConfigureServices(IUmbracoBuilder builder) method. They should be stateless and have parameterless constructors.

There is no reason I can see to have individual composer types.

```c#
public interface IComposer
{
void ConfigureServices(IUmbracoBuilder builder);
}

`IComposer` should not manage lifetime events, nor should it have concrete implementations of events passed to it. For example, if you want to register an event to be triggered by the content service on save, you should register an implementation of an interface that would be added to a collection and injected into the concrete `IContentService` implementation.

Here's a simplified example of the pattern at the user level. 

```c#
public class MyComposer : IComposer
{
    public void ConfigureServices(IUmbracoBuilder builder)
    {
        builder.AddOnSaveContentEvent<MyOnContentSaveEventHandler>();
    }
}

public class MyOnContentSaveEventHandler : IEventHandler<OnContentSaveContext>
{
    public void Handle(OnContentSaveContext context)
    {
        throw new NotImplementedException();
    }
}

And at the service level. My personal preference would be to use Mediatr to handle events but since composition is in the core library (_perhaps it shouldn't be_) this voilates the no dependency rule..

``` c#
public interface IContentService
{
public IEnumerable> OnSaveHandlers { get; }

public void OnSave();

}

public class ContentService : IContentService
{
// Obviously other services would be injected here.
public ContentService(IEnumerable> onSave)
{
this.OnSaveHandlers = onSave;
}

public IEnumerable<IEventHandler<OnContentSaveContext>> OnSaveHandlers { get; }

public void OnSave()
{
            // Create and populate with relevant info.
    var context = new OnContentSaveContext();
    foreach (var handler in this.OnSaveHandlers)
    {
        handler.Handle(context);
    }
}

}

public class OnContentSaveContext
{
// relevant info.
}

public interface IEventHandler
{
void Handle(T context);
}
```

@lars-erik I know you have raised something similar to this before. I'd love your thoughts.

[rustybox]

@Shazwazza what happened to netcore/bugfix/integration-tests-examine-indexes, seems to be gone and not merged, do I need to rebase onto netcore/dev?

I have integration tests running again with no code changes, no idea what was going on, SQL Server profiler showed no signs of issues.

I think I'm good to keep hacking.

At present I have Umbraco.Tests.Integration 300 passing 5 failing which is the same as when I started.

[Shazwazza]

@rustybox that's been merged now into netcore/netcore branch so you can rebase onto that one and that will be the branch to keep in sync with your own work. Glad the SQL Server mystery is working :)

[rustybox]

I wonder at this point, what it would take for an initial merge? keeping these branches in sync will be a nightmare.

[rustybox]

What needs to be the strategy for keeping tests in the .net 472 projects passing?

Does MSDI need to be backported into the Umbraco.Web project?
Does Umbraco.Core need to work with both IFactory and IServiceProvider?
Is it appropriate to remove the failing tests in Umbraco.Tests?
Can Umbraco.Web be removed from the sln along with Umbraco.Tests?

[JimBobSquarePants]

The unit tests shouldn’t be targeting NET Framework

[rustybox]

@JimBobSquarePants what do you mean?

Umbraco.Tests is the legacy test project that covers Umbraco.Web these are both .net framework projects, then there are some new test projects that cover the netstandard and netcoreapp projects.

[JimBobSquarePants]

I mean, the test projects should be targeting NET Core only. Having test code that targets NET 472 is blocking at best.

Building and testing should be incremental otherwise you end up with an incomplete, hard to refactor hybrid (which is exactly the place the code base is in).

Umbraco.Tests should be dropped from the solution. Any tests relevant to each src project should be added to an Umbraco.[PROJECT_NAME].Tests project.

The fact that there isn't a dedicated test project per src project is utterly bonkers. If things are to be treated as separate concerns by isolating them in separate projects then the tests should be separate concerns also.

[rustybox]

As much as I agree with what you're saying, I can't imagine me dropping Umbraco.Web from the solution will get this initial refactor merged.

Presumably there is method in the madness and Umbraco.Web is hanging around & being built for some reason? @bergmania @Shazwazza?

[lars-erik]

If the problem is that Umbraco.Web and Umbraco.Tests currently don't have a reference to MSDI (IServiceCollection etc.), then I suggest just giving it to them. Microsoft.Extensions.DependencyInjection.Abstractions is (also) built for framework 4.6.2.
Otherwise, I'm not sure I get the problem?

[rustybox]

Where I'm at,

Umbraco.Tests.Integration all pass (except that one about naming conflicts that's documented for failing upstream),
Umbraco.Tests.UnitTests all pass

LightInject is gone, IFactory is gone, IRegister is gone the world is a better place, not perfect as there's still lots to do like handling the lack of named registrations for IFileSystem etc.

I'd say we're potentially in a reasonable place to merge so that there isn't a constant need to rebase / merge upstream branches but there is pushback in the pull request as the changes completely break Umbraco.Web and make the legacy tests fail.

I'm not sure of the benefit of backporting changes to the legacy projects when they are legacy, Umbraco.Web will not be released for Umbraco @ 9

It's not so much adding MSDI dll's it would also require removing all usages of IRegister, IFactory, and repeating the work that was done for the core projects also in the .Net framework projects.

What would be the purpose of this, it wouldn’t help merging over changes from v8 as there would be constant conflicts and you’d still have to copy changes in Umbraco.Web to Umbraco.Web.BackOffice & Umbraco.Web.UI.Netcore.

In short..

Why does Umbraco.Web exist in the netcore branches, why is it being built, why are the tests running against it?

[lars-erik]

LightInject is gone, IFactory is gone, IRegister is gone the world is a better place but not perfect there's still lots to do like handling the lack of named registrations for IFileSystem etc

Wow! 😁👍

If the .Web and .Test projects are there just for copying then I say drop them from the sln.

[JimBobSquarePants]

As much as I agree with what you're saying, I can't imagine me dropping Umbraco.Web from the solution will get this initial refactor merged.

They have to drop it sometime. I can't see it being refactored in a single, consumable, PR.

[lars-erik]

@JimBobSquarePants Reg. https://github.com/umbraco/Umbraco-CMS/issues/8653#issuecomment-688376166

I think the IComposer attributes that lets us add things before or after other composers have ran is useful with regards to customizing packages. I guess the "big problem" is that Umbraco is still supposed to be able to run with "no-code" setups while still using packages. This means we can't rely on packages being added by "services.UsePackageXyz()" from Startup.
So if they trigger their setup through some autodiscovery mechanism, I want to be able to replace configuration done by default.
I don't mind how, as long as I can. Make sense?

Reg. events, I'm all for some sort of domain event mechanism. 👍
Here's a trusty old article on a basic setup:
https://udidahan.com/2009/06/14/domain-events-salvation/

[rustybox]

I LOVE MediatR, but I'd be scared to include it in Umbraco, features are not always consistent across containers so people could do some awesome stuff that will break builds elsewhere

https://github.com/jbogard/MediatR/wiki/Container-Feature-Support

[lars-erik]

@JimBobSquarePants Continuing reg. https://github.com/umbraco/Umbraco-CMS/issues/8653#issuecomment-688376166

It boils down to IComponent, not IComposer. Say I package up a health check for usync or something. I need my IComponent.Initialize to run after usync's IComponent.Initialize. Today I'd do that with [ComposeAfter] (I think).
I guess components could be changed into a collection builder though. Since registering and composing are now two distinct phases, all components could just be registered in an ordered list and we could InsertBefore, Remove etc.

/co @rustybox - reg. MediatR

I don't think we need a dependency like MediatR for "simple" delegation. See the article I posted - it's fairly simple to provide the hooks for peeps to add mediatr or similar things themselves. Could be worth looking into how Function Monkey does it wrt. different command mediating frameworks.

[JimBobSquarePants]

I think the IComposer attributes that lets us add things before or after other composers have ran is useful with regards to customizing packages.

@lars-erik If you simplify the process to a single pipeline triggered only by Build() at the end and allow methods to insert collection items at a given index then order is preserved. Nothing should be initialized other than by the service provider when creating any items for injection anyway. If you're introducing some sort of temporal coupling then you're on the road to hard to diagnose runtime issues. I hate to think of the cost of running through all the type attribute via reflection at startup each time!

This means we can't rely on packages being added by "services.UsePackageXyz()" from Startup.

The packages will all have to have a service registration component if they're touching services. If we use the type finder to enumerate the assemblies then you have each IComposer instance which can be initialized and the ConfigureServices called. There would be no need to manually register each package.

1. UmbracoBuilder registers the default services with IServiceCollection
2. Within UmbracoBuilder.Build() TypeFinder enumerates assemblies for IComposer
3. UmbracoBuilder then enumerates the result creating an instance of each type and running ConfigureServices on the instance passing itself as the parameter. Extensions methods on IUmbracoBuilder allowing instances to replace registered services.
4. Finally in Startup.cs the UmbracoBuilder.Build() method performs any final reordering and registration.

UmbracoBuilder.Build exists for lazy invocation which allows developers to use Startup.cs if they choose to.

One pipeline, one container... Lovely jubbly.

It boils down to IComponent, not IComposer. Say I package up a health check for usync or something. I need my IComponent.Initialize to run after usync's IComponent.Initialize. Today I'd do that with [ComposeAfter] (I think).
I guess components could be changed into a collection builder though. Since registering and composing are now two distinct phases, all components could just be registered in an ordered list and we could InsertBefore, Remove etc.

Yep, collections are a more predictable and testable methodology here. I think, tbh, we could drop IComponent or repurpose it's use for injecting elements for calling during the app.UseUmbraco() stage of StartUp.Configure

@rustybox Yeah Mediatr the weakest performing container there is MSDI which, since we're targeting the lowest common denominator via abstractions anyway, shouldn't be an issue. Like Lars says though it's an easy enough pattern to replicate (though I wouldn't allow people to plug and play their own). Imagine how nice we could make event handling!

[rustybox]

CI build pipeline relies on the legacy projects existing :(

[JimBobSquarePants]

Those pipelines should be configured in the repo via yaml.

[Shazwazza]

Hi All! sorry busy week last week for me but will try to keep up more this week. (sorry for the long message below but just trying to respond to everything)

Great works so far!! The event handling you describe would be freaking awesome and something we've chatted about for some time now but I think we need to try to focus on one thing at a time for our own sanity. Events not being static would have to start first :P There's soooo many things in this code base we'd all like to fixup :) ... we will get there but lets get MSDI handled first and iterate.

I'm not sure of the benefit of backporting changes to the legacy projects when they are legacy, Umbraco.Web will not be released for Umbraco @ 9. It's not so much adding MSDI dll's it would also require removing all usages of IRegister, IFactory, and repeating the work that was done for the core projects also in the .Net framework projects. hy does Umbraco.Web exist in the netcore branches, why is it being built, why are the tests running against it?

We'll need to make a feature branch for this work. We can't just merge into netcore/netcore as it's a work in progress and we'll need to review changes hopefully in iterations as we go. I've just pushed a branch: netcore/feature/msdi-refactor if you can target a PR to that branch? Then we can have a look, review, etc... merge and keep going until we're happy with the feature. We can continually merge netcore/netcore -> netcore/feature/msdi-refactor to keep everything in sync. We are still in the process of migrating tons of things. We've got the back office 'working' but there's still the whole front-end rendering part to go. That also comes with migrating all of the tests to netcore. All of that is going to take some time.

I agree, the effort wouldn't make sense to update all of the existing legacy code and projects as it will be migrated/removed. Perhaps it makes more sense to remove Umbraco.Web project from the sln build configurations in this feature branch for now. Possibly even adding a custom custom compilation symbol to the legacy Umbraco.Tests like MSDI_REFACTOR and use that to exclude code from building where needed like #if !MSDI_REFACTOR

The packages will all have to have a service registration component if they're touching services. If we use the type finder to enumerate the assemblies then you have each IComposer instance which can be initialized and the ConfigureServices called. There would be no need to manually register each package.

100%, that's pretty much what happens today how this looks later after the refactor will most likely be a little different but the idea is pretty much the same. Current IComposer is like ConfigureServices (plus being able to modify collection builders) and IComponent is like Configure but for package. Ordering is important which is why we have things like ICoreComposer, IComposer, IUserComposer which affects the order that IComponent is executed (i.e. Core ones will generally need to come first). But with IUmbracoBuilder perhaps the idea of Umbraco itself using composer/components might not make sense and these end up only being used by packages - though maybe that refactor if we want to do that can come later once the primary goals are reached.

Say I package up a health check for usync or something. I need my IComponent.Initialize to run after usync's IComponent.Initialize. Today I'd do that with [ComposeAfter] (I think).

This part and the type scanning for IComposer is currently all done in the CoreRuntime. It would be possible to write a cache file of ordered resulting list instead of reflection checks on the resolve IComposer instances if we determined how much a bottleneck that is (would be pretty easy). That said, I'm all for simplifying this instead of any attributes if possible but what will make things interesting is how one package can manipulate the collection because there's no way of knowing if your IComposer is going to run before or after another one. So to make that possible? Another method, event or something else that a package author can subscribe to?

[Shazwazza]

Btw, this first round effort is super impressive! The changes so far are actually very minimal and this has already happened 🎉

Can't wait to see how this progresses :)

[rustybox]

~1900 tests fail in Umbraco.Tests without working implementations of IRegister / IFactory. That would be an awful lot of preprocessor directives.

Still not following why 472 tests are running on this pipeline, surely they're only needed on v8 pipeline?

PR is now targeting upstream msdi-refactor

[Shazwazza]

~1900 tests fail in Umbraco.Tests without working implementations of IRegister / IFactory. That would be an awful lot of preprocessor directives.

sorry i didn't see that part, well then I'd suggest removing Umbraco.Web and Umbraco.Tests from the sln build for this branch for now.

Still not following why 472 tests are running on this pipeline, surely they're only needed on v8 pipeline?

Because we haven't migrated everything from v8 to netcore yet, we're not even half way there yet, so what is there needs to continue to work and pass else we'll end up in a world of hurt not knowing what is still working or not. There's also a large amount of tests that need to be migrated to the netcore tests which just haven't been done yet.

[rustybox]

Removing projects entirely causes a build failure at "Set Umbraco Version"
Removing projects from sln causes build failure at "Build Tests" (This is how it is in branch at present)

I think the pipeline will require an edit

[Shazwazza]

😩 argh, yes the build pipeline is another one of those tasks...

Can you try to remove them from the sln configuration like this?

[lars-erik]

This is why I recommended renaming all methods and extensions on IRegister and IFactory first, so the remaining step would be to replace using statements. I might be too optimistic, but I believe that would have prevented this problem.
https://github.com/umbraco/Umbraco-CMS/issues/8653#issuecomment-686412483

Sorry it wasn't communicated clearer. Dunno if it's still a viable path or we should just proceed with the removed dependencies.
Gonna be a huge merge in any case.

KUTGW!

[rustybox]

That was indeed the strategy I went for, had an adapter for IServiceProvider -> IFactory and a bunch of extensions such that IServiceCollection could be used like IRegister then started to remove it all in a later commit, however I did most of this work with the net 472 projects unloaded.

I didn’t know the Umbraco.Web project still needed to work.

[lars-erik]

however I did most of this work with the net 472 projects unloaded.

Aww. 😆 So close, yet so far. That would've done all the refactoring for you.
Dunno how much work it is to "replay" the commits?

[rustybox]

I could bring the adapter and the extensions back, I think I’d Introduce them to Umbraco.Web though rather than keeping them in core like they were before.

There’s still a bunch of refactoring to do though, either bringing back LightInject to Umbraco.Web or switching it to ServiceProvider.

I still don’t really get the whole concept, Umbraco.Web works in dev/v8 why does it need to work in netcore/* alongside its replacements.

[rustybox]

@Shazwazza disabling builds in solution config doesn't stop build pipeline from trying to run them.

[Shazwazza]

I still don’t really get the whole concept, Umbraco.Web works in dev/v8 why does it need to work in netcore/* alongside its replacements.

Because we are refactoring and changing things along the way, everything must build and pass, we can't have a solution that is broken while we develop and migrate because then we don't know what is broken on purpose or broken by accident without having to do a bunch of investigation. Umbraco.Web and Umbraco.Tests in dev/v8 is different from what is in netcore/netcore while this migration takes place. In the case of this feature branch, we could get away with knowing that Umbraco.Web and Umbraco.Tests is broken or not included in the build pipeline so long as netcore/netcore is still fine.

disabling builds in solution config doesn't stop build pipeline from trying to run them.

Boo. Well i guess our options are to bring the adapter/extensions back and see if we can get the whole thing working again, or try to modify the build pipeline to work without these building. The latter might be easier but might need to wait for a bit since I'm pretty swamped this week. Is another ugly option to just exclude most of the files from these 2 csproj?

[rustybox]

I did some stuff, all checks have passed

[Shazwazza]

@rustybox amazing stuff! I'll try to find some time tomorrow

[rustybox]

I still have a little downtime before I have to prioritise paying the bills. If this is going in the right direction I can sink some time into further enhancements, could do with some feedback on current state and next steps

[Shazwazza]

Yep hoping to get some time early next week :)

[Shazwazza]

Just picking this up, will have a look at code, etc... and report back

[rustybox]

1 down n to go!

[rustybox]

TODO:

• [x] Composition should depend on IServiceCollection instead of IRegister
• [x] Anywhere that returns a IFactory or has an IFactory out param should be void or return IServiceCollection for fluent chaining
• [x] Remove LightInject references entirely (netcoreapp/netstandard)
• [x] Remove LightInject references entirely Umbraco.Web
• [x] Kill IFactory
• [x] Kill IRegister
• [x] Probably bring back the helpers for setting MediaFileSystem with a different underlying IFileSystem (whilst maintaining shadow wrapper setup)
• [x] Fix comments around replacing underlying media file system
• [x] Kill Current, there's barely anything left using it now
• [x] Try to split CoreRuntime into separate Config / Run classes, CreateDatabaseFactory needs thought.
• [x] Try to reduce calls to BuildServiceProvider (ideally 0 let ASP.Net do it for us, but perhaps 1 in worst case scenario, it's currently 6)
• [x] CoreRuntimeBootstrapper should construct bootstrap dependencies like DatabaseFactory and LoggerFactory rather than resolving from container.
• [x] Ensure all TODO: MSDI .* are gone (1 remains to cover ValidateOnBuild)
• [x] Turn ServiceProvider ValidateOnBuild back on

[rustybox]

Turn ServiceProvider ValidateOnBuild back on

This is going to be interesting.

IIndexDiagnosticsFactory is registered by ExamineLuceneComposer (required by ExaminManagementController), but only if RuntimeLevel >= Run, so on first boot it wouldn't be registered and therefore container can't validate.

What would one do here, register a NOOP IIndexDiagnosticsFactory for RuntimeLevel Unknown/Boot and replace it in the composer when Level is Run?

[JimBobSquarePants]

Kill those runtime levels.

[rustybox]

I guess I explained badly, there's 3 real stages i suppose, Install, Update, Run.

Those dependencies would only be registered when it's Run.

I'm intrigued how many noop uniques would be required so gonna have a quick play there.

--

Alternatively we need it such that ExaminManagementController isn't registered until RuntimeLevel == run either.

Edit: solve for ExaminManagementController, then LogViewerController then ModelsBuilderDashboardController....

There are 18 CoreComposers with a MinLevel of Run so presumably all of those will register something that would need a Noop replacement when < Run

Edit 2: Only 7 of these add services that matter

[bergmania]

I guess 99% of the controller is only intended to run on Level==Run.

The following is expected to run on Level < Run

• InstallController
• InstallApiController
• BackOfficeController (Due to AuthorizeUpgrade action)

[lars-erik]

Without going back and looking at the code, this sounds and smells a lot like "Replace typecode with polymorphism" would do the trick. Make three concrete composition strategies that are instantiated based on whatever logic would've set the runtimelevel field.
Hope I'm clear enough, otherwise ask. 👼

[rustybox]

@bergmania so would you suggest ensuring controllers are ~kept out of the ServiceCollection~ not discovered until they are required.

@lars-erik I'm not following 😕

Assuming we're using DefaultAssemblyProvider and not using AddControllersAsServices, I believe this means we need a custom implementation of ~DefaultControllerTypeProvider~ ControllerFeatureProvider

[lars-erik]

@rustybox

https://www.refactoring.com/catalog/replaceTypeCodeWithPolymorphism.html

Instead of in several places doing:

if (runtimeLevel < RuntimeLevel.First) {
    composition.Register(xyz);
    composition.Register(zyx);
} else if (runttimeLevel < RuntimeLevel.Second) {
    composition.Register(xyz);
    composition.Register(abc);
    composition.Register(cdf);
    composition.Register(zyx);
}

Make it in one place:

CompositionStrategy strategy;
if (runtimeLevel < RuntimeLevel.First) {
    strategy = new InstallationComposition();
}
else if (...) // ... is there more?
else {
    strategy = new RuntimeComposition();
}
strategy.Compose();

Make sense?

[rustybox]

@lars-erik That's already what happens with the attributes, the issue is the controllers which have dependencies on services which are conditionally registered

Unless you are suggesting that yes we should register Noop dependencies?

These are the two strategies I can come up with, open to further suggestions.

1) Have Noop implementations which are registered when level < run and real when level >= run
2) Change controller discovery so those only used when level is run are discovered

I'm leaning towards the latter as that prevents creating a metric fuck ton of Noop implementations.

[lars-erik]

"Attributes are type codes too" 😆

By building strategies for DI, you should be able to make strategies that only registers controllers with the dependencies that are registered for that level.

I'm not sure auto discovery is a good idea for the two lower levels than "Run". Which I guess is your pt. 2 that were written while i wrote this. 😆

So autodiscovery should only be done by the "Strategy" (WebRuntime?) that runs when we are in the "Run" state.
Core- and WebRuntime are strategies (they're the ones orchestrating composition, right?).
Maybe we're just missing an InstallationRuntime class?

@JimBobSquarePants would you oppose this type of "runtimes" or did you complain about the typecode in conditionals? :P

(Apologies for not looking at the code, working from memory. Please forgive any wrong names.)

[rustybox]

At present controllers ~aren't registered~ (are but shouldn't be) in ServiceCollection, this can be turned on with mvcbuilder AddControllersAsServices, (i'm learning as I go along, didn't know anything about how controller discovery worked in .net core MVC until 10 minutes ago).

[lars-erik]

Sounds good. Then I'd leave that out for install/update, but manually register the few controllers and dependencies needed for those. Am I missing anything?

[rustybox]

Nope that sounds like the right approach to me too.

[rustybox]

Right further confusion

By default controllers shouldn't be registered in ServiceCollection as DefaultControllerActivator will instantiate the controllers without resolving them (it just resolves the dependencies).

Umbraco.Web.Website.UmbracoWebstiteServiceCollectionExtensions adds ServiceBasedControllerActivator (which could be done instead by using the AddControllersAsServices extension on mvcbuilder) which does resolve controllers from container.

Does anyone know the reason why DefaultControllerActivator was replaced? Is this legacy hangovers or would it be required going forwards.

It tends to be replaced to enable property injection on controllers and whatnot.

~If DefaultControllerActivator can indeed be used, any ideas where TreeControllers are registered, I can get the controller count down from 100 to 21 in ServiceCollection but I haven't spotted how the tree controllers make their way in.~

TreeControllers are registered by SearchableTreeCollectionBuilder (CollectionBuilderBase).

Not sure if it matters that they make their way in yet.

They are registered as self rather than as ISearchableTree at the moment which could be problematic as MSDI tends to want you to be very explicit, but this doesn't appear to be a problem just yet😕

If it becomes a problem may need another CollectionBuilderBase whose RegisterTypes is more explicit.

[lars-erik]

DefaultControllerActivator have to be replaced on .NET Framework. I guess .NET Core does it better.

[rustybox]

So possibly legacy hangover from 8, will see if I can get things running with DefaultControllerActivator :D

[JimBobSquarePants]

I'm still voting kill. Registering an object isn't the same as creating an instance of an object. I've never seen a codebase attempt to split up DI like this and there's a good reason why.

[rustybox]

@JimBobSquarePants Kill what though?

[JimBobSquarePants]

RuntimeLevel.

[rustybox]

Is that possible, this isn't just composition it's components too.

Components that get registered by compositions are initialized when CoreRuntime starts, some of these could break if there's no database etc.

[JimBobSquarePants]

That sounds like temporal coupling to me + some tight coupling that shouldn't be.

DI should provide assurance that an injected dependency is stable. If something breaks because something else doesn't exists then it's likely that it is referencing something it shouldn't.

[rustybox]

So perhaps the RuntimeLevel stuff needs to move away from dependency registration and over to the Components.

Register all components but only call Initialize on those it makes sense to initialize.

[lars-erik]

I agree with the latter and @JimBobSquarePants's notion that everything should be registered.
I still think the "non-run" levels should specify centrally and concretely which components are supposed to be executed instead of using a typecode / discovery to run them. I think in this case it would be OK to use the container as a factory (service locator). Which means we could possibly remove the runtimelevel markers (attributes, interfaces, whatever).

IE.

if (isInstallingOrUpgrading) {
    factory.GetService<InstallationComponent1>().Initialize();
    factory.GetService<InstallationComponent2>().Initialize();
} else {
    factory.GetServices<IComponent>().ForEach(x => x.Initialize());
}

Again, even better if each block is a strategy class instead of "inline" code.

factory.GetService<Func<RuntimeLevel, IComponentInitializer>>()(runtimeLevel).Initialize()

This would support registering the installer strategy with concrete ctor params per component supposed to be ran.

public class InstallerInitializer : IComponentInitializer
{
    public InstallerInitializer(Component1 init1, Component2 init2) ...
}

public class RuntimeInitialiizer : IComponentInitializer
{
    public RuntimeInitializer(IEnumerable<IComponent> all) ...
}

In any case, good discovery that we're talking about "components" and not registered types. 👍

On a sidenote, it bothers me is that in general DI and pattern language, everything is a component.
Wish we had another name for the ones we call components in Umbraco.
Initializer? Package? Module?
Naming's hard. 😆

[JimBobSquarePants]

Yep, but something should never be "initialized" outwith the service provider. Construction, setup, and disposal of objects should be cheap.

IComponent in this manner simply shouldn't exist. The Initialize method introduces temporal coupling. e.g I cannot create an instance of a type implementing this interface and be assured it is in a ready state.

Anything that requires cleanup should implement IDisposable.

```c#
public interface IComponent
{
///

/// Initializes the component.
///

void Initialize();

    /// <summary>
    /// Terminates the component.
    /// </summary>
    void Terminate();
}

```

[rustybox]

I agree with the latter and @JimBobSquarePants's notion that everything should be registered.
I still think the "non-run" levels should specify centrally and concretely which components are supposed to be executed instead of using a typecode / discovery to run them. I think in this case it would be OK to use the container as a factory (service locator). Which means we could possibly remove the runtimelevel markers (attributes, interfaces, whatever).

My initial thought was just have RuntimeLevel as required prop on the IComponent Interface

All components are injected into the "new" slim CoreRuntime,

CoreRuntime can then just filter the collection to only initialize if prop matches runtime state

[rustybox]

Yep, but something should never be "initialized" outwith the service provider. Construction, setup, and disposal of objects should be cheap.

They aren't, components are initialized by CoreRuntime when CoreRuntime.Start is called on application start.

However at present the trick is conditional registration of components rather than conditional running of components.

[lars-erik]

IComponent in this manner simply shouldn't exist. The Initialize method introduces temporal coupling. e.g I cannot create an instance of a type implementing this interface and be assured it is in a ready state.

@JimBobSquarePants
Why would you ever want to instantiate one?
What other approach do you recommend we use to allow package devs to run code at startup?
I agree about the principles, but think this is the exception. (packages)

@rustybox
I think Umbraco should be in charge and not leave it up to implementors to put the right attribute in.
IE. nobody else should be able to specify a "core component". Which means the attributes should die.
Several options, I currently lean towards the ctor injecton mentioned in previous post.

[rustybox]

Several options, I currently lean towards the ctor injecton mentioned in previous post.

This is already done by constructor injection, components are injected into CoreRuntime then initialzed, you can move it elsewhere but it's the same thing.

I think Umbraco should be in charge and not leave it up to implementors to put the right attribute in.

In my current vision there are no attributes, only properties.

Are all "user components" assumed to be run never install?

To aid discussion here is less current implementation of CoreRuntime (with a bit of creative editing to keep it less verbose)

    public class CoreRuntime : IRuntime
    {
        public IRuntimeState State { get; }

        private readonly ComponentCollection _components;

        public CoreRuntime(
            IRuntimeState state,
            ComponentCollection components)
        {
            State = state;
            _components = components;
        }


        public void Start()
        {
            if (State.Level <= RuntimeLevel.BootFailed)
                throw new InvalidOperationException($"Cannot start the runtime if the runtime level is less than or equal to {RuntimeLevel.BootFailed}");

            // create & initialize the components
            _components.Initialize();
        }

        public void Terminate()
        {
            _components?.Terminate();
        }
    }

CoreRuntime is resolved and started in the Configure Step of the Startup class

[lars-erik]

Are all "user components" assumed to be run never install?

That's my understanding. Which is why I think it's a good idea for Umbraco Core to specify a concrete list of "components" to execute in the off cases of installation and upgrading.

In your example above, I think _components.Initialize() should be replaced with instantiating a runtime based strategy for running components. Either a core Umbraco one (probably not even in DI) or something running all of them.

No need for either properties nor attributes, just two concise strategies for initialization.

[JimBobSquarePants]

I'll preface this all with "In a vacuum".

Let's say hypothetically that I was absolutely stuck for some reason and had to use IServiceScopeFactory to resolve an IComponent instance.

Anything resolved via the IServiceScope should have it's lifetime managed by the IServiceProvider. That's the golden rule of DI.

```c#
using (IServiceScope scope = serviceScopeFactory.CreateScope())
{

// I should be able to fire and forget this and not worry about the lifetime since disposing
// of the IServiceScope also cleans up any resolved types.
MyComponent component = scope.ServiceProvider.GetRequiredService<T>();

// BANG!!! I forgot to call Initialize.
component.DoSomething();

}

// BANG!!! I left component hanging. Maybe it'll get GC'd but what if it's wrapping something unmanaged, or leaves a connection open.
```

I should be able to resolve anything at any time outwith any artificial pipeline. IRuntime (which should implement IDisposable) is now trying to do the job of the container.

[rustybox]

is now trying to do the job of the container.

How so, you can't resolve anything from CoreRuntime.

[lars-erik]

@JimBobSquarePants
I'll argue that composing and initialization are the two places where we're allowed to cheat.
Having "components" do other things (DoSomething) than initialization and teardown would breach a whole lot of other principles than just temporal couplings. :)

Still curious what you suggest we do to support package initialization. (Other than IComponent.Initialize())
I agree that principally it's a temporal coupling, but maybe the name makes it sound worse than it is.
IComponent.Start()?

Otherwise totally agree that since those are singletons (?) they should be possible to tear down using Dispose instead.

[Edit] Have everyone lazily ensure their initializations? Same temporal coupling if something's not set up...

[rustybox]

In your example above, I think _components.Initialize() should be replaced with instantiating a runtime based strategy for running components. Either a core Umbraco one (probably not even in DI) or something running all of them.

Seems reasonable, it's just the same thing but centralizing the business rules for what's required when.

is there a decent way to differentiate a UserComponent vs a CoreComponent, do we need a IUserComponent interface and a ICoreComponent interface.

Then our "Run" strategy can initialize/start whatever all "IUserComponents"

[lars-erik]

is there a decent way to differentiate a UserComponent vs a CoreComponent

I would drop the concept since it's not needed for discovery if a concrete list for install is kept.

[lars-erik]

Just have to grab comment #100 😆🙌

[rustybox]

We can't have a concrete list of UserComponents though. How do they get "Initialized"

[lars-erik]

We can't have a concrete list of UserComponents though. How do they get "Initialized"

if (isInstallingOrUpgrading) {
    factory.GetService<InstallationComponent1>().Initialize();
    factory.GetService<InstallationComponent2>().Initialize();
} else {
    factory.GetServices<IComponent>().ForEach(x => x.Initialize());
}

IE. either the few needed for installation, or all of them.

[JimBobSquarePants]

Let's look at the IRuntime interface.

```c#
///

/// Defines the Umbraco runtime.
///

public interface IRuntime
{
///

/// Boots the runtime.
///

void Configure(IServiceCollection services);

/// <summary>
/// Gets the runtime state.
/// </summary>
IRuntimeState State { get; }

void Start(IServiceProvider serviceProvider);

/// <summary>
/// Terminates the runtime.
/// </summary>
void Terminate();

}
```

It's registered against the service provider yet manages a lifetime outside of the service provider scope!!!

[rustybox]

That's not the current IRuntime interface, where are you looking, not netcore/netcore

    public interface IRuntime
    {
        /// <summary>
        /// Gets the runtime state.
        /// </summary>
        IRuntimeState State { get; }

        void Start();

        /// <summary>
        /// Terminates the runtime.
        /// </summary>
        void Terminate();
    }

[rustybox]

I'm sold @lars-erik , seems reasonable.

[JimBobSquarePants]

That's not the IRuntime interface, where are you looking, not netcore/netcore

Apologies, my fork wasn't up to date. The same issue still stands - It's registered against the service provider yet doesn't allow the provider to manage it's lifetime.

```c#
///

/// Defines the Umbraco runtime.
///

public interface IRuntime
{
///

/// Gets the runtime state.
///

IRuntimeState State { get; }

void Start();

/// <summary>
/// Terminates the runtime.
/// </summary>
void Terminate();

}

``` c#
var runtime = app.ApplicationServices.GetRequiredService<IRuntime>();

...

// Start the runtime!
runtime.Start();

Honestly this is all square peg round hole stuff. The Umbraco architecture appears to be trying it's hardest not to use the tooling available.

[rustybox]

Honestly this is all square peg round hole stuff. The Umbraco architecture appears to be trying it's hardest not to use the tooling available.

Yes and we (I say we, I'm community not core) are trying to resolve that, one step at a time.

[lars-erik]

We still haven't solved @JimBobSquarePants's grief over the .Initialize() temporal coupling.
Mark Seemann suggests factories over here: https://stackoverflow.com/a/1945023/937791
Maybe we should introduce a "IPackageFactory" or "IModuleFactory" that does the setup in a ".Create()" method?
That would leave out the option of having the container tear them down again tho.
🤷‍♂️

(Just playing with the idea of renaming component / initialize, still the same algorithms within)

But maybe also this part of the discussion should be moved to another issue, since this issue is about introducing MSDI, and not necessarily rewrite all of the core abstractions. 😁

[Edit] The whole problem lies in that the startup of packages might need injected services. Which means we can't register singletons that initialize at registration time. This is why we have to delay a call to .Initialize() instead. Any suggestion to delay execution of singleton ctors would solve the whole conundrum. They could still have Dispose instead of Terminate.

[rustybox]

Haha, but I want to turn container validation back on it's one of my few and final TODO's, that means the conditional registrations need to go.

[JimBobSquarePants]

Yes and we (I say we, I'm community not core) are trying to resolve that, one step at a time.

And that is why we shouldn't encourage this kind of lifetime mismanagement. Initialize(), Startup(), Terminate() etc.

If there's an issue, let's not try to get creative and hack something that allows it to exist. Let's raise it as such and get it removed.

@lars-erik There simply shouldn't be a need for all this registration Create stuff. Packages, UserThingies etc should all should be updating the IServiceCollection via methods specified by an IUmbracoBuilder type. Any object resolution should then take place during runtime by implementations that require interfaces.

Wanna add a new property mapper? Add one to the property mapper collection.
Wanna register an angular view? Add it to some sort of view location resolver location list.

[rustybox]

That's not what we're talking about though, Components get registered, they aren't registering things, all registration either takes place directly in ConfigureServices, or via the Services prop on Composition.

Part of the problem with umbraco is Composition, Composer & Component are very similarly named.

Components hook onto events to index in examine (yes there should be fewer static events and more pub/sub)

Component is basically "Plugin" / Runtime event hooker upper.

[lars-erik]

should all should be updating the IServiceCollection via methods specified by an IUmbracoBuilder type.

Wouldn't that mean all package consumers would have to modify their startup?
I think a lot of what's done in .Initialize() today could be done lazily without having a concrete IComponent.Initialize. (Caching, verification of environment etc.)
There's also the !#¤%& static event delegates, but those could be attached to from composers. (Although to which instance?)
The latter could likely be done by a singleton in it's ctor.
But then there's stuff like "run examine rebuild" on startup, import usync changes, prune the ImageProcessor cache...
How will this be ran without a temporal coupling like component.Initialize()?

Keep in mind that not all implementors of Umbraco will be able to control their own startup pipeline. (Using the builder pattern)
They should still be able to install and use packages.

[JimBobSquarePants]

A plugin should be able to register things. The whole problem stems from the fact that objects have been segregated for no good reason. I covered a better registration system back in September

[rustybox]

A plugin _should_ be able to register things

And they can, that's composition/composer not component.

[lars-erik]

To get our terms right...

IComposition = IUmbracoBuilder in James' samples.
IComponent handles initialization and teardown. Could it be named IPlugin?

We want to be able to run code on startup and teardown without having marketing agencies have to learn how to use a startup class. (IE. learn asp.net core) I agree it hurts, but I'm 100% confident that's a HQ goal.

So the question remains. How does a package dev. make sure code that should run on app start/stop runs?
(For instance usync import)

IMHO, IComponent is "just" a command object with Execute and Undo. No temporal couplings there.
Again - maybe the problem is just in the naming?

[rustybox]

We want to be able to run code on startup and teardown without having marketing agencies have to learn how to use a startup class. (IE. learn asp.net core) I agree it hurts, but I'm 100% confident that's a HQ goal.

Having worked at several marketing agencies, I hate this, if you want to build an Umbraco site you have to know asp.net, if you don't you make a monster where all business logic exists in views.

Literally just inherited an V7 project like this.

[lars-erik]

I hate this

Can't blame you. I hate Covid as well... Not much to do, though. 😆

[rustybox]

Do components have to start in correct order?

usync after models builder after examine or whatever.

throw in mediatr, remove all static event handlers fire an UmbracoStarting INotification

go rollerskating

[JimBobSquarePants]

IMHO, IComponent is "just" a command object with Execute and Undo. No temporal couplings there.
Again - maybe the problem is just in the naming?

@lars-erik Naming is definitely an issue and leads to massive confusion.

Ok.... so let's say all IPlugin instances can run installation/uninstallation code. That's not temporally coupled no. OnInstall, OnUninstall would surely me more clear.

Do components have to start in correct order?

@rustybox You could ensure all "Core" components are registered first in your umbraco builder then allow registration of additional items. Usync would essentially be an IComponent, IPlugin, or whatever we name it.

[rustybox]

Do we need that though, I was being serious with the Mediatr thing, can't "plugins" just hook onto the UmbracoStarting notification.

If they don't have to run in order, this would be fine.

ComponentCollection.cs, they terminate in reverse order, no idea if this matters or not.

[lars-erik]

can't "plugins" just hook onto the UmbracoStarting notification

YES! It could be the start of the event system we talked about in September that followed the link @JimBobSquarePants just posted. Same goes for stop/teardown events. Brilliant! 😁👌
With that we can likely remove the registration of components and register them with an initialization pipeline.
That would give implementors the option of inserting after or before other "plugins".
Would still leave problems with whether the "dependency" is already registered in the pipeline tho.

I'm suddenly getting deja vu - did we agree on this before? 🤣

[rustybox]

If they need to run in order, maybe there should be multiple notifications to hook onto.

UmbracoStarting
FinishedDoingSomeStuffWithExamine
blah
UmbracoShuttingDown

[lars-erik]

An in-process event dispatcher open for registration by plugins sounds like a very good path forward.

[rustybox]

So can Umbraco take a dep on Mediatr rather than rolling its own.

[lars-erik]

It could, except I don't see the benefit when it's that little code. We are after all _removing_ dependencies here. ;)

https://github.com/umbraco/Umbraco-CMS/issues/8653#issuecomment-691022362

[rustybox]

I guess my argument would be, Mediatr works well, has good test coverage, why re-invent the wheel

[lars-erik]

Would you install a library to do Singleton<MyType>.Create(new MyType()) instead of just implementing the singleton pattern in MyType? (I know Singleton is frowned upon, but hope it gets the point across).

I think an event dispatcher is too small a "pattern" to use a library for, but that's me.

[rustybox]

It's not quite at the left-pad or is-odd levels of let's take a dependency.

But yeah it can be kept simple so I can understand your argument.

[JimBobSquarePants]

I would absolutely love Mediatr to be taken as a dependency. It would allow the simplification of so, so much.

[rustybox]

I 50% agree, but to play @lars-erik's role, so would an simple UmbracoEventAggregator that service locates for subscribers

Throwing it out there that I prefer EventAggregator to EventDispatcher as a name.

[rustybox]

@bergmania you ~removed~ moved this comment from Startup.cs in b5b49210

// TODO: We will need to decide on if we want to use the ServiceBasedControllerActivator to create our controllers
// or use the default IControllerActivator: DefaultControllerActivator (which doesn't directly use the container to resolve controllers)
// This will affect whether we need to explicitly register controllers in the container like we do today in v8.
// What we absolutely must do though is make sure we explicitly opt-in to using one or the other *always* for our controllers instead of
// relying on a global configuration set by a user since if a custom IControllerActivator is used for our own controllers we may not
// guarantee it will work. And then... is that even possible?

Did you guys come to a conclusion, I have a commit ready that switches to DefaultControllerActivator and removes all controller registrations

Edit: Oh it just moved into UmbracoBackOfficeServiceCollectionExtensions which has gone at some point.

[JimBobSquarePants]

Why are they using custom controller registration?

[rustybox]

Why are they using custom controller registration?

Legacy, to enable ctor injection for controllers in net framework (or at least so I'm told about 100 comments up that away ^^)

[bergmania]

@bergmania you ~removed~ moved this comment from Startup.cs in b5b4921
[...]

Wow you guys are writing a wall of text :)

We ended up registering controllers and deriving them from the container. I do not remember the golden reason. But I'm pretty sure we need to get controllers from containers in a few different situations, so even if they are not necessarily returned from the container on requests, they still need to be registered.

[rustybox]

They get found by typeloader for most cases, can't find them ever being resolved.

e.g.

var umbracoApiControllerTypes = composition.TypeLoader.GetUmbracoApiControllers().ToList();
 composition.WithCollectionBuilder<UmbracoApiControllerTypeCollectionBuilder>()
      .Add(umbracoApiControllerTypes);

^^ Same for SurfaceControllers and SurfaceControllerTypeCollection

There's also SearchableTreeCollectionBuilder which registers the TreeControllers

BackOffice works fine with DefaultControllerActivator (and all but the TreeControllers not registered) so if there's an issue it must be an Umbraco.Web thing

Trees get pulled from Tree collection rather than from container so nothing to do with that.

[lars-erik]

Mediatr

I won't die on the hill of no Mediatr. But I think we'd get far with the Domain Events class linked above. Maybe introduce Mediatr when more than that is needed and we can see the benefit clearly?

Controllers

As mentioned, I had to swap the controller activator for v8 when I did the previous DI rewrite. Prev. ASP.NET did not support injection without it. Sure it's fine now.

I guess implementors are more free to intercept controller creation etc. if Umbraco just leaves the asp.net infrastructure as default. So don't mind pulling the controllers out of DI if they're not needed.

The problems mentioned about the misc. controller registrations and magic (tree etc) have _one_ solution as I see it.
Make all controllers anemic delegators between asp.net and framework-independent use-case classes (commands/queries).
In many cases, a controller can be boiled down to a simple delegator/dispatcher (mediatr style).
Currently, there's too much logic coupled to the concept of asp.net controllers, which yields the questions about controllers in DI or not.

What's been done already in these PRs is awesome though. H5YR, @rustybox!
And awesome discussions all. 👍

[JimBobSquarePants]

What's been done already in these PRs is awesome though. H5YR, @rustybox!

Seconded this is fantastic so far! 👍

[rustybox]

Yeah I’m sold on bespoke EventAggregator until such a time as Mediatr is required, swapping some interfaces around in future a lot easier (find replace) than removing the static events in the first place.

As far as I can tell the tree stuff is not a problem as they are not resolved via container, they get added as a byproduct of inheriting collection builder base or whatever it’s called but really they just need to be in the collection builder collection which is registered they don’t need to be resolved as self.

That’s not to say that there isn’t a magic need to resolve controllers for some reason somewhere, I’m just not aware of it yet.

Edit: Cheers folks, pretty proud of the refactor so far, definitely starting to look better!

Edit2: also I don’t think replacing all the events needs to be in this issue, the groundwork can get into place so components can be always registered but not always “initialized”, that way container validation can be turned back on and the static events can go in a later effort (although it sounds fun might volunteer)

[rustybox]

Before I stop spamming for the night, another selling point for mediatr is the pipeline stuff, that could be used perhaps to init things in a certain order even as a plug-in dev.

[lars-erik]

Ghn, don't get me started. There's walls exponentially longer than this about interception, decoration and composition (not being) in the MSDI repo. Why should we need to bring in a "commanding framework" to deal with common DI issues? ;)

Oh well, I'll leave it at that. 🤷‍♂️

[lars-erik]

Throwing it out there that I prefer EventAggregator to EventDispatcher as a name.

Just re-read the whole thread and noticed I skimmed past this earlier. I associate aggregation with reduction. (Perhaps because of the eerily similar methods in LINQ and JavaScript.) I looked up the word, and I can see the connection. Not being a native english speaker, I might have the wrong deeper meaning about the word.
I wouldn't say the observer pattern, which is really what we're introducing, creates a "whole". It sort of is a holistic handling of individual events, but I see it more as orchestration than aggregation. Hence dispatcher and not aggregator.

On a sidenote, I challenged Jimmy at NDC on the naming of mediatr since I'm not quite confident associating it with the mediator pattern either. He shrugged, said I was partially right, but that it seemed good at the time. (Paraphrasing!)
IMO. mediating is done between a model and a view for instance (controllers), while notifying observers is dispatching. Writing this, I'm wondering why we're not notifying, but it might be better with dispatch due to in-proc/out-of-proc being unknown.

NOW I'll leave it. 🤓

[rustybox]

I've had lots of happiness with Lamar let's just force all Umbraco users to use that!! /s

EventAggregator is nice as top result (or 2nd) for any one new to the concept goes to the Martin Fowler EventAggregator article

prism article has nice graphics

DomainEvents is a bit too DDD speak (not that there's anything wrong with DDD)

EventDispatcher takes everyone off to php land.

🚌 🚌 UMBRACO BUS 🚌 🚌

[lars-erik]

If Martin Fowler says so it has to be so. I yield!

[rustybox]

I’m also tempted by

UmbracoEvents

[rustybox]

I figured it was about time I re-read the thread, some points of interest.

@shazwazza https://github.com/umbraco/Umbraco-CMS/issues/8653#issuecomment-677203944

What I'm trying to say is that one goal of this release is to reduce the friction for Umbraco developers to make the transition from Umbraco 8 to the Umbraco netcore version and as such we are trying to maintain (where possible) the public APIs and patterns that exist in Umbraco 8. For developers to interact with Umbraco's startup, container configuration, plugins, collection builders etc... we have our own IComposer/IComponent which we need to keep. If changes to these types of things are necessary that is fine but we want to minimize the changes where possible. So it's totally fine we make changes but the goal is to minimize the transition for Umbraco devs from v8 to the netcore version.

So IComposer should stay, ~Composition can probably die though, like @JimBobSquarePants said, UmbracoBuilder can scan for IComposers and run them (still making user of Composers.cs to sort such that it's possible to replace uniques later?).~ Composition presumably needs to stay also for altering the CollectionBuilders or would they move to UmbracoBuilder also.

We now have UmbracoBuilder and Composition/Composers.

Is the intent that all Umbraco Composers ~35 disappear and become extensions on UmbracoBuilder?

Are IComposers just hanging around just for package devs and end-users?

Is UmbracoBuilder for internal use for Umbraco setup that is always required and Composers are for people downstream?

How do the existing UmbracoBuilder extensions handle the replacement scenario, if I wanted to replace IRuntimeMinifier to swap out Smidge with blah how do I do that as a package dev, do I have to remove those services from the service collection in a Composer and ensure no one calls app.UseUmbracoRuntimeMinifcation during Startup.Configure?

---

IComponent, we just spent a bunch of time talking about killing and replacing with EventAggregator.
Is it too much of a migration pain for package devs to implement IEventHandler\,IEventHandler\, instead of IComponent.

Perhaps the StartUpStrategy idea using existing Components is less problematic for migration.

---

Edit: I spent some time watching the 2012 Codegarden Keynote (v5 death) & re-reading the thread (I missed or failed to parse a lot of sage wisdom previously).

I'm feel like StartUpStrategy might be the way to go, my only concern is that the RuntimeLevel attribute is documented on the Composing docs, and whilst Run is default if attr missing it has been available outside of core and may well be used in the wild.

[JimBobSquarePants]

I think we have to be honest here and say that this is a breaking change (_and absolutely should be_) but also a massive improvement for plugin developers. If we get this right (_and I'm confident we will_) you will likely see a pick up in plugin/package development.

Regarding composers. I would say (_as noted previously above_) UmbracoBuilder for internal use, IUmbracoBuilder for external.

You can then pass IUmbracoBuilder to IComposer.

```c#
public interface IComposer
{
void ConfigureServices(IUmbracoBuilder builder);
}

```c#
public class MyComposer : IComposer
{
    public void ConfigureServices(IUmbracoBuilder builder){

            // Set stuff using well named extension methods.
            builder.Configure<MyFileSystemOptions>();
            builder.SetFileSystem<MyFileSystem>();
            builder.AddPropertyMapper<MyPropertyMapper>();

            // IApplicationBuilder contains the service provider so smidge could check to
            // see if is the registered implementation in app.UseUmbracoRuntimeMinifcation.
            builder.SetRuntimeMinifier<MyRuntimeMinfier>(); 

            // Add events using some nice syntax we haven't defined yet.
            // I cannot decide whether we should have specific methods or something that takes two generic params.
            builder.AddEvent<ISaveEvent, MySaveEvent>();
            builder.AddSaveEvent<MySaveEvent>();

            // Add something we don't have an extension for, we can extend IServiceCollection for convienience though.
            builder.Services.Add(MyCustomThingy);
            builder.Services.Set<IThing, MyCustomThingyThereShouldOnlyBeOneOf>(); // Ooh custom set extension!
        }
}

Surely that would be a good developer experience no? I think it provides real power.

Docs... Can be updated... If I finally get my way with #9338 then we can generate proper API docs for everything public which would be a massive improvement for plugin/package developers...

P.S I think there should probably be the equivalent to this that allows access to a Configure(IUmbracoApplicationBuilder app) maybe even the same composer to reduce lookup?

P.P.S I think IComposer should be called IUmbracoComposer for better SEO.

[rustybox]

So

• UmbracoBuilder would 100% replace Composition.
• UmbracoBuilder would need to support the CollectionBuilder setup that Composition has at present (or something similar)
• IComponent would disappear because you can just hook onto AppStarting or whatever.

I have no problems with it personally, but it conflicts with the minimal migration path goal.

---

My point about the docs was more about how public the RuntimeLevel attribute was, if IComponent was to survive (with StartupStragety) we may need to support User/Package components which start at install/upgrade time.

[JimBobSquarePants]

My point about the docs was more about how public the RuntimeLevel attribute was, if IComponent was to survive (with StartupStragety) we may need to support User/Package components which start at install/upgrade time.

User/Package components should be I{Specific}Event types that are registered against the service collection.

CollectionBuilder is more complicated than it needs to be. The extensions methods managing ordered collections could have overloads containing an index. The internal logic collection building logic is handled by the UmbracoBuilder and the types injected as IEnumerable<TService>.

I have no problems with it personally, but it conflicts with the minimal migration path goal.

We've already crossed that line.

[lars-erik]

I've been considering those events and dependencies between packages. Should there be some metadata about the registrations to imply that you depend on other startup handlers?

For instance USync:
builder.RegisterEventListener<UmbracoStartupEvent, UsyncStartupListener>()

My package:
builder.RegisterEventListener<UmbracoStartupEvent, MyThingDependingOnUsync>().DependsOn<USyncStartupListener>()

We don't know which order these composers will be ran in, so "my package" can't insert before or after the usync thing in a pipeline.
I think that's the last non-addressed issue for now.

Thoughts? Or did I miss anything?

[JimBobSquarePants]

I don't think event handlers should have dependency like that. If something introduces a temporal dependency then it should provide it's own event handling mechanism to register against.

E.g. Usync adds extensions to IUmbracoBuilder to allow registration.

[lars-erik]

So in order to depend on another plugin's initialization, the other plugin has to provide a startup event?
builder.RegisterEventListener<UsyncStartupEvent, MyThingDependingOnUsync>()

[JimBobSquarePants]

99.9999999% (that's a real number) of plugins won't need this functionality and I don't think Umbraco should introduce complexity in order to solve it.

[lars-erik]

I'm fairly much on board with that. If you need to depend on a plugin's startup, either reconsider your options, or PR a startup event. :)

[rustybox]

It'd be nice to hear it has a chance of being merged before cutting code :)

[rustybox]

I don't think I'm sold on the event based component setup anymore.

• UmbracoBuilder and kill Composition, sure.
• Add EventAggregator to kill static events, absolutely.

But for Component setup all it would take is adding a RuntimeLevel prop to IComponent or a couple of extra interfaces like IUmbracoInstallComponent and then initialize based on that.

Refactor could be done in about an hour and is tiny in terms of migration path, just set prop if required and remove attribute.

What's so bad about this approach?

It's so simple, and removes the need for conditional registration of services or customized controller discovery.

[lars-erik]

When the point of runtimelevel is just to distinguish between install or not, I think having everyone's "components" having to implement a runtimelevel is overkill. Also, it's a typecode (smell). Maybe a marker interface would do, but I still favor having HQ maintaining a concrete list for it. (If only we had named services & composition in MSDI)

Maybe we're even forgetting the awaited events. UmbracoInstallingEvent vs. UmbracoStartingEvent?

[rustybox]

concrete list breaks the ability to add user/package dev components in between

[lars-erik]

The third party packages or user things are not ever supposed to run during umbraco installation are they?

[lars-erik]

Just to reiterate - didn't we agree to kill component all together in favor of events? If so, the installation components need to be moved to an installation event, and incidentally packages might actually listen to it unless it's made internal.

[rustybox]

If that's the intent then the docs are mental https://our.umbraco.com/documentation/implementation/composing/#runtimelevel

We did, but then I'm doubting it as it's a much more complex refactor that makes migration path for others harder and I'm not sure of the benefit.

[lars-erik]

Migration path would be swapping interface and change signature of components. Not that bad?

[rustybox]

Migration path would be swapping interface and change signature of components. Not that bad?

That's fair but the refactor is still much larger in comparison especially if you consider all the tests that are in place.

What would be so bad about interfaces then, resolve all IUmbracoInstallComponents then call Start on them.

Regardless of opinions on [ComposeAfter] [DisableComposer] it's there and would work with this approach.

Always register according to the graph in composers.cs but only start those appropriate for runtime level.

[lars-erik]

It's not super bad, yet it would be more uniform with segregated events. However, it might be an idea to merge "swap LI for MSDI" before we start the whole event aggregator adventure. (See? I used the name. I used it!)

[rustybox]

Or how about not having special install interfaces.

But ~composition~.UmbracoBuilder.AddInstallComponent, AddRunComponent.

Do the collection builder thing.

I think I like this best!

Could centralize these as well, have all components added in one place so you get your static list, it's just that the list exists in a composer.

Migration path for others is Add/WithInstallComponent instead of AddComponent (add component can be the default case so 99% of folk don’t even have to alter their composer except to remove annotation).

---

Swap LI for MSDI is already merged 😕

[lars-erik]

Moving the registration to concrete methods on the builder will allow _any_ refactoring later, so it sounds like the proper path forward. 👍

Swap LI for MSDI is already merged 😕
🎉

[JimBobSquarePants]

The docs are mental, the current architecture is mental. That’s why I raised the issue in the first place!

Let’s not keep back tracking. It all doesn’t have to happen in one PR.

[rustybox]

I think this is better and back tracking in this case is the appropriate thing to do.

I still think EventAggregator should be in for replacing static events, but I honestly believe using the collection builder pattern for components would work perfectly.

There's barely any difference between AddEventHandler\ and AddInstallComponent<>() but the latter is much simpler in terms of refactoring and migration path so it's a massive win.

Also it handles all the existing use cases better (run after Examine before USync) or whatever without begging for an event in Examine to hook onto.

• Replace Composition with UmbracoBuilder
• Allow adding Components (perhaps with rename) to UmbracoBuilder with the option of running always or only on install etc
• Make CoreRuntime (or equivalent) & IComponent implement IDisposible and let container trigger Component shutdown
• Add EventAggregator and kill all static events (separate Issue)
• Add all the nifty extensions on UmbracoBuilder to AddPropertyMapper etc

[lars-erik]

Can't AddStartupHandler<>() just wrap AddEventHandler<UmbracoStartup, ExamineStartupThinger>?
And AddInstallHandler<>() the install one?
If we absolutely have to keep the "old way" in there, we can always just make a generic adapter...

[rustybox]

Doesn’t fix the before usync after examine use case.

I’m 100% convinced this is the best of all worlds.

[lars-erik]

So you're bent on keeping attributes to apply order? In that case, we could just as well make it fluent like I suggested. In any case, @JimBobSquarePants names it temporal couplings because it's something that have to run in order. ;)

🤷‍♂️

[rustybox]

You cant index a database before there’s a database, I can’t fix that.

It doesn’t have to be attribute based there can be an install component composer that adds all install components.

People downstream can insert between because it’s just a collection of components.

[lars-erik]

You won't know where you are in the downstream, so usync might be loaded or not. That's where attributes, properties or metadata (dictionary somewhere central) comes into the picture. I'd prefer metadata.

[JimBobSquarePants]

There's barely any difference between AddEventHandler and AddInstallComponent<>() but the latter is much simpler in terms of refactoring and migration path so it's a massive win.

I don't want to think about the now, I want to think about the next X years/versions. This is the only chance for real positive change.

Doesn’t fix the before usync after examine use case.

There should never be cross pollination in that manner. Event handling by design should isolated reaction to a given trigger. The dependence on a given order is risky.

Examine should never be indexing the database until a saved event is fired. If it were to try before then it would be registered against the wrong event.

[rustybox]

It’s just an example even if it’s bad.

Why is events better than collections.

This IMHO is the less hacky solution, it’s also the simplest.

[lars-erik]

I just did a search for ComposeAfter on github. You can see it here:
https://github.com/search?l=C%23&q=composeafter&type=Code

There's 85 publicly available classes using it. I didn't search for ComposeBefore yet.

Quite a few do

[ComposeAfter(typeof(IUserComposer))]
[ComposeAfter(typeof(ICoreComposer))]

just to be able to compose after everything else have been set up. Sounds like Application_Started(object sender, EventArgs e) to me. 😆 (clearly missing UmbracoCompositionCompleteEvent)

But there's also a lot of components adding to collection builders. Which is what you're on about.
Collection builders are registered after all composers are ran, right?
If so, collections are extensions on the builder and will be lazily initialized.
Could we instead of adding "temporal couplings" to the DI, add metadata for order fluently to those?

builder.UsyncPipeline().Add<MyDependentPipelineThing>().DependsOn<AnotherPackagesPipelineThing>()

That would mitigate having to register in order, and it would isolate "temporal coupling" to collection builders.

[JimBobSquarePants]

Why is events better than collections.
Because events do not and should not require temporal order. I can't stress this enough.

I have nothing positive to say about the existing CollectionBuilder architecture. It serves no good purpose IMO

[lars-erik]

LOL, so previous comment is void?

[JimBobSquarePants]

If you want to register something in a particular order insert it into a list.

[rustybox]

ComposeAfter etcneeds to be around such that add unique works (or not because people can write a startup class that uses different umbraco builder calls, but let’s just focus on components right now.

My issue at the moment is because services are conditionally registered based on runtime level I can’t validate the container.

The conditional registration is only around to prevent run only components being initialized when installing or upgrading, so let’s always register and do something else to work out whether to start the components.

If there was a component composer that added all components to service collection in a way that some are install only, some are marked run only and some are always run (this can be a simple list instead of a ComponentCollection build by a ComponentCollectionBuilder that subclasses OrderedCollectionBuilder)

That gets you the static list of components in an order that they should be initialized wherever order is important.

What are the negatives of this approach.

What are the positives of events.

[lars-erik]

Would it be an idea to do another live chat? I'm getting the feeling we're all talking past each other...

[JimBobSquarePants]

Perhaps....

I _think_ there's a fundamental misunderstanding here about how service registration actually works. Both in the existing code (_where they keep creating instances of things for some reason_) and in this conversation. (I could be wrong, maybe I'm simply not being clear enough).

Registering services doesn't create instances of those services. You're merely storing the information required for the service provider to be able to create instances as and when required.

Registration should always happen. Determining when things are activated is a case of injecting the right things in the right constructor when the service provider is called upon by the application to create instances of things like controllers, middleware, or background tasks.

The fact that runtime only components exist currently for registration is the result of a misunderstanding of the technology and pattern.

Even things like named services are not really required for Umbraco Cloud. You can create an unlimited number of environmental variables and extend the hosting environment to recognize the relevant config for that environment. (I touch upon conditional registration via configuration for tests in #8654)

RegisterUnique (_I hate that name_) is super easy to implement without anything like ComposeAfter. There already exists the IServiceCollection.Replace() method that can be used to implement this. I have examples in ImageSharp.Web for my limited use case there.

I'm not advocating events for temporal registration of services. I'm advocating events for handling events, notifications, etc. I am saying though that event handlers should be registered against the service collection.

For example, say you create an IComposer which registers a bunch of property converters etc. It also registers an IUmbracoStartupHandler against the service collection that handles an UmbracoStartup event. The application starts and at the correct moment in the application runtime (_likely via middleware_) the collection of registered IUmbracoStartupHandler that were loaded via constructor injection are iterated through and their Handle method called.

[rustybox]

Would it be an idea to do another live chat? I'm getting the feeling we're all talking past each other...

Probably a good idea.

I think there's a fundamental misunderstanding here about how service registration actually works. Both in the existing code (where they keep creating instances of things for some reason)

Agree

and in this conversation. (I could be wrong, maybe I'm simply not being clear enough).

Strong disagree

Registering services doesn't create instances of those services. You're merely storing the information required for the service provider to be able to create instances as and when required.

Indeed.

Registration should always happen. Determining when things are activated is a case of injecting the right things in the right constructor when the service provider is called upon by the application to create instances of things like controllers, middleware, or background tasks.

Agreed, the only time conditional registration is mentioned is to discuss current implementation.

The fact that runtime only components exist currently for registration is the result of a misunderstanding of the technology and pattern.

That's what I'm trying to resolve right now.

RegisterUnique (I hate that name) is super easy to implement without anything like ComposeAfter. There already exists the IServiceCollection.Replace() method that can be used to implement this. I have examples in ImageSharp.Web for my limited use case there.

RegisterUnique doesn't exist anymore, ServiceCollection.Replace is used (although we have an extension called AddUnique(this IServiceCollection...) which calls Replace to ease migration)

I'm advocating events for handling events, notifications, etc. I am saying though that event handlers should be registered against the service collection.

Yes, agreed

But all of this is about is Composers and Composition

I am talking only about Components.

The fact that runtime only components exist currently for registration

There's possibly some confusion here, components + for registration throws me off.

Components are at present conditionally registered, then those which are registered are resolved at boot time and "Initialize" is called on them, when they initialize they don't continue to register things as the service provider is built at that point.

[bergmania]

I think it is a very good idea with a live chat.
@Shazwazza is back from vacation next week, but will most likely need some days to get up to speed.

[lars-erik]

I think I might have misunderstood everyone's view on ordering what. So we say that we are OK with some control mechanism to control the order of _registration_. So we're OK with ComposeBefore and ComposeAfter?
These are the mechanisms that makes us able to modify a list _after_ something else have created and initialized it, as well as actually replacing something in the service collection, rather than replacing nothing since the depended composer haven't ran yet.
We do not want to control the order of execution when it comes to events (components / startup / teardown)

If this is the right understanding, I still say we could replace attributes with metadata / fluent registration of the _composers'_ order. I guess properties, external metadata and attributes are all matters of taste, though. In this case the attributes does not imply any more dependencies than we already have, so I'm not as opposed as I'd be against 3rd party framework attributes.

If I'm still misunderstanding something, I'd be super happy for a chat. 😆

[rustybox]

So we're OK with ComposeBefore and ComposeAfter?

I'm ambivalent about it, it's there and it allows AddUnique to work such that IThing can be replaced by IAnotherThing easily in a Composer.

We do not want to control the order of execution when it comes to events

Components do currently initialize in order and terminate in reverse order, I don't know why I just know that's what it does right now :)

[lars-erik]

I'm advocating events for handling events, notifications, etc. I am saying though that event handlers should be registered against the service collection.

Sure, the EventAggregator can look up handlers in DI instead of having its own static list.

Components do currently initialize in order and terminate in reverse order, I don't know why I just know that's what it does right now :)

So there's the detective work. No way to figure if the ordering of execution is used (not compose order), is important, will be broken, and how to replace the possibilities. Unless we wade through the search I did yesterday, and/or ask around and figure why peeps execute initialization in order. Then of course there's the question of "should we just say don't do it, and what do we recommend".

Still thinking we should land all this in a live chat. :P

[rustybox]

Sure but at the same time there is that mandate of not making too drastic changes to ease migration.

I play devils advocate here but I literally don't give as shit about keeping the old ways, however I feel like the argument needs to be represented :)

[rustybox]

A quick scan of all Components doesn't shed any light on why they init / terminate in order.

99% of them just hook event and un-hook on terminate.

Exceptions

• BackOfficeWebAssets (creates some static css bundles & js bundles)
• ClientDependencyComponent (does some stuff)
• CoreInitialComponent (creates some directories if they don't exist)
• DatabaseServerRegistrarAndMessengerComponent starts a BatchedDatabaseServerMessenger
• DisabledModelsBuilderComponent adds a ModelsBuilderDashboardController to disabled controllers (basically feature flags)
• ExamineComponent (creates some some indexes before hooking some events)
• ExamineFinalComponent (rebuilds indexes on init)
• ManifestWatcherComponent (starts some file system watchers)

I'm bored now, have looked at all but not written about all of them, there's nothing I can find that couldn't be handled by EventAggregateor startup event.

I can't find any that have to start in order, but someone somewhere may know of one.

[lars-erik]

The only thing I see in your list is the directory creation. That could be handled by having every consumer use a common thing to ensure folders exist. But what about stuff that adds indexes or such? Won't they have to run after examine have instantiated the directory, a writer and the "manager" things? I still say that could be handled by having n startup events like in V7, though. Initializing, Starting, Started etc. CompositionDone even. And then again, ExamineReady...
Or composed and ordered pipelines of startup handlers... 🤣🤷‍♂️

[rustybox]

n startup events

Yep sounds good

CompositionDone

What would this be used for, composition is ConfigureServices, CompositionDone is basically the same as AppStarting

[lars-erik]

What would this be used for, composition is ConfigureServices, CompositionDone is basically the same as AppStarting

Yep. But more concise. Nothing wrong with multiple events happening at the same time, but with completely different meaning and purpose. Means they can also be moved apart in time without consumers noticing or caring.

[rustybox]

I'm starting a poem

Composers composes Composers in compose by calling compose on the Composers passing a Composition.
When Composers composes Composers they add Components to a Composition, an IRegister with an IRegister that's an IFactory.

[lars-erik]

How many compositions would a composer compose if a composer would compositely compose compositions?

[JimBobSquarePants]

ComposeAfter and ComposeBefore? They’re used for controlling service registration order? If so absolutely not!

[JimBobSquarePants]

I think I’m done with all this tbh. It’s not worth the effort.

[rustybox]

ComposeAfter and ComposeBefore? They’re used for controlling service registration order?

Yes

absolutely not

Then we need to agree an alternative (with buy in from hq), your plan is just controlling it with the order of calls to UmbracoBuilder in Startup.cs I believe?

I think I’m done with all this tbh. It’s not worth the effort.

Haha I have spent much of the refactor thinking I should just build a new CMS :D

[lars-erik]

@JimBobSquarePants, please don't give up on us. 🙏

I think we're _only_ missing a clarification on how to control that serviceCollection.Replace<Something, MyThing>() is ran after serviceCollection.Register<Something>(). Without composition order, how do we solve that?

[rustybox]

Speaking on behalf of @JimBobSquarePants using calls to UmbracoBuilder in order, but that doesn't account for plugins, do the plugins have to write changes to startup.cs?

[lars-erik]

Yes, it's only relevant for plugins.

[rustybox]

Personally I'm fine with having to write a startup.cs.

Plugins presumably would have to document hey call umbracobuilder.AddFoo() after umbracoBuilder.AddBar() (or just services.AddFoo/Bar)

Can we get the buyin from Umbraco though?

[lars-erik]

It will prevent some of the SaaS things to work with those plugins in a no-code way. 😒

[JimBobSquarePants]

I think we're only missing a clarification on how to control that serviceCollection.Replace<Something, MyThing>() is ran after serviceCollection.Register<Something>(). Without composition order, how do we solve that?

Replace doesn't care if the service doesn't exist.

https://source.dot.net/#Microsoft.Extensions.DependencyInjection.Abstractions/Extensions/ServiceCollectionDescriptorExtensions.cs,678

UmbracoBuilder would register all critical services first anyway before all ~IComponent~ IComposer instances are loaded and passed the instance. Only IUmbracoBuilder is passed to those instances.

[rustybox]

Replace doesn't replace if the thing it's replacing gets added after.

---

before all IComponent

IComponent wouldnt need to exist if we agreed to replacing it with an EventAggregator setup.

Components don't register services, they hook events.

Perhaps none of this matters because you could just add subscribers for events which never get fired when RuntimeLevel == Install

I don't know reasons why this wouldn't work other than backwards compat, @Shazwazza might.

[lars-erik]

Interesting. What happens if two replaces are done in random order?

[rustybox]

the last call wins :D

[rustybox]

Is there a suggestion for no-code support (my personal suggestion would be don't support no-code, this is a CMS framework with a built in backoffice UI not am OOTB ready to go CMS) but I don't think that will fly :)

Edit: I'd prefer if Umbraco intended to compete with EpiSever rather than Wordpress, and not attempt to do both, but that's because I'm a programmer, I can't argue from the other side where a designer wants to build a ~scalable~ fast (until their BBOM explodes) CMS without needing devs.

[bergmania]

Is there a suggestion for no-code support (my personal suggestion would be don't support no-code, this is a CMS framework with a built in backoffice UI not am OOTB ready to go CMS) but I don't think that will fly :)

I'm sure we will need to support OOTB.

In general, I think the changes you are discussing now, are so extensive, we will need to write the proposed changes in an RFC, and have comments from a wider range of the community before we can decide to do it. I fear there are some use cases we are not aware of.

[rustybox]

In general, I think the changes you are discussing now, are so extensive

That was hyperbole.

---

My suggestion is that we have multiple AddComponent methods on a composition/umbraco builder

AddInstallComponent, AddRunComponent, AddAlwaysComponent.

---

@JimBobSquarePants wants the more idealistic approach which I agree with in principal but I'm trying to offer a pragmatic solution.

[JimBobSquarePants]

Yeah, I think IComponent is unnecessary. Event handlers should be registered against the service collection

[rustybox]

Presumably the mandate not to change everything stems from

[rustybox]

I have had 2 epiphanies before my morning coffee.

1. Anyone that really needs to not do stuff in a component (or elsewhere) if running in install mode can take a dependency on IRuntimeState and use it as a feature flag.

2. I have no idea why the RuntimeLevel attribute exists, I made an assumption that it was about Components but I don't know how I came to that conclusion, it's not documented anywhere.

I disabled the filtering in Composers.GetRequirements and did an install followed by a run and messed around in the back office.

Guess what, the world didn't explode.

tl;dr I have no idea of the intended purpose of RuntimeLevel attribute.

[Shazwazza]

Hi all (slowly getting back), amazing thread btw and a ton of great ideas and discussion 👏

Some quick answers to some things:

Anyone that really needs to not do stuff in a component (or elsewhere) if running in install mode can take a dependency on IRuntimeState and use it as a feature flag.

Absolutely!

The RuntimeLevel attribute should probably die and I agree with what everyone has (I think) agreed with here: Everything should always be registered in DI regardless of the runtime state. If code needs to do different things it can just check the IRuntimeState

The third party packages or user things are not ever supposed to run during umbraco installation are they?

Yes, some packages will do things differently depending on the runtime state but as above, packages will need to check for that. Things like Deploy, and others execute things on the installation state.

Components do currently initialize in order and terminate in reverse order, I don't know why I just know that's what it does right now :)

I don't see any reason why they need to terminate in reverse order. I have no idea why Terminate exists and it wasn't IDisposable from the beginning.

Is there a suggestion for no-code support (my personal suggestion would be don't support no-code, this is a CMS framework with a built in backoffice UI not am OOTB ready to go CMS) but I don't think that will fly :)

no-code support will be required. Perhaps it's possible after all of this stuff that a developer can opt-out of that in their Startup and manually initialize each package's startup code. I think that's for another discussion though.

ComposeAfter and ComposeBefore? They’re used for controlling service registration order? If so absolutely not!

It's an ugly way to control a couple things:

a) as a package dev I want to execute code after/before another packages IComponent.Initialize
b) as a package dev I want to be able to replace a service from another package

I don't like it at all and I think a lot of this recent discussion is directly related to how to solve a) and b).

I think a) can be solved with events and I think that is what people are agreeing to here? If there would be a simple way for a package to have a way to register services and execute code on startup/shutdown and somehow automatically behind the scenes events are raised for it's startup/shutdown sequence (before/after) then if required another package can do things before/after startup/shutdown of any other package easily and ideally without taking a dependency on that package.

I think we're only missing a clarification on how to control that serviceCollection.Replace() is ran after serviceCollection.Register(). Without composition order, how do we solve that?

So this is b). Can this be solved with events too? I realize that if 2x different packages want to replace a services from package xyz then it's not possible to know which of the 2x other packages would 'win' but that's the same case as we have today with ComposeAfter so I'm not sure we need to really worry about that. If some edge case requires this then it would be up to the end user to adjust the final Replace in their Startup. But perhaps there's other ways too?

In general, I think the changes you are discussing now, are so extensive, we will need to write the proposed changes in an RFC, and have comments from a wider range of the community before we can decide to do it. I fear there are some use cases we are not aware of.

I think that will be ok though? I think so long as whatever a package dev needs to do to register services and execute code on startup/shutdown is simple (maybe more simple that what we currently have?) then it still shouldn't be too much code for them to change, at least that's how it seems to me. We could even have some shim of some sort later if we felt it was important enough to make. Having some POC code to demonstrate how to do the following as a package dev would be quite helpful:

• register services
• execute code on startup/shutdown
• execute code before/after another package
• execute code based on IRuntimeState

I think that covers everything being talked about?

I think I’m done with all this tbh. It’s not worth the effort.

Please no, this thread has come so far, I don't think there's much more to go. 🙏

[Shazwazza]

I'm unsure if this is helpful or not but I found some blog posts detailing composition in v8 from the early days just in case there might be some other info in there about what is in v8:

• https://www.zpqrtbnk.net/posts/composing-umbraco-v8/
• https://www.zpqrtbnk.net/posts/composing-umbraco-v8-collections/
• https://www.zpqrtbnk.net/posts/composing-umbraco-v8-components/

[rustybox]

So this is b). Can this be solved with events too?

In theory, however not with mediatr, with Udi's sample you can add an event handler without making use of the container, ~we could pass the instance around to composers (this would require temporal coupling to set ServiceProvider after initial construction)~, but in general what we were talking about was the EventAggregator service locating handlers from the container when events are fired.

This wouldn't be useful for working out when someone else has finished adding services as the only time the handlers could be triggered is after the ServiceProvider has been built at which point no one can add services anymore.

[rustybox]

In summary.

1. RuntimeLevel attribute can be removed
2. Anyone that needs to conditionally exec code can check against RuntimeState.Level

That change alone (plus a fix against the setup for BackOfficeUserManager and possibly just a couple more) gets us to a point where container validation can be turned back on, I'll gladly put a PR in to resolve this.

• Any enhancements around killing ComposeAfter/ComposeBefore can come at a later date.
• As can discussions around killing IComponent entirely.
• EventAggregator discussions can evolve in #9397

---

Outstanding question, is Umbraco.Web.Common.Builder.UmbracoBuilder intended to replace Composition or did you expect them both to exist?

---

Having some POC code to demonstrate how to do the following as a package dev would be quite helpful:

Would this go on https://github.com/umbraco/UmbracoDocs? v9 branch?

[Shazwazza]

---

Outstanding question, is Umbraco.Web.Common.Builder.UmbracoBuilder intended to replace Composition or did you expect them both to exist?

Yes IIRC (sorry still getting back into the groove of things and trying to remember everything). The example that @JimBobSquarePants provides above https://github.com/umbraco/Umbraco-CMS/issues/8653#issuecomment-726096784 is along the lines of I think where we want to go.

In above discussions like: https://github.com/umbraco/Umbraco-CMS/issues/8653#issuecomment-686400142 we said

IUmbracoBuilder used for end user developers for adding/replacing

I know we have the generic ability to IServiceCollection.Replace any service but I think the intention/end goal was to make extension methods on IUmbracoBuilder to be very explicit to make it super easy/clear for developers to do things. Like builder.SetRuntimeMinifier<MyRuntimeMinfier>(); (as per james' example).

There's a few of other questions you had above in relation to some of this too

Is the intent that all Umbraco Composers ~35 disappear and become extensions on UmbracoBuilder?

Yes i think so, above I mentioned "But with IUmbracoBuilder perhaps the idea of Umbraco itself using composer/components might not make sense and these end up only being used by packages - though maybe that refactor if we want to do that can come later once the primary goals are reached."

Are IComposers just hanging around just for package devs and end-users?

Yes I think so, I don't think there's any benefit of having core 'IComposers' apart from just logicially separating code that registers different types of services but this can just as easily be done with ext methods. I would suggest even end-users just use their Startup code and not IComposers, so those should just be for packages.

If all core stuff is just registered by us, then IComposer's (or as James says to be renamed IUmbracoComposer) would execute then the whole ordering of ICoreComposer vs IUserComposer insanity doesn't need to exist.

---

Would this go on https://github.com/umbraco/UmbracoDocs? v9 branch?

I was only referring to POC/demo code for this discussion just to start getting a better understanding for all of us as to where we want to head in a more ideal startup approach. Like James' example but to include all 4 of these:

• register services
• execute code on startup/shutdown
• execute code before/after another package
• execute code based on IRuntimeState

Could even be a gist or something that is editable so it doesn't get lost in the abyss

---

I've been thinking about the ordering of service registrations:

I think we're only missing a clarification on how to control that serviceCollection.Replace() is ran after serviceCollection.Register(). Without composition order, how do we solve that?

So this is b). Can this be solved with events too?

Maybe this just unnecessary and can be simplified with a callback or similar. Maybe something like:

public class MyComposer : IComposer
{
    public void ConfigureServices(IUmbracoBuilder builder)
        {
            // add callback to execute before the ServiceProvider is built but after all ConfigureServices calls are done
            // and before the end user's ConfigureServices is done
            builder.OnContainerBuilding(c => {
                 // will execute just before the container is built providing a final opportunity for packages
                 // to replace services from other packages
                 c.Replace<Something, MyThing>();
            });
        }
}

Since there's no way currently in v8 to have 2x different packages trying to replace a service of another package and knowing which one will win, then it doesn't actually make sense to control 'order', it only makes sense for a package to declare some code to replace services at a 'final' stage. This would achieve the same thing and doesn't mean we need to have packages listening for when other packages do things regarding composition.

If there's some crazy edge case where 2x packages are trying to replace another package's service or core service then it will be up to the end-user to define the replacement. These callbacks would need to execute 'before' the absolute final stage of when the end-user configure's services. The end-user's code should in Startup should be the absolute final place to replace/configure services.

That seems pretty simple to me but maybe I've totally missed/forgot something, let me know what you think

[rustybox]

Examples

Register services

In startup

// Startup.cs
ConfigureServices(IServiceCollection services) 
{
   services.AddTransient<IFoo,Bar>();
}

In a composer

public class MyComposer : IUmbracoComposer
{
    public void Compose(UmbracoBuilder builder)
    {
        builder.Services.AddTransient<IFoo,Bar>();
    }
}

Execute code on startup/shutdown (+ RuntimeState, omit if not required)

Depends on event aggregator talks, if we go down that route there's no need for IComponent anymore

public class MyComponent : IComponent
{
    private readonly IRuntimeState _state;

    public MyComponent(IRuntimeState state)
    {
        _state = state;
    }

    // Maybe this happens in ctor instead if IComponent survives
    public void Initialize()
    {
        if(_state.Level < RuntimeLevel.Run)
            return;

        // Do Stuff
    }
}

With event aggregator that looks something like

// If this class is a singleton and requires shutdown code it just implements 
// INotificationHandler<UmbracoApplicationStopping> also
public class MyComponent : INotificationHandler<UmbracoApplicationStarting>
{
    public Task Handle(UmbracoApplicationStarting notification, CancellationToken cancellationToken)
    {
        if (notification.RuntimeLevel < RuntimeLevel.Run)
            return Task.CompletedTask;

        // Do Stuff
    }
}

Execute code before/after another package

At the moment this is handled by Composition order, I'm not sure whats happening here.

Do we actually have concrete examples and requirements for this?

[rustybox]

Replacing composition with UmbracoBuilder and removing CoreComposers isn't too bad, it's just boring and begging for conflicts.
IUmbracoBuilder presumably needs to move out of Umbraco.Web.Common.Builder and into Core

What happens to collection builders are they still around?

Are we saying ComposeAfter Compose before would be gone entirely?

Services are then just configured in order of

1. Umbraco internal builder stuff
2. IUmbracoComposer (for plugins, ran in order of type finder discovery)
3. Actions added by composers for OnContainerBuilding

[JimBobSquarePants]

The state wouldn’t be a constructor parameter. Any requires information should be passed to the handler via Handle()

[rustybox]

So RuntimeLevel would live as a prop on UmbracoApplicationStarting, sure.

Edit: updated example to match

[JimBobSquarePants]

Yep...

In startup I would always recommend using the UmbracoBuilder instance there also.

```c#
var builder = services.AddUmbraco();

builder.SetFileSystem();
```

Though UmbracoBuilder would have to have a Build() or Complete() method to ensure auto registration occured.

[Shazwazza]

What happens to collection builders are they still around?

yes these should remain as-is. Collections will need to be exposed one way or another on the IUmbracoBuilder just like they are currently on Composition. Later on/ideally we have nicer typed syntax for dealing with the different collections.

Depends on event aggregator talks, if we go down that route there's no need for IComponent anymore

I can see some benefits of this approach but I'm unsure they outweigh the amount of effort involved for us to do it and then to get all package developers to change since IComponent (or IUmbracoComponent) seems ok to me and it already exists/works without too many changes for both us and package devs. But is the consensus that event aggregator is better/simpler than IComponent (with IDisposable?) Can you inject things into that class? If so then why not just inject IRuntimeState instead of having to add that as part of UmbracoApplicationStarting? If you can't inject things into it, how do you access all the services you might need?

At the moment this is handled by Composition order, I'm not sure whats happening here. Do we actually have concrete examples and requirements for this?

Most concrete examples are package running things after certain core components but if all core services are registered ourselves before any package components are run then this scenario doesn't ever occur. I don't have a concrete example of a package that relies on another package using ComposeAfter but i'm sure it exists someplace, but like I said above, that would be solved in a simpler way by defining a callback during composition like the OnContainerBuilding example. Then ordering doesn't matter at all.

Services are then just configured in order of
Umbraco internal builder stuff
IUmbracoComposer (for plugins, ran in order of type finder discovery)
Actions added by composers for OnContainerBuilding

I think this needs to be further defined, something like:

• end-user's ConfigureServices (user can add things before all of this)
• Umbraco internal builder stuff
• IUmbracoComposer (for plugins, ran in order of type finder discovery)
• Actions added by composers for OnContainerBuilding
• end-user can then replace things after all of this

// Startup.cs
ConfigureServices(IServiceCollection services) 
{
   // user can have custom services before Umbraco
   services.AddTransient<IFoo,Bar>();

  // will add all of Umbraco's services
   var umbracoBuilder = services.AddUmbraco();

   // does this makes sense to initiate the IComposers or maybe that just automatically happens in AddUmbraco
   // or there's some options that can be passed into AddUmbraco to control what it does.
   // this could run all IComposers and then run all OnContainerBuilding callbacks after
   umbracoBuilder.AddPackages();
   // Or I think this is the phase James wanted to be called Build()? 

   // user can replace Umbraco and package services and collections
  builder.SetFileSystem<MyFileSystem>();
  builder.OEmbedProviders().Append<Spotify>();

   // user can have custom services after Umbraco
   services.AddTransient<IHello, World>();
}

Just an example/idea, if it can be made better/simpler or if I'm way off please post :)

[rustybox]

I can see some benefits of this approach but I'm unsure they outweigh the amount of effort involved for us to do it and then to get all package developers to change since IComponent (or IUmbracoComponent) seems ok to me and it already exists/works without too many changes for both us and package devs

The real win here isn't that we can 1:1 replace components with big event handlers, it's that most components just hook ContentService.Saving etc, if we were to replace the static events then you don't need a component in the first place as you can just add an extra handler for ContentSaving event right into service collection from a composer.

The ApplicationStarting event is more for something like creating the media directory if not exists (CoreInitialComponent) or setting up the file system watchers (ManifestWatcherComponent).

Can you inject things into that class? If so then why not just inject IRuntimeState instead of having to add that as part of UmbracoApplicationStarting?

Absolutely, the handlers are resolved from container, @JimBobSquarePants point was that in the case of ApplicationStarting, RuntimeLevel is relevant data for the event. It's not that you cant take a dep on RuntimeState, more that you shouldn't have to in this particular case.

end-user's ConfigureServices (user can add things before all of this)
end-user can then replace things after all of this

Yeah all the concern has been around no-code support.

If you have ability to edit Startup.cs (or replace it entirely) then you can add code at the beginning or end of ConfigureServices.

[JimBobSquarePants]

@Shazwazza I think CollectionBuilderBase and any derived types need a thorough review. As-is they seem to be responsible for resolving items which they shouldn't. I think there's great potential for simplification and optimization there.

[Shazwazza]

@JimBobSquarePants yep sounds great, there's probably a lot of legacy/etc... in there! There's stuff in there that shouldn't even exist in v8 (i.e. LazyCollectionBuilderBase) do we want a call about that and/or Is it worth starting a diff ticket specifically about this?

[rustybox]

I have the site working for install & run (backoffice good post install) with CoreRuntimeBootstrapper removed.

The tests aren't very happy but this is huge.

RuntimeState.Level determined as part of Startup.Configure instead of Startup.ConfigureServices.

We can drop this BS

public static IUmbracoBuilder AddUmbracoCore(...
  Func<GlobalSettings, ConnectionStrings, IUmbracoVersion, IIOHelper, ILoggerFactory, IProfiler, IHostingEnvironment, IBackOfficeInfo, ITypeFinder, AppCaches, IDbProviderFactoryCreator, CoreRuntimeBootstrapper> getRuntimeBootstrapper)

We never have to call BuildServiceProvider, not even once! (if we fix a couple more little bits)

---

And tests passing, there's still more cleanup to do but this is so much better 8b97ce6 (and my wife will kill me if I don't step away from PC)

[JimBobSquarePants]

LEGEND!

[rustybox]

It's gone, the crazy func is gone!!!

Only calls to BuildServiceProvider are in Test projects (and Umbraco.Web but that doesn't count)

[rustybox]

I believe that once #9415 is merged we could close off this issue?

9397 & #9446 document the further desirable enhancements

Possibly require an extra issue for some of the syntactic sugar extensions on UmbracoBuilder but I think it's probably better if someone else types that up @JimBobSquarePants?

[JimBobSquarePants]

Agreed. I'll have a look at raising a specific issue.

[rustybox]

Oh and collection builder review issue?

[Shazwazza]

Merged 🎉 🚀 bloody amazing work everyone 😍 and yep I agree I think we should close this issue now and discuss further on #9397 & #9446 + created issues re: UmbracoBuilder enhancements and collection builders