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

Individual scope for each binding, allowing decoration #42

Closed
wants to merge 3 commits into from

Conversation

nilpunch
Copy link

@nilpunch nilpunch commented Nov 2, 2023

Pull Request Template

Description

I just want to share a simple design that allows you to adjust the scopes of each individual binding, and thus allow to use patterns such as decorator and composite and also to add specific dependencies to concrete types without polluting container with them.

I think this might be interesting to you. I am very grateful for the simplicity of your current API and it's internals, please keep it that way :)

I've added no much, but it's pretty versatile:

  • Add extend scope for binding, allowing bind some specific dependencies to
    concrete types without polluting parent container with this extra dependencies.
  • Add IgnoreParentBind method. With the change above, it allows to solve cyclic reference injection, which occures where you want to use decorator or composite patterns.

P.S. sorry for unrelated README.md and package.json changes in diff.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

I have a project, that requires creating different pickup's with some gameplay "effects" on them. And I want to define all effects with some configs, such as ScriptableObject.

It is working well so far. With this setup, I can create big sandwiches of many different effects, and even nest multiple MultiEffectConfig with itself in Editor.

Some context:

// Concrete effect config
[CreateAssetMenu]
public class SpeedChangeEffectConfig : EffectConfig
{
	[SerializeField] private float _speedDelta = 5f;
	[SerializeField] private float _duration = 5f;
	
	public override void BindSettings(ContainerDescriptor descriptor)
	{
		descriptor.AddSingletonExtend("SpeedChangeEffectScope", scopeDescriptor =>
		{
			// This bind will affect ONLY this extended scope for SpeedChangeEffect type
			scopeDescriptor.AddInstance(new SpeedChangeEffect.Args(_speedDelta, _duration));
		}, typeof(SpeedChangeEffect), typeof(IEffect));
	}
}

// Composed effect config
[CreateAssetMenu]
public class MultiEffectConfig : EffectConfig
{
	[SerializeField] private EffectConfig[] _effectConfigs;
	
	public override void BindSettings(ContainerDescriptor descriptor)
	{
		descriptor.AddSingletonExtend("MultiEffectScope", scopeDescriptor =>
		{
			// If we don't add this, then MultiEffect from parent scope will be counted as IEffect in this scope,
			// and so cause loop on itself, because we are injecting IEnumerable<IEffect> into MultiEffect.
			scopeDescriptor.IgnoreParentBind(typeof(MultiEffect), typeof(IEffect));
			
			foreach (var effectConfig in _effectConfigs)
				effectConfig.BindSettings(scopeDescriptor);
		}, typeof(MultiEffect), typeof(IEffect));
	}
}

// Just a classic composite
public class MultiEffect : IEffect
{
	private readonly IEnumerable<IEffect> _effects;
	
	public MultiEffect(IEnumerable<IEffect> effects)
	{
		_effects = effects.ToArray();
	}
	
	public void Apply()
	{
		foreach (var effect in _effects)
			effect.Apply();
	}
}

// Some use case
IEffect CreateEffect(EffectConfig effectConfig)
{
	using var scope = _container.Scope("CustomEffectScope", effectConfig.BindSettings);
	return scope.Construct<IEffect>();
}

Test Configuration:
Unity 2021.2.3

nilpunch and others added 2 commits November 2, 2023 19:20
- Add extend scope for binding that allows bind specific stuff to
concrete types without polluting container with extra dependencies.
- Add IgnoreParentBind method, with the change above it allows to use
patterns such as decorator and composite.
@gustavopsantos
Copy link
Owner

Hello @nilpunch, sorry for the big delay, I have a lot to do recently on my job plus im moving to a new apartment, so at the end of the day im totally destroyed xD.
Somebody already requested ability to add decorators, so your changes are very welcome, I dont have time to test it now, but I will for sure once I have as its a already requested feature. So thanks so much for you contribution.

@nilpunch
Copy link
Author

@gustavopsantos Of course, take your time. You'll need sharp mind for this one ;)

I will be happy to continue discussion on the feature, and I can share more interesting real-world use cases, if needed.

@gustavopsantos
Copy link
Owner

Hello @nilpunch I just started reviewing it finally xD. Do you mind sharing a tiny, but complete use case of the decorator feature you implemented? Because from your example above idk whats happening behind EffectConfig, SpeedChangeEffect.Args and IEffect

@nilpunch
Copy link
Author

Hello @gustavopsantos

The first sample hardly can be described as "tiny, but complete", that's for sure. I just slapped the code straight out from the pet project to the PR template, so that the Testing section is not empty. Not that it's in any way useful, just like, "Here the code, it's real, I am not crazy".

So yeah, I need some time and there will be quality use case👌

@gustavopsantos
Copy link
Owner

@nilpunch A more complete example will not be necessary, I saw that microsoft DI solution also now accepts this type of pattern implementation and was easy to understand the expected behaviour. However I do not want to use scoped containers to do so, its more like a workaround because actual implementation would cause cyclic dependency resolution, thus StackOverflow.
Im gonna start changing a bit internals to allow this change but also other future changes to come, thank you very much for the idea, im gonna post here once everything is ready.

@nilpunch
Copy link
Author

nilpunch commented Nov 30, 2023

@gustavopsantos I didn't know about microsoft DI solution. Just researched it on my own. Main goals were reuse as much code as possible and maintain order-independency of the bindings.

If you come up with something simpler that allow to use patterns like adapters, decorators, composites - that will be great. What will also be great, if you can add multiple layers of decoration to one type. Like:
var megaBonus = new MultipliedBonus(new RandomizedBonus(new Bonus()));

But imo, scoped bindings is a cool feature on it's own. You can bind really complex things with it. Main use case - bind multiple types, that require different parameters of the same type.
With your current design, how do you bind different float parameters to multiple types? Personally, I use factories for this case. But it is manual resolving at runtime. I want to be able to just bind them on the binding stage.

And just with added ignoring, scoped bindings become decoration ready.

@gustavopsantos
Copy link
Owner

@nilpunch What will also be great, if you can add multiple layers of decoration to one type
This is exactly what I want to be able to achieve, something that will allow us to do the following:

        [Test]
        public void Decorate_CanNestDecorators()
        {
            var container = new ContainerDescriptor("")
                .AddSingleton(new Number(10), typeof(INumber))
                .Decorate(typeof(DoubledNumber), typeof(INumber))
                .Decorate(typeof(DoubledNumber), typeof(INumber))
                .Decorate(typeof(HalvedNumber), typeof(INumber))
                .Decorate(typeof(HalvedNumber), typeof(INumber))
                .Decorate(typeof(DoubledNumber), typeof(INumber))
                .Build();

            var number = container.Resolve<INumber>();
            number.Get().Should().Be(20);
        }

@gustavopsantos
Copy link
Owner

@nilpunch Can you reach me on discord?

@nilpunch
Copy link
Author

nilpunch commented Dec 1, 2023

@gustavopsantos yep, that's good idea

@gustavopsantos
Copy link
Owner

@nilpunch I've added decoration support on main branch, note that I havent done a release yet as I want to add some more changes, but you can already use this version on your project by replacing the package version by main at the package git url

@nilpunch
Copy link
Author

nilpunch commented Dec 5, 2023

@gustavopsantos In current implementation, as I understand, the behavior is not well defined for the decoration of several binds of the same type, and I can't do something like:

        [Test]
        public void Decorate_CanNestDecorators()
        {
            var container = new ContainerDescriptor("")
            
                .AddSingleton(new Number(10), typeof(INumber))
                .AddDecorator(typeof(DoubledNumber), typeof(INumber))
                .AddDecorator(typeof(DoubledNumber), typeof(INumber))
                
                .AddSingleton(new Number(8), typeof(INumber))
                .AddDecorator(typeof(HalvedNumber), typeof(INumber))
                .AddDecorator(typeof(HalvedNumber), typeof(INumber))
                .Build();

            var numbers = container.All<INumber>();
            
            numbers.Should().ContainEquivalentOf(40);
            numbers.Should().ContainEquivalentOf(2);
        }

