Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
glennawatson opened this issue Aug 6, 2018 · 20 comments
Open

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

glennawatson opened this issue Aug 6, 2018 · 20 comments

Comments

@glennawatson
Copy link
Contributor

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
Copy link

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
Copy link
Contributor Author

glennawatson commented Aug 6, 2018

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
Copy link

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
Copy link

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
Copy link
Contributor Author

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
Copy link
Member

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

@glennawatson
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

glennawatson commented Aug 6, 2018

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
Copy link
Member

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
Copy link

vector-man commented Aug 23, 2018

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
Copy link
Contributor Author

@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
Copy link

Slesa commented Nov 18, 2018

@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
Copy link
Contributor Author

glennawatson commented Nov 18, 2018

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
Copy link

Slesa commented Nov 18, 2018

Thanks a lot.

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

@Slesa
Copy link

Slesa commented Nov 18, 2018

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
Copy link

vector-man commented Jun 17, 2019

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
Copy link
Member

RLittlesII commented Jun 18, 2019

Locator.SetLocator(new MyCustomLocator())

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants