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

Auto-injection #13

Merged
merged 7 commits into from Jan 14, 2016
Merged

Auto-injection #13

merged 7 commits into from Jan 14, 2016

Conversation

ilyapuchka
Copy link
Collaborator

This PR adds auto-injection feature.

What is auto-injection

Auto-injection is a feature that let's user to get all dependencies (properties) injected by container automatically without calling resolve for each dependency.

Why we need this

  • It's a more convenient way to define circular dependencies
  • It will enable automatic injection of dependencies for objects created by storyboards or nibs (as I currently imagine possible implementation of that feature) what I think is a very important feature to adopt Dip in real life projects.

How it works

This implementation provides not completely automatic resolution which seems to be impossible to implement in Swift without using ObjC runtime. So to inject dependencies user needs:

  • wrap properties with special wrappers to indicate that they should be injected by container
  • obtain an instance of object to inject to (either create it manually or resolve with container)
  • call resolveDependencies method of container and pass it this instance

resolveDependencies method uses reflection to inspect types of properties. To work around different issues, like initial nil values of properties, weak references and others we use simple wrapper classes: Injected<T> and InjectedWeak<T>. Using Mirror we iterate over properties and pick up those of type Injected or InjectedWeak (actually dummy internal protocols). Then we use resolve and assign resolved instance to values wrapped by those properties. The fact that Injected and InjectedWeak are classes makes it possible to change wrapped value not only in the mirror, but also in reflected object cause values of mirror children are copied by reference as any class in Swift (though Apple does not document this behavior of Mirror with all the possible consequences).

To be able to resolve this wrapped values for each definition registered with nil tag and block-factory that does not accept runtime arguments we register two additional definitions for Any and AnyObject type (for strong and weak properties correspondingly) associated with special tags. Those tags are just string descriptions of type being registered wrapped in Injected or InjectedWeak. Using such tags makes it possible to distinguish those accessory definitions, cause they all resolve Any or AnyObject types. User will likely not use such tags so there should be no collisions.

Drawbacks

  • Using wrappers requires some boilerplate code. The same can be done using resolveDependencies method of DefinitionOf. But using auto-injection registration syntax looks much cleaner. Also I think that Injected and InjectedWeak wrappers serve as a developer's documentation to indicate that those properties will be injected at runtime, so developer does not need to go to the point where all registration is performed. Other frameworks like Typhoon also use similar approach.
  • Apple does not document Mirror enough to be sure that required behavior (referencing to child values instead of coping) will not change in future. And actually they already changed reflection API (but not the behavior I believe) between Swift 1.2 and Swift 2.0. But this behavior looks logical and reflection is a language feature, not an OS, so we will be able to adapt (or disable) this feature on demand in later Swift releases.

Example

protocol Server {
  weak var client: Client? {get}
}

protocol Client: class {
  var server: Server? {get}
}

class ServerImp: Server {

  private var _client = InjectedWeak<Client>()

  weak var client: Client? {
    return _client.value
  }
}

class ClientImp: Client {

  private var _server = Injected<Server>()

  var server: Server? {
    return _server.value
  }
}

container.register(.ObjectGraph) { ServerImp() as Server }
container.register(.ObjectGraph) { ClientImp() as Client }

let client = container.resolve() as Client

@ilyapuchka
Copy link
Collaborator Author

@AliSoftware I will update documentation and playground after your review when we will agree on the implementation

@ilyapuchka
Copy link
Collaborator Author

Also as you will see I rolled back to using final class for DefinitionOf cause struct semantic makes it still possible but unnecessary complicated and much more convoluted than it should be.

@AliSoftware
Copy link
Owner

Interesting feature!

  • How does Injected and InjectedWeak know which container to resolve from though?
  • As this feature requires the use of ObjC-Runtime, I wish we add it as a subspec of the Dip pod (even if we choose to include it by default, maybe?), so that the Dip core stays full-Swift.

_It some point I wish that Dip core can be used even on Swift projects not relying on Cocoa, Foundation nor ObjC, especially with Swift becoming OpenSource and opening to other platforms in the future, that's why I'd separate this code in a dedicated subspec)