For now, that's a deal breaker to me. And I know it's a matter of refactoring iterations, but I can't see how current implementation can easily achieve this.

@gustavopsantos
Copy link
Owner

@nilpunch Hmm I see, as AddDecorator is a method from ContainerDescriptor, it decorates all currently binded services without knowing to decorate a specific binding.
So in this case Number(10) will be decorated 4 times, while Number(8) will be decorated 2 times, so All would return [10, 2] in this case.

Microsoft way of solving this was adding an Key to a dependency, like Zenject did too, but I think on Zenject was called Tag or something else. But I dont really think this is a good way to solve, as probably the project will start to have a lot of even more implicit ways of finding its dependencies.

Maybe what you can do with currently implementation, as you were already using multiple scopes, is doing something like this for your effect provider.

        [Test]
        public void Decorate_ShouldBeAbleToNestDecorations3()
        {
            var numberProviders = new List<Container>
            {
                new ContainerDescriptor("X")
                    .AddSingleton(Number.FromValue(10), typeof(INumber))
                    .AddDecorator(typeof(DoubledNumber), typeof(INumber))
                    .AddDecorator(typeof(DoubledNumber), typeof(INumber))
                    .Build(),

                new ContainerDescriptor("Y")
                    .AddSingleton(Number.FromValue(400), typeof(INumber))
                    .AddDecorator(typeof(HalvedNumber), typeof(INumber))
                    .AddDecorator(typeof(HalvedNumber), typeof(INumber))
                    .Build(),

                new ContainerDescriptor("Z")
                    .AddSingleton(Number.FromValue(12), typeof(INumber))
                    .AddDecorator(typeof(DoubledNumber), typeof(INumber))
                    .AddDecorator(typeof(HalvedNumber), typeof(INumber))
                    .Build()
            };

            var numbers = numberProviders.Select(np => np.Single<INumber>());
            //var numbers = numberProviders.SelectMany(np => np.All<INumber>()); // All<T> works too, but you have to use SelectMany then
            numbers.Select(n => n.Get()).Should().BeEquivalentTo(new int[] {40, 100, 12});
        }

What do you think? Its a good fit for your use case?

@nilpunch
Copy link
Author

nilpunch commented Dec 5, 2023

@gustavopsantos Unfortunately, in my case with effects, some class that depends on effect do not depends on List<Container> or IEnumerable<IEffect>. They are depends only on IEffect and doesn't worry about the details. Actual IEffect can be composite of many effects, which are also can be decorated in many ways, or be composites themselves.

This can be achieved with current approach, but not in a form of simple Binds and Resolve(). I don't really want to deal with it in its current state.

About keys for bindings. Decoration implementation already quite wordy, I'm afraid that adding some sort of keys to bindings will ruin simplicity even more. Also, working with the keys has always been hard and unintuitive, and this path has been taken by DI containers many times before. Maybe we can come up with something better?

What I can propose is to add some hierarchy to your decoration, so there will be no need for keying each binding to decorate it. Basically, each decorator will affect only types within some scope. And I can't come up with something not similar with scoped bindings, because it is what it is :)

With your current AddDecorator API scopes can look like this:

	[Test]
	public void Decorate_CanNestDecorators()
	{
		var numberOne = new ContainerDescriptor("1")
			.AddSingleton(new Number(10), typeof(INumber))
			.AddDecorator(typeof(DoubledNumber), typeof(INumber))
			.AddDecorator(typeof(DoubledNumber), typeof(INumber));

		var numberTwo = new ContainerDescriptor("2")
			.AddSingleton(new Number(8), typeof(INumber))
			.AddDecorator(typeof(HalvedNumber), typeof(INumber))
			.AddDecorator(typeof(HalvedNumber), typeof(INumber));
                
		var container = new ContainerDescriptor("Main")
			.BindFromScope(numberOne, typeof(INumber))
			.BindFromScope(numberTwo, typeof(INumber));
			.Build();

		var numbers = container.All<INumber>();

		numbers.Should().ContainEquivalentOf(40);
		numbers.Should().ContainEquivalentOf(2);
        }

P.S. in this design, number scopes do not inherit "parent" bindings, so there are no cyclic dependencies issues. But it is also means, that number scopes can't have all useful bindings from the "parent". This could be addressed later, if you will like this design at all.

@gustavopsantos
Copy link
Owner

@nilpunch Your design can already be achieved by the following, what do you think?

        [Test]
        public void Decorate_CanNestDecorators()
        {
            var numberOne = new ContainerDescriptor("1")
                .AddSingleton(Number.FromValue(10), typeof(INumber))
                .AddDecorator(typeof(DoubledNumber), typeof(INumber))
                .AddDecorator(typeof(DoubledNumber), typeof(INumber))
                .Build();

            var numberTwo = new ContainerDescriptor("2")
                .AddSingleton(Number.FromValue(8), typeof(INumber))
                .AddDecorator(typeof(HalvedNumber), typeof(INumber))
                .AddDecorator(typeof(HalvedNumber), typeof(INumber))
                .Build();

            var container = new ContainerDescriptor("Main")
                .AddSingleton(numberOne.Single<INumber>(), typeof(INumber))
                .AddSingleton(numberTwo.Single<INumber>(), typeof(INumber))
                .Build();

            var numbers = container.All<INumber>().Select(n => n.Get());

            numbers.Should().ContainEquivalentOf(40);
            numbers.Should().ContainEquivalentOf(2);
        }

@nilpunch
Copy link
Author

nilpunch commented Dec 5, 2023

@gustavopsantos In fact, this is what I came to in my first attempt, when I used factories. And then I realized, that I don't want to introduce resolving at the binding stage. It's escalating very quickly, and eventually you start to resolve everything manually. In this case, the whole point of the DI container is lost.

As a workaround it could work, but it doesn't satisfy all use cases, and require manual resolving, which is not very welcome in my opinion.

@gustavopsantos
Copy link
Owner

gustavopsantos commented Dec 5, 2023

@nilpunch I have been thinking here, but I cannot think nothing other than keying the descriptors, so when adding a decorator you would have the option to decorate a keyed singleton/transient.
However I dont like this solution, from my perspective it can bring more issues than solutions to devs, i'll continue thinking about solutions for this, but if you come with something in mind let me know

@nilpunch
Copy link
Author

nilpunch commented Dec 5, 2023

@gustavopsantos Maybe we should start by formulating the requirements that our goal design must satisfy.

I have some on my mind:

  1. Design should allow any kind of decoration.
  2. Binding should be order-independent. This will eliminate possible contradictions.
  3. Binds should be undoable. So, we don't have methods such as Unbind<T>, and you can't decorate type after it has been binded. This will eliminate side effects when binding with multiple installers, so no one can break our binds.

We need to really think about these, so that we have a solid foundation.

Keying actually meet 2nd statement, and maybe 1st. But fails on 3rd.

My first solution in PR with scopes meet 1st and 2nd, but fails on 3rd, because of IgnoreParentBind(). We can break binding by calling or not calling this method.
But, if all extend scope contracts will be ignored in a scope by default, then 3rd requirement will be satisfied, and we should have no instruments to break or modify our binds.

@gustavopsantos
Copy link
Owner

@nilpunch Do you have any plans to continue on this topic of adding decoration support as a plugin as we discussed?

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