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

Add non-generic RegisterInstance overload #695

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Valax321
Copy link

This pull request simply adds a new overload for RegisterInstance that allows the caller to specify the type of the instance as a parameter without generics. This matches the behaviour of other registration overloads that allow passing the type at runtime.

This behaviour is useful when registering multiple items from a list when we can't know at compile-time what types may appear in the list. For instance, in my game each game state can register arbitrary ScriptableObjects that are assigned in the inspector:

foreach (var data in m_GlobalData)
 {
    builder.RegisterInstance(data, data.GetType());
 }

The implementation is nearly identical to the generic version but uses the method parameter implementationType instead of a generic type parameter in the As() call.

This allows for registering things automatically when we don't know the type at compile time.
Copy link

vercel bot commented Jul 25, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
vcontainer ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 22, 2024 3:23am

@hadashiA
Copy link
Owner

hadashiA commented Jul 25, 2024

Thank you for thePR.

I think this is fine on its own, but overall, it seems to deviate from the API consistency point of view, as it is aligned with .As(Type) when passing the type.

I don't think .As() is the best way to go at the moment, but I'm trying to be consistent, so I think it would be better to be able to pass Type as an argument for all registers if I were to make such a change.
But I think it's a bit late now.
If you have a strong reason why it would be better to give special treatment only to RegisterInstance, please let me know.

@Valax321
Copy link
Author

Sorry it took me a while to get back to this, I've been moving and haven't had much time recently.

The issue I had with using .As() is that the generic version of RegisterInstance() assumes you to want register the object as the type you pass in TInterface at minimum in addition to any As() calls, which was behaviour that didn't work in my case.

[MethodImpl(MethodImplOptions.AggressiveInlining)]
        public static RegistrationBuilder RegisterInstance<TInterface>(
            this IContainerBuilder builder,
            TInterface instance)
            => builder.Register(new InstanceRegistrationBuilder(instance)).As(typeof(TInterface)); // No good for me!

In my case I am registering objects from a list where I only know the base class (which is ScriptableObject in the case of my original PR message, from an inspector-editable list), but want to register them only as their actual type, not ScriptableObject as well.

My project's use case is maybe fairly unusual (I'm registering lots of things assigned by inspector rather than through code and already had to work around a couple of API limitations already) but I couldn't find a way to go about this through the public API, so if you think this behaviour should be exposed or named differently I am happy to rework it.

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

Successfully merging this pull request may close these issues.

2 participants