@AliSoftware
Copy link
Owner

@ilyapuchka Did you properly rebase that branch on top of develop? I saw in the travis build that the Sample app still had the depreciation warnings whereas I fixed them by using register(.Singleton) … in a recent commit on develop this branch doesn't seem to have picked. Also be reminded that I also renamed the "Sample app" directory to "SampleApp" (you might want to do a git clean -df after rebase to make sure there is no old code remaining in your local copy).


let stringKey: String
switch key.associatedTag {
case .String(let tagValue)? where tagValue.hasPrefix("InjectedWeak<"):
Copy link
Owner

Choose a reason for hiding this comment

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

Couldn't we do more secure and error-proof tests rather than string-based comparisons here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, sorry, thats not needed anymore, redundant code from my attempts to make it work with structs. Will fix now.

@ilyapuchka
Copy link
Collaborator Author

No no, there is no objc runtime at all, I meant something else =) It relies completely on Swift reflection. Regarding the first question I actually does not detect what type is wrapped, instead I rely on tags that are constructed based on that wrapped types, so there is no confusion cause the same tag is used to register original definition for not wrapped type.

@AliSoftware
Copy link
Owner

Ah I see. Don't like the string-based comparisons at all anyway (I know that Swift's Mirror is not easy to play with sadly 😞 but it would really be cool if we can find a better, more secure solution)

@ilyapuchka
Copy link
Collaborator Author

Yep, that looks a bit fragile but as long as runtime generated tags and dynamicType description of properties stay the same I thinks it's rather safe. With some redundant code removed now there are only two places when string comparison is used - when registering accessory definitions we construct those string tags, next time when we resolve we get the same string from dynamicType description.

I will appreciate your thought on that, cause I've spent a lot of time with this implementation and now it's hard to think about other possibilities (though I think I tried everything)

@ilyapuchka
Copy link
Collaborator Author

Regarding rebase it's strange cause what I did was just adding implementation after I pull latest develop. Will check now.

@ilyapuchka
Copy link
Collaborator Author

Ah, that's right, looks like you fixed warnings only in app code, not in tests, I will fix that =)

@AliSoftware
Copy link
Owner

Wooops, my bad ;)

@AliSoftware
Copy link
Owner

Yep, that looks a bit fragile but as long as runtime generated tags and dynamicType description of properties stay the same I thinks it's rather safe.

Mmmmh problem is I'd say that this is the most likely stuff that might change, a bit like the implementation of description or debugDescription are are not meant to be used for program logic and are not guaranteed to stay unchanged between Swift releases… :-/

@ilyapuchka
Copy link
Collaborator Author

What I can suggest to make it safer is instead of using "Injected<\(T.self)>" use "\(Injected<T>.self)" in register method. Actually I used that approach at some point. That will ensure that tags will be the same when registering and resolving. Child.value will have type Injected<T> (or InjectedWeak<T>, so I can not think of the way it's dynamic type description will be different.

@ilyapuchka
Copy link
Collaborator Author

Another possible solution could be to define some protocol, like AutoInjectable that will require type to return some tag value, store this value in Injected wrappers and use it to register definition. This way we do not depend on dynamicType description implementation (thought returning dynamicType description can be default implementation of this protocol). But that will require user to make his protocols to inherit from this protocol and I think that could be not possible in some cases. Will try this approach tomorrow.

@AliSoftware
Copy link
Owner

Maybe provide a protocol default implementation that return the dynamicType as the tag, which will still allow the user to provide a different one if they want, that could be a nice tradeoff

@ilyapuchka
Copy link
Collaborator Author

I've made some changes that make tag generation safer. If fact now it does not really matter what its value will be, protocols will ensure that the right one is used. But to make it easier to debug I think it is better to use "(Injected.self)" and "(InjectedWeak.self)". Also using generic T in this tag makes it possible to have different tags for different wrapped types. And reflection code became obvious now.

Also you may find this interesting - https://twitter.com/ilyapuchka/status/668870356225142784

To provide custom tags with additional protocol looks impossible. The problem could be in some bugs in protocol extensions (I personally filed two or three) but more likely in the way Swift picks up implementation when you cast your object in runtime to more general type. It picks up more general implementation when we need to use more specific one. And there is no way we can provide more specific type doing reflection, we just can not construct it cause it's a generic type. I've pushed it to separate brunch - bf9aa5e - so you can check out.

@ilyapuchka ilyapuchka force-pushed the feature/auto-injection branch 2 times, most recently from a4eb0c5 to 1377a9b Compare November 23, 2015 23:14
@ilyapuchka
Copy link
Collaborator Author

I refactored how the keys for auto injection are managed so that they are now encapsulated in DefinitionOf. It makes sense cause they only depend on generic argument T of definition and can be calculated in it's constructor.

@ilyapuchka
Copy link
Collaborator Author

@AliSoftware I tried to extract this implementation to separate framework, but it looks much more complicated and requires lots of delegate callbacks from container, which makes complicated both container and "extension". Some of the issues I could not solve, like storing auto injected instances for singleton scope. So I think it's better to include it in core. Also it does not change registration/resolution API, it only changes how you define properties in you classes and whether you use resolveDependencies API or use auto-injection.

@ilyapuchka ilyapuchka removed the WIP label Dec 13, 2015
@ilyapuchka ilyapuchka force-pushed the feature/auto-injection branch 2 times, most recently from 1e9e4c2 to 92f80e5 Compare December 13, 2015 23:03
@ilyapuchka
Copy link
Collaborator Author

I rebased this brunch on develop and squashed commits.
I've renamed private protocols used for auto injection to _InjectedPropertyBox and so on to make them better reflect their purpose.

@AliSoftware
Copy link
Owner

Oh god and it's already monday, 1am… won't have time to review this again until 2-3 days, will keep you posted.

@ilyapuchka
Copy link
Collaborator Author

No pressure 😉

@ilyapuchka
Copy link
Collaborator Author

@AliSoftware this one is desperately waiting for you review too =)

@AliSoftware
Copy link
Owner

Oh god thx for the reminder, totally forgot those! Will review now.

@AliSoftware
Copy link
Owner

Ok was stopped in my motivation by a phone call from family that lasted a bit longer than expected and now it's already late and haven't even eaten yet… so will probably do tomorrow instead 😉 but I didn't forget yet

@AliSoftware
Copy link
Owner

Looks good at first.
Could you add a dedicated page in the playground to demonstrate and document this?
Also maybe it's worth a paragraph in the README.

@ilyapuchka
Copy link
Collaborator Author

Yes, sure. I just wanted you first to check out the implementation and if it's ok for you proceed with documenting.

@AliSoftware
Copy link
Owner

Yeah I'm ok
BTW I think a more complete example than just Client and Server you use in your tests (with only one injected property each) should be used in the playground.

Reading a quite too simple example especially with only one injected property, I didn't immediately see the benefit of using Injected<T> on the property and call resolveDependencies() on the wrapping type as opposed to calling resolve on the property. Only your explanations in this PR comments helped me see that.

So maybe in the playground you can (1) use a first simple example of Client/Server to demonstrate how it helps with circular dips (2) but add another example using classes with 2 or 3 injected properties to demonstrate how it helps calling resolveDependencies on the class instead of on each prop, so those benefits become more clear to the user

@AliSoftware
Copy link
Owner

I like the examples, 👌 :shipit:

@ilyapuchka
Copy link
Collaborator Author

@AliSoftware let's merge this?

@AliSoftware
Copy link
Owner

Yep, that's what I meant by those ":ok_hand: :shipit:" emojis, I thought you'd do it :laughing:

AliSoftware added a commit that referenced this pull request Jan 14, 2016
@AliSoftware AliSoftware merged commit 47043a2 into develop Jan 14, 2016
@ilyapuchka ilyapuchka deleted the feature/auto-injection branch January 14, 2016 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants