[Parked] V9: Removal of Composers.cs, various composition attributes and IUserComposer

[p-m-j]

Morning all,

There is a PR waiting to get merged which removes Composers.cs and the ordering/disabling attributes (not the concept of composers or the IComposer interface but the class which is responsible for ordering them). This can be found at #10881

Ordering composers used to be much more important, if you wished to replace a service in core you could ensure you implemented IUserComposer which would always run after ICoreComposer instances or use the ComposeAfter/ComposeBefore attributes.

Compose order controlled component initialization order, which was important for ensuring dependencies would be running before your code (Examine etc).

After the various refactors for v9, those dependencies Examine etc will start as required and be available without worrying about composition order. Additionally, we have the EventAggregator setup which should make it easy for packages to add lifecycle events that other packages can hook onto.

Composers are still around for use in packages but are never used by core, they will always run after core has added all of its service registrations. Composers are still useful in low/no code environments so it’s often worthwhile to support both Composers and Startup.ConfigureServices for registering your services either by providing another library with a Composer, or by conditionally exiting out of a composer if everything is already configured by other means (thanks @KevinJump for the idea here).

We would like to know if you have any use cases for ComposeAfter/ComposeBefore so that we can discuss alternatives and find any problems we have yet to think about.

The PR also removes IUserComposer, this would be a find and replace (to IComposer) fix in any packages that make use of the interface but we have also discussed bringing the interface back with an obsolete attribute if this is a concern for people?

I think if there are major objections, we are willing to hold off and just add obsolete messaging before rolling out changes in the next major version, but it’d be really nice to bite the bullet to clean up some of our legacy.

[mattbrailsford]

@p-m-j so in Vendr, we have our own eventing system as we don't want to rely on Umbraco's implementation and so if anyone wants to hook into Vendr's events, they are going to need to wait until Vendr is installed.

This is pretty straight forward for people modifying startup.cs to just make sure their custom registration logic is inserted after Vendr, but we also still make use of Composers for the "automatic install" ability (ie, if someone doesn't call AddVendr() in startup,cs then we have a composer that ensures it get's installed / setup regardless).

I guess the thing to consider here is do we want to support "automatic install" whilst at the same time allow devs to define our custom events, which if we do, we will need composers and compose after in order to maintain sequencing. The alternative is, we say "if you want to extend Vendr, then you MUST use AddVendr() and then you must register your event handlers after this".

[KevinJump]

Yeah this is more - at the moment - depending on the complexity of your project -

I don't think you can have a "no composer registration" model for a project/package without the risk of crashing the site on dependencies that haven't been registered for things that Umbraco has auto discovered.

Our pattern is - if you are in a composer and want uSync. then builder.AdduSync() in your composer, if its not already registered this will register it, and its like you've done ComposeAfter

ComposeBefore is harder - but we've changed some internal patterns to load collections, so things don't have to be manually registered - I am not sure if this is a performance nightmare. (not loads of things but its still type scanning/loading at startup) but it does get around needing things registered before your core thing.

[p-m-j]

@mattbrailsford but events won't fire during service registration (Startup.ConfigureServices) only at runtime, the order in which services are registered shouldn't matter.

[p-m-j]

@KevinJump yep we absolutely agree that Startup.cs only is probably not viable, that's why Composers are still around.

[p-m-j]

@mattbrailsford, didn't spot the collection builder part. I guess that would mean any users wishing to add to the collection builder would have to add WithCollectionBuilder<VendrClass> just in case their composer runs first, calling WithCollectionBuilder<T> multiple times isn't an issue.

However this could be a problem if users need to insert to the collection in a specific order before / after you?

[mattbrailsford]

@p-m-j yup, exactly. Which for us is important as we have pipeline collection builders so we need to define the pipeline first and then people can compose after us to modify that pipeline

[KevinJump]

Similar to @mattbrailsford - we've moved everything into IBuilderExtension methods.

This does mean If someone needs uSync the can AddUsync() inside their startup/composer and If it hasn't already been added at that point it will run. (so the builder extension's first line checks for the existence of a service it registers and doesn't run if that is there). but the preferred method would still be to add them in order in startup.cs as this allows options etc to also be configured.

We do still trigger the builder extension in a composer - so people don't have to change startup.cs, not everyone installing our packages will be also changing the core code of the site. (maybe with v9 that is a change that is going to happen).

[p-m-j]

Am I right in understanding that you don't have concerns around removing ComposeBefore, ComposeAfter, Disable etc?

[KevinJump]

Yes i think its fine. (sorry - should have said).

[p-m-j]

OK with removal of IUserComposer also, happy to replace any usages with ICompser?

[KevinJump]

Its slightly more than a find and replace of IUserComposer to IComposer because IUserComposer is only ran at RunTimeLevel.Run ?

It a documentation thing - but think that would need to be stated that the caller is now responsible for working out the runtime state of the application.

[p-m-j]

Yep good shout, that's definitely a point which needs documenting if it's not already.

Edit: Also worth noting this is true even if we don't remove IUserComposer interface.
All composers are added before runtime level is determined by CoreRuntime.

Composers are added during Startup.ConfigureServices, runtime level determined when the application starts (CoreRuntime is now an IHostedService)

[ronaldbarendse]

We can probably get rid of the IComposer implementation and use IStartupFilter instead: https://docs.microsoft.com/en-us/aspnet/core/fundamentals/startup?view=aspnetcore-5.0#extend-startup-with-startup-filters.

These are very similar, but have (at least) the following differences:

• IComposers are automatically discovered/registered, but IStartupFilter needs to be manually added to the service collection/DI container (which already implicitly orders them)
• IComposer exposes an Compose(IUmbracoBuilder builder) method and IStartupFilter a Configure(Action<IApplicationBuilder> next) (similar to middleware, invoking the next action before or after your own code allows explicit ordering)

So I guess the main reason for having the ComposeBefore and ComposerAfter is because of the automatic type scanning/registration of IComposers. And IUserComposer will be similar to invoking the next action in the IStartupFilter before your own code, which is currently already obsolete because the ICoreComposers are/were already executed...

[p-m-j]

https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.hosting.istartupfilter?view=aspnetcore-5.0
Can be used to add defaults to the beginning or end of the pipeline without having to make the app author explicitly register middleware.

[Shazwazza]

RE: IStartupFilter

IStartupFilter is much like our IComponent. However, all IStartupFilters run Before the Startup.cs Configure method which means that anything any user puts in there will execute before any of our own UseUmbraco which will be problematic if people are wanting to add things like endpoints or other middleware that needs to be executed before or during our Umbraco pipeline. IStartupFilter will work but it will only work for folks who want to do things on startup or enable middleware to run before all default middleware.

So for 99% of people, IStartupFilter will not work for integrating middleware with Umbraco.

[p-m-j]

You can also add to pipeline at end if you

public Action<IApplicationBuilder> Configure(Action<IApplicationBuilder> next)
{
    return builder =>
    {
        next(builder); // Startup.Configure called here
        builder.Use(rd => rd); // add middleware very late in pipeline
    };
}

vs

public Action<IApplicationBuilder> Configure(Action<IApplicationBuilder> next)
{
    return builder =>
    {
        builder.Use(rd => rd); // add middleware very early in pipeline
        next(builder); // Startup.Configure called here
    };
}

[Shazwazza]

Sure - but that will still be problematic with Umbraco. It would need to be very custom stuff before that doesn't rely on any other middleware that we register like auth, etc... and if you do it after, the only thing that will run will be endpoints because endpoint middleware is terminating and we register that in our builder at the end so any middleware registered after that will not run.

[p-m-j]

Stumbled across this quite randomly, it's pretty interesting https://andrewlock.net/inserting-middleware-between-userouting-and-useendpoints-as-a-library-author-part-1/

[leekelleher]

Hi @p-m-j 👋

Quick answers to your questions, from my own usage as a package dev...

• Use cases for ComposeAfter / ComposeBefore? Nope. I have seen examples of their usage, (in relation to my packages), but most cases it turns out there were unnecessary. 🤷
• Replacing IUserComposer with IComposer - Yes, that's fine with me. 👍

---

I'd like to make a note about Composer usage...

Composers are still around for use in packages but are never used by core, they will always run after core has added all of its service registrations. Ideally packages would never use composers and would just document how to add service registrations and any request pipeline middleware in startup.cs, however Composers are still useful in low/no code environments so it’s often worthwhile to support both methods.

With v9/netcore, I can see from a dev perspective that Startup.ConfigureServices might be the preference to register services, but putting my website implementers hat on 🤠 and highlighting "Composers are still useful in low/no code environments" bit... with my Contentment package (which is a bunch of property editors), I would not expect anyone to have to manually add the .AddContentment() extension to their Startup code. Meaning that my services would need to be auto-registered within a Composer, (or some other auto-startup code).

[p-m-j]

Cheers Lee, I'm going to edit the OP to remove the bit about preferring ConfigureServices as I meant it in a perfect world where everyone was a dev :)

[leekelleher]

All good, I totally get it. For me it was more that if I don't say anything then "website implementers" may become forgotten.
Whether they are still a part of the demography in a future Umbraco world, who knows?! 😆 , but I do like that we can have "install & forget" packages.

[p-m-j]

I don't think there will ever be a plan to forget website implementers :)

Supporting low code setups is a core concern!

[Mantus667]

Actually, that is the core discussion with v9 I would say.
If you have a look at the way you build "native" applications every extension has to be plugged in by the developer of the application. That is the way it works.

As the users of Umbraco we are kind of "spoiled" by having an automatic way that extensions are picked up by the CMS.

Going forward we have to make the decision if the native way is the "correct" way to go, which makes the named low-code approaches much harder, or keep providing it.

I personally wouldn't mind going the native way. All packages are installed via Nuget on my part. And I wouldn't let the actual customers install packages on their own. But I'm aware that this is a thing for others.

[p-m-j]

I had a quick catch up with @mattbrailsford on Monday to discuss his setup, not intending to keep discussions behind closed doors rather because it was expedient to do so.

Matt had some excellent points which are as follows.

1. Vendr has a default order number generator which is commonly replaced by users to meet their business requirements and as such ordering the calls for AddUnique to the ServiceCollection is important.
2. Vendr makes use of CollectionBuilders to compose event pipelines and again ordering here is important.

On the first point inspecting the ServiceCollection in the UmbracoBuilder and conditionally registering should work for most scenarios.

• If a user has already added a order number generator one will not be added.
• If a user adds a order number generator after this composer with AddUnique then the default will be replaced.

    public class MyEcommerceComposer : IComposer
    {
        public void Compose(IUmbracoBuilder builder)
        {
            if (builder.Services.Any(x => x.ServiceType == typeof(IOrderNumberGenerator)))
            {
                return;
            }

            builder.Services.AddUnique<IOrderNumberGenerator, DefaultOrderNumberGenerator>();
        }
    }

It's worth noting that if a user is able to implement the IOrderNumberGenerator interface here it is also seems likely that they could easily order calls in Startup.ConfigureServices.

Composers are intended for low code setups and for more complicated technical packages there will be times where it makes more sense for users to just write some code, however an argument can be made that another package developer might like to add an extension to Vendr e.g. Our.Vendr.SomeERPVendor with low code support.

---

On the second point, during previous discussions I have to admit I hadn't considered package developers own CollectionBuilders.
Here I start to lose faith a little, and perhaps the crusade to clean up the dependency injection story is going too far.

I personally have an issue with the name of the class Composers which composers Composers (implementations of IComposer) as it makes communication harder than it needs to be, but a rename here could resolve this along with perhaps a little cleanup of the way state is managed inside the class.

On the flip side I can argue that those who wish to have fine grained control over the order in which pipelines are constructed should be doing so in code where they have absolute control over ordering in a more transparent way than searching for composer implementations in various packages which may alter the setup.

To handle collection builder setup in low code environments I have two possible workarounds.

1. Making use of the weighted collection builder, allow users to add before, after or in the middle of a pipeline by setting a weighting that will achieve their goals.
2. Separating service registration from collection builder setup, it would be possible to fire an event at startup time to allow users to add types which have already been registered in the container to a mutable CollectionBuilder (which has also been registered into the container)

Any feedback on the above would be sincerely appreciated.

[mattbrailsford]

Honestly, I'm not really a fan of any of the suggested workarounds. I really don't want to have to perform checks before adding every overridable service (the order number generator was one example, we have many overridable services). Could that not just be something that is coded into / an override to AddUnique?

I still believe there to be an issue with "weighted" collection builders. There are explicit InsertBefore and InsertAfter calls which could be tracked, but there are also methods Insert and Append which assume some initial order (appending after Vendr has run yields a different result to appending before Vendr has run). It may be possible in some way to weight the overall collection builder call

composer.WithCollectionBuilder<MyCollectionBuilder>(weight: -100)
    .Append<MyCollectionItem>();

... but I'm never really a fan of weight numbers as they are just "magic".

I don't like the second option either splitting the registration from the ordering as it promotes a level of duplication and would be pretty easy to mess up.

As mentioned on our chat, I believe all of these approaches are ultimately making it so the Umbraco API works one way for Umbraco (which can safely guarantee it runs first) compared to package devs using those same patterns (who can no longer guarantee order of registration and so now need to jump through hoops to work with the Umbraco API's in a different way).

If ComposeAfter and ComposeBefore allow consistency, I'd much rather keep those. I agree people should be encouraged to use the new UseXxx methods and we will absolutely document this as the right approach, but while composers are around and an option, I think we should keep what is necessary to maintain guaranteed order and thus code can work the same for both methods.

[p-m-j]

Thanks everyone for contributing to the discussion!

I think it’s clear that making these changes would cause unnecessary pain for package developers and so it will be parked until a better solution can be found, perhaps something like this #8653 (comment) from @lars-erik which looks like a great idea but probably isn’t feasible for v9 release any more but could make its way into v10.

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>()

---

Whilst there don’t seem to be any major concerns with the loss of IUserComposer I think it makes sense to mark it as obsolete and remove in a future version as it doesn’t cause any harm lingering a little longer.