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

New instance per dependency strategy added for type registration #16

Merged
merged 5 commits into from
Sep 19, 2019

Conversation

MKMZ
Copy link
Contributor

@MKMZ MKMZ commented Jun 26, 2018

@gasparnagy

BoDi currently does not allow to change a strategy of resolving objects. I am currently working on 2 projects that is using SpecFlow with BoDi and I have a problem when I was to have a class with isolated state. It is impossible without the disposal of current ObjectContainer and/or using a different IoC container.

I have added a new strategy which will create a new instance each time Resolve function is executed on ObjectCointainer.

Usage:

container.RegisterTypeAs<SimpleClassWithDefaultCtor, IInterface1>().InstancePerDependency();

var obj1 = (SimpleClassWithDefaultCtor) container.Resolve<IInterface1>();
var obj2 = (SimpleClassWithDefaultCtor) container.Resolve<IInterface1>();
Assert.AreNotSame(obj1, obj2);

@trendfollowerpl
Copy link

Please add also usage to RegisterFactporyAs

@MKMZ
Copy link
Contributor Author

MKMZ commented Jul 5, 2018

I have also added a strategy for factory registration.

Here is an example:

            container.RegisterFactoryAs<IInterface1>(() => new SimpleClassWithDefaultCtor()).InstancePerDependency();

            var obj1 = (SimpleClassWithDefaultCtor)container.Resolve<IInterface1>();
            var obj2 = (SimpleClassWithDefaultCtor)container.Resolve<IInterface1>();
            Assert.AreNotSame(obj1, obj2);

@MKMZ MKMZ changed the title New instance per request strategy added for type registration New instance per dependency strategy added for type registration Aug 12, 2018
Copy link
Collaborator

@gasparnagy gasparnagy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quite like this PR. My only concern is about the change in the Resolve method, see below.

@@ -689,11 +756,10 @@ private object Resolve(Type typeToResolve, ResolutionList resolutionPath, string
AssertNotDisposed();

var keyToResolve = new RegistrationKey(typeToResolve, name);
object resolvedObject;
if (!resolvedObjects.TryGetValue(keyToResolve, out resolvedObject))
object resolvedObject = ResolveObject(keyToResolve, resolutionPath);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original code was using the already resolved object, the new one, seems to be always resolving a new object. Is this really how this would work? I thought this should decide what to do based on the strategy...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have implemented a stategy in IRegistration implementations (ITypeRegistration and IFactoryRegistration). They are implementing a new interface called IStrategyRegistration which have methods to choose a type of resolving strategy.

Going back to ObjectContainer.Resolve function there was a condition which had tried to resolve an object by looking for a matching instance in resolvedObjects. I had to remove it because the responsibility of resolving a new object should be completely moved to IRegistration classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is not harming any part of library. All unit tests along with a new ones has been passed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I will check it once more and approve the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you take a look one again at this PR please?

@gasparnagy
Copy link
Collaborator

Could you please also add a line about this change to the bodi.cs header (change history)?

Copy link
Contributor Author

@MKMZ MKMZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a line to change history:
"New 'instance per dependency' strategy added for type and factory registrations (by MKMZ)"

@@ -689,11 +756,10 @@ private object Resolve(Type typeToResolve, ResolutionList resolutionPath, string
AssertNotDisposed();

var keyToResolve = new RegistrationKey(typeToResolve, name);
object resolvedObject;
if (!resolvedObjects.TryGetValue(keyToResolve, out resolvedObject))
object resolvedObject = ResolveObject(keyToResolve, resolutionPath);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have implemented a stategy in IRegistration implementations (ITypeRegistration and IFactoryRegistration). They are implementing a new interface called IStrategyRegistration which have methods to choose a type of resolving strategy.

Going back to ObjectContainer.Resolve function there was a condition which had tried to resolve an object by looking for a matching instance in resolvedObjects. I had to remove it because the responsibility of resolving a new object should be completely moved to IRegistration classes.

Copy link
Collaborator

@gasparnagy gasparnagy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is cool. Sorry for not approving/merging it earlier.

@gasparnagy gasparnagy merged commit 7c0135c into SpecFlowOSS:master Sep 19, 2019
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.

None yet

3 participants