reactiveui / rfcs

RFCs for changes to ReactiveUI
https://reactiveui.net/rfcs
5 stars 5 forks source link

RFC: How to support DI containers other than Splat #14

Open glennawatson opened 6 years ago

glennawatson commented 6 years ago

How to support DI containers other than Splat Summary Users are finding one DI container a pain point. How should we handle more than one DI container while allowing Splat to be the default.

Motivation Users tend to have their own DI engine, and we've had several confused users through the years. This came out of #13

Discussion What should be our approach to allow it? Should we allow it?

nesterenko-kv commented 6 years ago

The idea could be the same as in Caliburn.Micro which comes with pre-bundled DI-container and allows switch it to any of existing containers. It uses Func factories to resolve dependencies.

glennawatson commented 6 years ago

Well in theory we do already support different DI containers

You can swap inside the Locator class:

       /// <summary>
        /// Convenience property to return the DependencyResolver cast to a
        /// MutableDependencyResolver. The default resolver is also a mutable
        /// resolver, so this will be non-null. Use this to register new types
        /// on startup if you are using the default resolver
        /// </summary>
        public static IMutableDependencyResolver CurrentMutable {
            get { return Current as IMutableDependencyResolver; }
            set { Current = value; }
        }

So you would provide an adapter to your preferred DI.

What does Caliburn Micro do differently that would make it easier to interact with your preferred DI?

nesterenko-kv commented 6 years ago

This does not negate the fact that ReactiveUI refers to Splat. Talking about Caliburn.Micro - It has built-in support for composition root aka BaseBootstrapper

KallynGowdy commented 6 years ago

I think part of the issue is the large amount of "magic" that RxUI has during initialization.

Sure you can implement your own IMutableDependencyResolver, but my guess is that most people don't know that can work. Instead, many will just assume RxUI doesn't let them change their DI provider simply because they don't know how DI works in RxUI.

For example, as a RxUI consumer, I have no way to control how RxUI initializes itself. Instead, I need to trust that all of the RxUI services have been registered properly via the static constructor on RxApp.

If I don't reference RxApp in my app, then it's possible I'm missing some RxUI services in my DI container. Alternatively, if I reference RxApp after I've setup some custom dependencies (like a custom IActivationForViewFetcher) then RxApp will just siliently replace my services.

In most scenarios I think the current system works fine, but I would like to see an API that encourages developers to manually initialize RxUI so these sort of issues are easier to track down.

It doesn't need to be complicated, maybe just something like RxApp.Initialize(). Something that is easy to document while at the same time indicating where RxUI's "main" function happens.

Maybe that API can also include convienence options so it is obvious where to go when you want to customize RxUI's behaviour.

e.g:

var rxAppOptions = new RxAppOptions();
rxAppOptions.DependencyResolver = new MyCustomDependencyResolver();
RxApp.Initialize(rxAppOptions);

Also (and this is really just from an engineering perspective), I'm pretty sure there's also an opportunity here to convert RxApp into a normal class so that multiple RxApp instances can exist in a single AppDomain. Of course, there's really only one practical reason to want this, which is multi-threaded unit testing. But that would just be icing on the cake, it's not really relevant to this conversation.

glennawatson commented 6 years ago

A bit of the magic kinda has to exist due to different platforms needing different schedulers and also for testing.

I think it's been compounded more in recent versions due to increasing our nuget package space for different Platforms, and it not being transparent to the user if they failed to add that package that something is wrong.

RLittlesII commented 6 years ago

Added an issue to the website repository to document this. I actually just added Autofac using this technique last week.

glennawatson commented 6 years ago

I think this is a two step approach I guess. Maybe use a extension method approach like asp net core for rxapp initialisation or similar helpers.

Short term and long term we should act the referenced documentation task from Rodney

RLittlesII commented 6 years ago

I think we should go to the drawing board on this. @KallynGowdy brings up good points about the registration lifecycle. I explicitly still need to use Locator.Current in some manner because IViewLocator has need of it (if I am remembering correctly).

The following methods are not intuitive, I personally strayed away from them because I didn't understand them.

InitializeSplat();
InitializeReactiveUI();

I agree with you @glennawatson if this is a feature people want, we should come up with a long term strategy and get it on our road map. But we shouldn't halt documentation of what currently works. We should note in the documentation the limitations, and express our interest in making it more user friendly.

glennawatson commented 6 years ago

Was talking to Rodney offline so here are some of my thoughts.

