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

Initializer circular dependencies - support or not? #161

Closed
jasperblues opened this issue Feb 2, 2014 · 8 comments
Closed

Initializer circular dependencies - support or not? #161

jasperblues opened this issue Feb 2, 2014 · 8 comments

Comments

@jasperblues
Copy link
Member

So, we're moving towards raising an exception for circular dependencies in initializers?

I was moving the RaiseCircularInit exception concern over to TyphoonCallStack (better OO and I want to reduce complexity in TCF+InstanceBuilder). .

In doing so, I found a new case that wasn't being detected as yet:

  • (void)test_initializer_injected_component_is_correctly_resolved_in_circular_dependency

thoughts?

As we always try to check in green builds, i've changed the test conditions, but we need to decide what to do here. support or not.

@alexgarbarev
Copy link
Contributor

I think moving RaiseCircularInit into TyphoonCallStack is not good idea, because it used here:

[[_stack peekForKey:componentKey] addInstanceCompleteBlock:^(id reference)
{
    [(NSObject*) instance setValue:reference forKey:propertyName];
}];

That means if we init first object with second as init parameter, and second references to first with property, second will wait until first end initialization and then point to first by property.
I think it is ok and expected by user.
I didn't see a reason to not support it.
I'll refactor to move exception from TyphoonCallStack

@alexgarbarev
Copy link
Contributor

Fixed by adding peekInstanceForKey method to TyphoonCallStack class. So if we want to peek only element - don't raise an exception and raise if we want instance.

@jasperblues
Copy link
Member Author

Perhaps its a little bit too surprising to have the two methods with different types of behavior?

I had it like this:

peekForKey:(NSString*)key raiseCircularDependenciesException:(BOOL)raise

. . but was toying with the idea that maybe we don't need that parameter.

I started moving some things out of TCF+InstanceBuilder (was becoming a god-class). . definitely seemed to work for the init invocation - allows using polymorphism by assigning the responsibility of argument setting to the parameters. . . .

I thought it made sense for TyphoonCallStack to take on this responsibility. Perhaps not?

@alexgarbarev
Copy link
Contributor

Yes, I agree that TyphoonCallStack is place for exception. I just want to say that we have to support this circular dependence. It is not hard (or a lot of code) to support.

I can't imagine example of usage, but If user can imagine it, he will be surprised that it is not supported.

@alexgarbarev
Copy link
Contributor

I think better to raise exception in TyphoonStackElement instance getter. I.e. when someone access to instance while it in initializing state -raise exception

@alexgarbarev
Copy link
Contributor

@jasperblues check current implementation, may be it is better

@jasperblues
Copy link
Member Author

Looks g

@jasperblues
Copy link
Member Author

Looks good Aleksey :)

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

No branches or pull requests

2 participants