reactiveui / splat

Makes things cross-platform
MIT License
967 stars 142 forks source link

Bug: The simple injector DI container isn't practical #240

Open glennawatson opened 5 years ago

glennawatson commented 5 years ago

As originally reported by @chuuddo, they have provided unit tests to help produce the exception here:

https://github.com/reactiveui/splat/compare/master...chuuddo:rxui_initialization_breaks_simpleinjector_resolver

Details of their explanation: 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();
  2. SimpleInjector support multiple registrations for same interface only through Collection.Register. We can't register this lines using _container.Register() method.

https://github.com/reactiveui/splat/blob/d27b0b48e71d38402c8e224b25e2dfff2daef875/src/Splat.SimpleInjector/SimpleInjectorDependencyResolver.cs#L38

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

Originally posted by @chuuddo in https://github.com/reactiveui/splat/issues/233#issuecomment-456362486

glennawatson commented 5 years ago

Again thanks for taking the time for letting us know about the issue @chuuddo

dpvreony commented 5 years ago

mental note: https://simpleinjector.org/ReferenceLibrary/?topic=html/Overload_SimpleInjector_Lifestyle_CreateProducer.htm

Simple Injector - Table of Content
dpvreony commented 5 years ago

doesn't seem to cater for contracts.

RLittlesII commented 5 years ago

There was a way to do named instances in Simple Injector. I can't remember how off the top. The library considers the concern a bad practice, but it still allows you to do it. I'll see if I can pull an example. But your right it isn't idea.

glennawatson commented 5 years ago

Worse case scenario we just do a intermeditory generic collection with a dict. Eg ContractContainer with extension methods for the simple injector side.

On Sat., 3 Aug. 2019, 8:23 pm Rodney Littles II, notifications@github.com wrote:

There was a way to do named instances in Simple Injector. I can't remember how off the top. The library considers the concern a bad practice, but it still allows you to do it. I'll see if I can pull an example. But your right it isn't idea.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/reactiveui/splat/issues/240?email_source=notifications&email_token=ABMQMMIM2YSS7QZHRTIAIALQCVMAXA5CNFSM4GRSDJG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3PL3XI#issuecomment-517914077, or mute the thread https://github.com/notifications/unsubscribe-auth/ABMQMMLPNKFVXUNFU5NKDDDQCVMAXANCNFSM4GRSDJGQ .

schnerring commented 3 years ago

Since using ReactiveUI + Splat, I find myself using the follwing (recommended) pattern over and over again:

public OAuthSettings(IConfiguration configuration = null)
{
    configuration ??= Locator.Current.GetService<IConfiguration>() ??
                      throw new ArgumentNullException(nameof(configuration));
    // ...
}

This bugs me because it's because of error-prone code duplication and the fact that this Splat pattern "bleeds" into every class using DI. It reminds me so much of MEF's [ImportingConstructor] attributes everywhere. Using SimpleInjector makes above pattern obsolete.

Using Splat.SimpleInjector makes Sextant unusable because RegisterParameterStackService makes a registration after resolving a service. From Stack Overflow:

Simple Injector locks the container after first use, and it does not allow the container to be unlocked. This behaviour is very deliberate and it is explained in detail here why making registrations after having resolved is a bad idea. Making registrations after you resolve is an anti-pattern that is sometimes referred to as Register Resolve Register (not to confuse with Register Resolve Release).

I wonder, is this just an issue with Sextant (should I open an issue?) or can the register-resolve-register pattern also be found in other ReactiveUI components?

Also, I find it hard to understand how SimpleInjectorInitializer works. Intuitively I tried registering to the SimpleInjector container using Splat's API. It took me a while to find out that SimpleInjectorDependencyResolver fails silently:

public void Register(Func<object> factory, Type serviceType, string contract = null)
{
    // The function does nothing because there should be no registration called on this object.
    // Anyway, Locator.SetLocator performs some unnecessary registrations.
}

I tried looking into SetLocator but I can't find these unnecessary registrations. Was this changed? If this function "shouldn't be called", a NotImplementedException, telling the developer to use the initializer instead, would be very much appreciated. I did this because I use the UseSerilogFullLogger() extension method and tried calling it after container.UseSimpleInjectorDependencyResolver(initializer).