I like the idea of RxUIOptions due to the fact we can have registrations happen in any order and not have registration side effects.

I'd like some standard extension methods on those to make it more obvious what's going on.

Eg

RxUIOptions options = new RxUIOptions();
options.UsePlatformSchedulers(); 
options.DependencyResolver = new AutofacResolver();

RxApp.Start(options);

So the above with RxUIOptions because you register the dependency resolver after the UsePlatformSchedulers() it won't matter. That won't happen until we hit the RxApp.Start();

Also be useful just to have a simple "as current" type option, which would register all the current settings we do by automagic, which should be simply as easy as:

RxApp.Start();

I think the other thing we consider is having some premade templates for the different scenarios, other projects such as Polly/DynamicData have a really nice in-depth samples project with snippets they keep up to date.

Thoughts?

KallynGowdy commented 6 years ago

So instead of something like options.UseDefaults() we would add some additional verbosity by splitting options out into their own functions?

Like

RxUIOptions options = new RxUIOptions();
options.UsePlatformSchedulers();
options.UsePlatformBindingConverters();
options.UsePlatformActivationForViewFetcher();
// etc..

Presumably the equivalent to options.UseDefaults() is simply RxApp.Start()? So what's the difference between this:

RxApp.Start(new RxUIOptions());

and this:

RxApp.Start();

or even this:

RxApp.Start(null);

Is the first just an "empty" RxApp whereas the second is an RxApp with all of the default platform registrations? (and the last is an ArgumentNullException?) I think if that's the case we might want to look at something like RxApp.StartDefault() instead of RxApp.Start() just to help make the behavior less surprising.

Also, would we be able to call RxApp.Start() multiple times with different options? What sort of bugs would we run into if that was supported? I would like to be able to call Start() multiple times so I can use different options for different unit tests, but calling Start() twice in a normal application seems like something that we probably don't want to support.

glennawatson commented 6 years ago

StartDefault() sounds good.

.Start(null) l agree should be a exception I agree.

I have in the past had need to replace the standard view fetcher etc so having the ability to specify those like above seems like a good idea. Pick and choose as you go type thing if needed for a advance configuration.

In terms of .Start(new RxOptions()) I would imagine nothing registered? A nice warning to the user would be appropriate.

RLittlesII commented 6 years ago

I think these are great. Do either of you know all the various hooks for allowing a consumer of ReactiveUI override defaults? I think next steps are to get the list and start designing how to override the platform defaults, if it is not possible for that option already.

I think a RxApp.StartWith(RxOptions options) is an option. I wouldn't want to mess with the current public API surface if there isn't a need. That way the current default isn't broken when I upgrade to new versions. But if I am looking forward to this feature I will know to use the new Start method.

vector-man commented 6 years ago

Definitely need this. Was using a custom container, by setting Locator. Updating RUI broke it somehow (my main WinForms dialog was getting disposed after changing its visibility)

glennawatson commented 6 years ago

@vector-man want to lodge a issue to the main rxui GitHub page and we will see if we can help fix that to get you back up and running?

Slesa commented 6 years ago

@RLittlesII Do you still have the example with autofac? I don't know how to translate the IEnumerable GetServices() to _container.Resolve<IEnumerable>, as autofac does not recognize collections...

glennawatson commented 6 years ago

Check out here https://reactiveui.net/docs/handbook/dependency-inversion/custom-dependency-inversion

ReactiveUI is a composable, cross-platform model-view-viewmodel framework for all .NET platforms that is inspired by functional reactive programming which is a paradigm that allows you to express the idea around a feature in one readable place, abstract mutable state away from your user interfaces and improve improve the testability of your application.
Slesa commented 6 years ago

Thanks a lot.

Grrr... I already was on that page, but did not notice it.

Slesa commented 6 years ago

The problem is, that you update the container's registration, but the update function is obsolete. "You should consider the registration as immutable". Also, the calls to

            resolver.InitializeSplat();
            resolver.InitializeReactiveUI();

are not recognized. Is it possible to use the ContainerBuilder somehow? I guess this is only possible if registration and resolving are separated.

vector-man commented 5 years ago

So, I was using this to add a custom DI, but it no longer works, as Locator.Current is now read only:

Locator.Current = new GriffinDependancyResolver(container);

Any idea on how I can update that line with the newest release?

RLittlesII commented 5 years ago

Locator.SetLocator(new MyCustomLocator())