Simplify service registration and use MSDI abstractions throughout. #8653
Comments
|
I'm sorry to say I haven't been able to look much at the core port yet. Hopefully I suddenly will. |
|
I do believe this would mostly still just work: (tho I dunno if I ran them before last commit. 😇) |
|
I say kill |
|
Not sure about |
|
At no point should there need an explicit concrete reference to IMO all you should ever care about is consuming // Startup.cs ConfigureServices
IUmbracoBuilder builder = services.AddUmbraco(this.IConfiguration);
// Startup.cs Configure
app.UseUmbraco(...); |
|
Abstraction miss fixed. 👼 (A bit sluggish on the APIs atm. Still mostly hacking framework stuff with V8.) |
I don't follow you here. The Perhaps you mean |
|
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:
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:
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! |
I do not understand this sentiment. You're already taking dependencies on all the That said.... Looking at the codebase (Infrastructure) it looks like you are creating your own container instance and then passing that instance around via
This is where you will never win me over. It's so overly complicated and unnecessary. There are already abstractions available for registration via |
|
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'.
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. |
|
Hey @Shazwazza happy to make time for a chat. Just concerned the conversation will be lost to the public. Regarding
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. |
|
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) |
|
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:
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 :) |
|
@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. 👼 |
|
@Shazwazza That works for me, looking forward to it! |
|
@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 Join Zoom Meeting Meeting ID: 884 4611 2341 Dial by your location |
|
@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! |
I'd love to sit in on this, is this open to all? |
|
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 Join Zoom Meeting Meeting ID: 856 1704 8741 Dial by your location |
|
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
|
|
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 Most of the methods and extensions on 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 While glancing over that interface now, I'm also reminded about the Looking forward to see what @rustybox comes up with! 🤘 |
|
I created The signature looks like this. public static IServiceCollection AddServiceFor<TService, TImplementation, TTarget>(
this IServiceCollection serviceCollection,
ServiceLifetime serviceLifetime,
ServiceLifetime targetLifetime)
where TImplementation : class
where TTarget : class |
|
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. |
|
RuntimeTests.Boot_Core_Runtime passed -- Was a bit concerned I might have lost my mind before I saw that message. |
I think RegisterFor is pretty much what we talked about. I think usage for filesystems were left as an intention. ;)
🤪🤘👍 (GJ) |
|
Any ideas why I might get the following during Setup in
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) |
|
Presumably the mandate not to change everything stems from |
|
I have had 2 epiphanies before my morning coffee.
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. |
|
Hi all (slowly getting back), amazing thread btw and a ton of great ideas and discussion 👏 Some quick answers to some things:
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
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.
I don't see any reason why they need to terminate in reverse order. I have no idea why
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.
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 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.
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
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:
I think that covers everything being talked about?
Please no, this thread has come so far, I don't think there's much more to go. 🙏 |
|
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: |
In theory, however not with mediatr, with Udi's sample you can add an event handler without making use of the container, 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. |
|
In summary.
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.
Outstanding question, is Umbraco.Web.Common.Builder.UmbracoBuilder intended to replace Composition or did you expect them both to exist?
Would this go on https://github.com/umbraco/UmbracoDocs? v9 branch? |
Yes IIRC (sorry still getting back into the groove of things and trying to remember everything). The example that @JimBobSquarePants provides above #8653 (comment) is along the lines of I think where we want to go. In above discussions like: #8653 (comment) we said
I know we have the generic ability to There's a few of other questions you had above in relation to some of this too
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."
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.
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:
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:
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 |
ExamplesRegister servicesIn 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 packageAt 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? |
|
Replacing composition with UmbracoBuilder and removing CoreComposers isn't too bad, it's just boring and begging for conflicts. 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
|
|
The state wouldn’t be a constructor parameter. Any requires information should be passed to the handler via Handle() |
|
So RuntimeLevel would live as a prop on UmbracoApplicationStarting, sure. Edit: updated example to match |
|
Yep... In startup I would always recommend using the var builder = services.AddUmbraco();
builder.SetFileSystem<MyFileSystem>();Though |
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.
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?
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
I think this needs to be further defined, something like:
// 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 :) |
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).
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.
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. |
|
@Shazwazza I think |
|
@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? |
|
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) |
|
LEGEND! |
|
It's gone, the crazy func is gone!!! Only calls to BuildServiceProvider are in Test projects (and Umbraco.Web but that doesn't count) |
|
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? |
|
Agreed. I'll have a look at raising a specific issue. |
|
Oh and collection builder review issue? |
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.
#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
LightInjectContaineris 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.
The text was updated successfully, but these errors were encountered: