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

Singleton circular #50

Merged
merged 6 commits into from
Aug 13, 2013
Merged

Conversation

cesteban
Copy link
Contributor

Sorry for the late reply! Before sending the pull request I wanted to do some more testing. These are my conclusions:

  • The failing test I'm providing does not expose a bug: the problem appears only when you mix initializer injection and property injection in long cycles involving singletons, and it is documented that circular dependencies are only supported with property injection.
  • However, it would be extremely useful for me to be able to use initializer injection in some links of these kind of chains. Typhoon 'almost' support this: you can use initializer injection in isolated links, but when singletons are involved, sometimes you end up injecting several different instances of the same singleton.
  • The change I made helps with the singleton multiplicity problem.
  • Although initializer injection of circular dependencies is still not supported, the change is basically a sanity check, so it shouldn't be harmful anyway.

I know the change is kind of a hack for a feature that is not even supported in the first place, but it is being very helpful in my everyday use of Typhoon. I hope you can find it helpful as well.

Note: To see the test failing, you need to bring back [self buildInstanceWithDefinition:definition] in TyphoonComponentFactory.m:250

@jasperblues
Copy link
Member

No, you're quite right - it is a bug.

Although you can only have circular dependencies with properties, you should still be able to have an initializer. . . (I haven't thought this through, but I don't see why not). .. . at the very least if there's something that would prevent this from working, it should throw an exception and fail early "Initializers not supported with circular dependencies". . . instead of behave in surprising ways.

jasperblues added a commit that referenced this pull request Aug 13, 2013
@jasperblues jasperblues merged commit d6af482 into appsquickly:master Aug 13, 2013
@cesteban cesteban deleted the singleton-circular branch August 13, 2013 10:17
@cesteban cesteban restored the singleton-circular branch August 13, 2013 10:17
@cesteban cesteban deleted the singleton-circular branch August 13, 2013 10:21
@rhgills
Copy link
Contributor

rhgills commented Aug 31, 2013

Great work, just ran into this issue myself when using an older version of Typhoon. Elegant fix.

rhgills added a commit that referenced this pull request Aug 31, 2013
Cleanup comment regarding failing tests that are no longer failing.
Remove commented out code to reproduce a failure that was fixed in #50.
Rename circular dependency test variables to clarify dependencies.
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.

3 participants