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

Feature: Add common DI engine adapters. #233

Closed
5 of 7 tasks
glennawatson opened this issue Jan 19, 2019 · 17 comments
Closed
5 of 7 tasks

Feature: Add common DI engine adapters. #233

glennawatson opened this issue Jan 19, 2019 · 17 comments

Comments

@glennawatson
Copy link
Contributor

glennawatson commented Jan 19, 2019

Is your feature request related to a problem? Please describe.
Add new NuGet packages for different DI engines, such as Autofac, SimpleInjector etc.

Describe the solution you'd like
Add under a NuGet Package scheme, such as Splat.DependencyInjection.SimpleInjector or something.

Describe suggestions on how to achieve the feature
A clear description to how to achieve the feature.
Gives users the chance to use popular DI engines.

  • Autofac
  • Ninject
  • Structure Map
  • Simple Injector
  • Prism
  • DryIoc
  • Microsoft.Extensions.DependencyInjection

Relates to reactiveui/rfcs#14

@glennawatson
Copy link
Contributor Author

This relates to a conversation started in the reactiveui/website project. Instead of documenting the different approaches we probably should just provide adapters.

@RLittlesII
Copy link
Member

Structure Map has been sunset

@chuuddo
Copy link

chuuddo commented Jan 22, 2019

SimpleInjectorDependencyResolver not works in real apps.
I made a pull request #239 with changes to View and ViewModel classes to start initialization of RxApp and all tests fails.

After some investigations i found this issues:

  1. The container is locked after the first call to resolve but ReactiveUI invoke all registrations after RxApp.EnsureInitialized(). This can be fixed by not getting instace of MainWindow from container. If there are no other solutions, we should add this workaround to documentation.
    //var window = (MainWindow)container.GetInstance<IViewFor<MainViewModel>>();
    var window = new MainWindow();
    window.ViewModel = container.GetInstance<MainViewModel>();
    window.Show();
  1. SimpleInjector support multiple registrations for same interface only through Collection.Register. We can't register this lines using _container.Register() method.

_container.Register(serviceType, factory);

I don't have any workarounds for second issue. Can we use this DI container with ReactiveUI?

@glennawatson
Copy link
Contributor Author

@chuuddo Thanks for the comments. Might be worth moving this one into it's own issue. Also if you can maybe provide a small project with the issue it'll like Rodney's life easier.

@glennawatson
Copy link
Contributor Author

@chuuddo Looks like you've done better than a small demo :) But the issue may be better to link to yourPR.

@chuuddo
Copy link

chuuddo commented Jan 22, 2019

@glennawatson i added my comment to PR.

@chuuddo
Copy link

chuuddo commented Jan 22, 2019

Just looked at Autofac implementation.
Autofac container is also immutable after build.
So, we can't use ReactiveUI with immutable DI containers without resolving reactiveui/ReactiveUI#1737.
Or use mutable DI containers like DryIoc, Ninject.

@RLittlesII
Copy link
Member

Thanks for the Simple Injector catch. It may not be compatible with ReactiveUI, I'll look at the findings and see if we can find a reasonable way to resolve it.

Im confused why muttability matters? As long as you aren't registering elements after the container is built there shouldn't be any issue using Autofac.

There is a link in the Autofac implementation explaining that you would have to explicitly rebuild the container.

This puts the onus on the developer to know that they are deviating from the frameworks in ended use, and that they have to manage it themselves.

Ninject is on the list of implementations, we just haven't gotten to it yet. I can add DryIoc.

Thanks again for the detail!

@chuuddo
Copy link

chuuddo commented Jan 22, 2019

Im confused why muttability matters? As long as you aren't registering elements after the container is built there shouldn't be any issue using Autofac.

Mutability matters because of current implementation of ReactiveUI initialization.
We build Autofac container before RxApp.EnsureInitialized(), thats why we cant register all RxApp stuff without builder.Update(container), but that method is obsolete.

And it looks like this method missed in current AutofacDependencyResolver implementation.

public virtual void Register(Func<object> factory, Type serviceType, string contract = null)
{
var builder = new ContainerBuilder();
if (string.IsNullOrEmpty(contract))
{
builder.Register(x => factory()).As(serviceType).AsImplementedInterfaces();
}
else
{
builder.Register(x => factory()).Named(contract, serviceType).AsImplementedInterfaces();
}
}

@RLittlesII
Copy link
Member

RLittlesII commented Jan 22, 2019

I plan on migrating an app to this implementation this week. If I have issues, I will update this implementation.

I am explicitly calling the Splat and ReactiveUI initialization before I assign the container to splat. I haven't had any trouble with it so far.

@chuuddo
Copy link

chuuddo commented Jan 23, 2019

Provide another repo with some tests. Hope it will be useful.
https://github.com/chuuddo/SplatDependencyResolversTests

GitHub
Contribute to chuuddo/SplatDependencyResolversTests development by creating an account on GitHub.

@worldbeater
Copy link
Contributor

Adding support for Microsoft.Extensions.DependencyInjection as well might be a good idea.

@glennawatson
Copy link
Contributor Author

hey @chuuddo -- the static locator is bad in tests, we have a fix coming in #282 which makes the Locator available as a class for testing. If you want to do a PR with your suggested tests feel free to pull a new PR after #282 has been merged.

@chuuddo
Copy link

chuuddo commented Feb 23, 2019

@glennawatson i can make a PR, but in #274 you want to keep RxUI out of Splat and i want to clarify do we need to add this tests at all.

@glennawatson
Copy link
Contributor Author

Any remaining tasks with this one @RLittlesII

@RLittlesII
Copy link
Member

@glennawatson I believe you implemented the Microsoft.Extensions.DependencyInjection. That just leaves Prism. I would be fine closing this issue and opening one up specifically for Prism DI, and let the community implement it if they determine they want it. At this point we have enough examples of how to do it, should be a good first issue for someone.

@glennawatson
Copy link
Contributor Author

Sounds good, mark it as first timer with a label I guess.

@lock lock bot added the outdated label Oct 13, 2019
@lock lock bot locked and limited conversation to collaborators Oct 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants