-
Notifications
You must be signed in to change notification settings - Fork 51
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
Conversation
- 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.
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. |
@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. |
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 |
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👌 |
@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. |
@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: 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. And just with added ignoring, scoped bindings become decoration ready. |
@nilpunch [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);
} |
@nilpunch Can you reach me on discord? |
@gustavopsantos yep, that's good idea |
@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 |
@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. |
@nilpunch Hmm I see, as AddDecorator is a method from ContainerDescriptor, it decorates all currently binded services without knowing to decorate a specific binding. 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? |
@gustavopsantos Unfortunately, in my case with effects, some class that depends on effect do not depends on 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 [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. |
@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);
} |
@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. |
@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. |
@gustavopsantos Maybe we should start by formulating the requirements that our goal design must satisfy. I have some on my mind:
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 |
e394db8
to
35d188b
Compare
@nilpunch Do you have any plans to continue on this topic of adding decoration support as a plugin as we discussed? |
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:
concrete types without polluting parent container with this extra dependencies.
P.S. sorry for unrelated README.md and package.json changes in diff.
Type of change
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:
Test Configuration:
Unity 2021.2.3