YairHalberstadt / stronginject

compile time dependency injection for .NET
MIT License
845 stars 24 forks source link

IOwned<out T> #155

Closed jnm2 closed 2 years ago

jnm2 commented 2 years ago

It's been useful for me to pass things around as IOwned<object> that were originally obtained as Owned<SomeClass>. The benefit is that you can cast without allocating a new Owned<object> instance to wrap the Owned<SomeClass> instance, and without hundreds of calls to a custom .AsObject() or .Cast<SomeClass, object>() extension method. Could this be useful to other people?

Someone could try to use IOwned<X> and become confused if StrongInject's error seemed to be telling them to register IOwned<X>. StrongInject should either provide a specific error message telling them to switch to Owned<X> or injecting IOwned<X> should just work. It seems unlikely that requiring the user to change IOwned<SpecificType> to Owned<SpecificType> would ever pose a problem, but the "just works" option sounds the simplest to implement.

API

These interfaces would be added to the base type list on the corresponding types in https://github.com/YairHalberstadt/stronginject/blob/main/StrongInject/Owned.cs.

using System;

namespace StrongInject
{
    /// <summary>
    /// A disposable wrapper for an instance of <typeparamref name="T"/>.
    /// 
    /// Make sure to dispose this once you are done using <see cref="Value"/>. This will dispose <see cref="Value"/> and all dependencies of it.
    /// 
    /// Do not dispose <see cref="Value"/> directly as that will not dispose its dependencies.
    /// 
    /// Do not use <see cref="Value"/> after this is disposed.
    /// </summary>
    public interface IOwned<out T> : IDisposable
    {
        T Value { get; }
    }

    /// <summary>
    /// An async disposable wrapper for an instance of <typeparamref name="T"/>.
    /// 
    /// Make sure to dispose this once you are done using <see cref="Value"/>. This will dispose <see cref="Value"/> and all dependencies of it.
    /// 
    /// Do not dispose <see cref="Value"/> directly as that will not dispose its dependencies.
    /// 
    /// Do not use <see cref="Value"/> after this is disposed.
    /// </summary>
    public interface IAsyncOwned<out T> : IAsyncDisposable
    {
        T Value { get; }
    }
}
YairHalberstadt commented 2 years ago

I don't see a problem with this, and I think it should be fairly simple to implement - we just add the interface to Owned<T>, and add a check to InstanceSourcesScope to return a ForwardedInstanceSource wrapping and OwnedSource if it sees IOwned.

My only issue is I'm not convinced of the utility. This seems like a relatively niche use case, and there's a valid workaround.

I can see a separate advantage in that it allows other libraries to provide their own implementation of IOwned, but in practice I can't really see that happening - Owned takes a delegate in it's constructor so is pretty flexible.

jnm2 commented 2 years ago

The biggest reason I would like this is that I recently had to add 110 occurrences of .AsObject() into a project when moving from my own Owned type to using the one in the StrongInject library. I could mitigate this in a minority of the cases by making all the windowing system methods generic so they can directly take Owned<T> without the caller having to create a new Owned<object> themselves.

Hopefully it's not too hard to see why a windowing system deals heavily with Owned<object> rather than Owned<AbcView> and Owned<DefView>, and why menu items have Func<Owned<object>> properties. But obtaining these Owned<object> instances has to start with StrongInject returning Owned<AbcView> and Owned<DefView> from injected delegates. I don't see a way around the .AsObject() call syntax bloat without going back to the bad old days of having the windowing system dispose both views and view models by calling container.Release directly rather than disposing an Owned<object>. StrongInject doesn't provide such a method, so the mere ability to use StrongInject is the driver behind all of this.

image

Example of the syntax bloat imposed on callers since there's no implicit conversion from what the caller must have (Lease<SpecificViewModel>) and what the navigation service must deal with (Lease<object>):

navigationService.AddSiblingViewModel(this, baseFormulasViewModelFactory.Invoke(EditList.FocusedItem).AsObject());
YairHalberstadt commented 2 years ago

I see. I wouldn't implement this myself, but if you send a PR I'd be happy to review.

jnm2 commented 2 years ago

@YairHalberstadt As I'm working with the XML docs, I noticed that it looks like <para> blocks may have been intended:

image

With <para>:

image

I'm going ahead and making this change before basing IOwned<T> docs off of them, but just give a shout if that's not what you would have wanted.