Circular dependencies

[ilyapuchka]

This PR adds support for circular dependencies. That feature and using resolve inside factory block requires recursion, but lock around resolve method causes deadlock. I removed locking completely and propose to managed thread safety on client side.

[ilyapuchka]

@AliSoftware hey, could you take a look when you have time?

[AliSoftware]

Missed that, sorry, will take a look during the week!

BTW don't you think we should do a release (containing the runtime arguments feature you added) first before adding this? The runtime arguments stuff is pretty huge already but is still in develop waiting.
Or maybe right after this PR, but I just hope I won't once again take too much time reviewing it before merging (swamped with SwiftGen + personal projects once again).

[ilyapuchka]

Actually I have one more feature ready in a queue - auto-injections, that I also thought to suggest for next release.

[AliSoftware]

Mmmh then maybe we should make a release right now with runtime parameters, then merge circular deps + auto-injections next, to make incremental releases instead of big leaps?

[ilyapuchka]

I would let you to decide =) Maybe auto-injection feature will take some time to approve and merge in.

[AliSoftware]

I was thinking (didn't think it through, that's just a though, will have to check if it really makes sense): couldn't DefinitionOfbe a struct to leverage value type?
Then inScope would return a new DefinitionOf instead of changing it in place (which seems less FP)?

[ilyapuchka]

That will not work because if those methods on DefinitionOf will produce new copies then we will need to replace them in registry. We could solve that by having register<T>(definition: DefinitionOf<T>) and creating definition manually, but that will break initially nice syntax in separate steps without adding anything valuable.

[AliSoftware]

Ok I see that now, but tbh I'm not sure I like that "nice syntax" of being able to register { … }.inScope(.ObjectGraph). Because having that inScope operator being able to change an already-registered dependency means that one can then change it at any time even later after registration, which feels wrong to me:

let def = container.register { [unowned container] ClientImp(server: container.resolve() as Server) as Client }
def.inScope(.ObjectGraph)
// … use it
def.inScope(.Singleton)
// … use it again
def.inScope(.Prototype)
// ... use it again

I think I'd prefer having container.register(.ObjectGraph) { … } instead (or use currying if you prefer or whatnot, but not a mutating function)

[ilyapuchka]

That's fair. What about resolveDependencies block?

[AliSoftware]

Haven't grasped that one yet. It seems like it has the same issue (mutating postfix function), but not sure how to avoid it in this case or if that's as problematic.
I guess I'm less annoyed by this case somehow. Maybe we could add a security in that resolveDependencies method so that it fatalError if we try to call it while it already has a value and so has already been called before, to prevent multiple calls at least?

[ilyapuchka]

We can try this:

• make DefinitionOf<T, F> where F will be type of factory and use it for factory property instead of Any
• add container argument to resolveDependencies method of DefinitionOf and use it to reinsert updated definition
• add remove<T, F>(_: DefinitionOf<T, F>) and register<T, F>(_: DefinitionOf<T, F>) to container (these methods can be handy anyway)

If that would not work (I suspect there will be ambiguity in method calls caused by generics) I would stick to using final class, adding fatal error and removing return self in resolveDependencies.

[AliSoftware]

Yeah, I think in that case directly keeping final class + fatalError + remove return self is probably the way to go. Adding that <T,F> will probably make the code harder to read — and indeed there is a also a risk of ambiguity for the compiler so better keep it simple and go with your second solution right away.

[ilyapuchka]

Actually it worked and I kind of satisfied with the result. You can see it in latest commit for this PR.
Using additional generic type added some complexity for sure, but that also lets get rid of force casting factory (which was Any) in resolve method.
Another two very positive side effects of this refactoring is ability to register/remove individual dependencies. These methods can be useful. Without generic type of factory that would be impossible cause there will be no way to compare definitions (because factory is of type 'Any', so no way to use ==).
Drawback is that now you have to separate creating definition and call to resolve dependencies like this:

    var serverDefinition = container.register(.ObjectGraph) { Server() as Server }
    serverDefinition.resolveDependencies(container) { container, server in
      server.client = container.resolve() as Client
    }

If you are ok with that change I will proceed with updating readme and playground.

[AliSoftware]

Can't we do that?

container.register(.ObjectGraph) { Server() as Server }
  .resolveDependencies(container) { container, server in
    server.client = container.resolve() as Client
  }

Sure it might seem odd to chain a call after a trailing closure, but that works, and some might be ok with that style. I myself as a user of the lib I would prefer writing it like you did with two lines but others can choose to chain too, so I'm ok with that ;)

One thing newcomers might be wondering is why you call resolveDependencies on serverDefinition and not the clientDefinition of your ObjectGraph or both. Be sure to explain the rationale and logic of that in the playground.

[ilyapuchka]

If resolveDependency is mutating method then this will not work cause methods return immutable values (the same as using let serverDefinition instead of var serverDefinition). But I guess we can make it not mutating and return new definition. Made another commit with that change.

[AliSoftware]

I have to go, will let you make the pending changes and I'll take a deeper look directly in Xcode later

[ilyapuchka]

@AliSoftware Updated playground and documentation. I think it's ready for merge now.

[ilyapuchka]

Now I've tried to reimplement auto-injection feature on top of that and it looks like structs semantics of DefinitionOf makes it much more harder. Maybe it would be better to rollback to final class variant.

[AliSoftware]

I've finally decided to release the 3.0.0 first, as the rest of the PRs will be big changes anyway and I liked having intermediate release. Even if we'll hopefully merge this PR and the next one soon, feel better to go incremental.

I've renamed the "Sample app" folder to "SampleApp" (I like avoiding spaces in paths), so you might want to rebase your branches on top of the new develop. (I was gonna do it myself but as it will mess with your local git working copy I didn't want to surprise you)

---

Regarding the auto-injection feature being harder to implement because of DefinitionOf being a struct instead of a final class, I'm ok in keeping it a final class in the end as long as we have as few mutating functions as possible in the API (like avoid methods like the previous inScope one)

Will take a look at the playground now 😉 and keep you posted.

[AliSoftware]

This test is mis-named now that inScope is not a thing anymore 😉

[AliSoftware]

I finally did the rebase myself and merged this PR after review. I admit I didn't have time to review all your code implementation to double-check it, but I figured I trust you on this and better not wait too much before merging.

Be careful as this as been rebased now: you'll probably need to rebase your new auto-injection on top of develop.
Feel free to insert some commits on develop to revert DefinitionOf back to a final class before rebasing, if that struct still bothers you for the implementation of auto-injection. I'm ok with going back to final class as long as we don't make methods that mutate the object if not necessary.

[ilyapuchka]

Thanks for very constructive review! =)

[AliSoftware]

Well maybe I merged a bit early as I forgot to address the comment about the documentation of <T,F> maybe you could improve that on develop?
(It's a small thing but with the library growing more and more and its implementation being more complex than in the beginning it's important to keep track 😉)