tschneidereit / SwiftSuspenders

NOTE: Find the offical repo at http://github.com/robotlegs/swiftsuspenders
https://github.com/robotlegs/swiftsuspenders
MIT License
280 stars 89 forks source link

Get existed InjectionConfig from cache directly, in order to get working with internal class injecting. #46

Closed imcotton closed 12 years ago

imcotton commented 13 years ago

Due to the ApplicationDomain.getDefinition() does not work on internal class, it's better to get those Class type reference from cache directly instead of unnecessarily reflection from ApplicationDomain again.

So this code could getting to work nicely:

public function Test ()
{
    var injector:Injector = new Injector();
        injector.mapClass(Item, Item);

    var box:Box = injector.instantiate(Box);

    trace(box.item);
}

class Box
{
    [Inject] public var item:Item;
}

class Item
{
}

This pull request based on the latest master branch, and I also made the same change on this branch based on tag 1.5.1, which used by Robotlegs in current version v1.4.0.

Please take a look, thanks for your time.

imcotton commented 12 years ago

I've added the test case against this issue.

tschneidereit commented 12 years ago

Sorry for not getting back to you earlier!

I've looked at your pull request and I'm afraid I won't accept it into my repository. Not because of the quality of your changes - they are very well implemented and tested.

They do add a lot of overhead to a very performance-critical part of Swiftsuspenders, though. Also, I honestly don't think that inner classes should be injected the way you're doing it here. The whole point of an IoC container is to make it easy to configure dependencies from the outside. An inner class, on the other hand, is inherently and completely tied to its outer class - and vice versa. That means that it doesn't make too much sense to try to provide this dependency from the outside: It simply can't change in any meaningful way and thus doesn't require configuration at all.

I'm very curious to hear what your real-world use-case for this feature would be. I readily admit that I might be missing something very obvious (or very clever) that would very much deserve to be enabled by this feature. Feel free to reopen the request if you think that you have such a use-case.

imcotton commented 12 years ago

Thanks for taking time to review here, I got two reason for enabling this feature:

  1. Performance optimizer.
  2. The the-future branch of SwiftSuspenders has already obtained this feature by whole new architecture for v2.0 (awsome BTW), which means the leak of capability in the 1.x maintenance branch didn't for purpose, or it's not been forbidden intentionally at least.

And for the real world use case, also the original reason of this patch came from, is when there is sort or heavy components being the part of an application, and just want hide the internal implemention from outside of those components in the package level, either the inner classes in one single class file, or parts class file by internal class definition works.

Due to below scenario, currently, there is no way to use SwiftSuspenders as DI in the hiding area because non-public class injection duing the issue we were dealing with.

The test case just to make sure the functionality works, and I hope below explanation could be cover the concept of this patch for.

As always, thanks for your time :)

imcotton commented 12 years ago

BTW, I don't have the permission to reopen this ticket.

tschneidereit commented 12 years ago

Ah, sorry - I thought that reopening was possible for ticket creators. But if I understand you correctly, your use-case will just work in Swiftsuspenders 2, right?

imcotton commented 12 years ago

You're right, but v2 isn't gain the backward compatibility from 1.x for those project still looking for the old 1.x version API. And I couldn't help but notice that v2 has been targeting to the Flash Player 10.2 recently, so it's another backward compatibility for those project still stuck with Flash Player 10 or even 9.

I think that would be nice to have this maintenance branch for 1.x like master branch currently is, to remain the less interface or capability change in the future as Robotlegs 1.x going to.

tschneidereit commented 12 years ago

I plan to release a façade for making Swiftsuspender 2 backwards compatible with Robotlegs 1. That means that projects should be upgradable by switching out a SWC file.

The bump to Flash Player 10.2 is purely compile-time for the SWC, you can still use the result in 10.0. Not 9, though, because some Arrays have been converted to Vectors.

If you absolutely must support a Flash Player 9-based project with your change, I suggest you just use your own build. Keeping official support for Robotlegs 1 would be too much of a burden for me, to be honest.

imcotton commented 12 years ago

Cool, that facade as adapter between two API version is great indeed!

After reading this discussion, it seems results as to drop support of FP 9 for RL 2, but the RL 1 will remain still for any of FP 9 needs. So there only gonna be either SS 2 backwards compatible with RL 1 or SS 2 targeting to FP 10 but not both.

I understand that not to having two branches parallel for v1 and v2, and I'm also 100% agree that both SS 2 and RL 2 should moving to the FP 10, but could this patch to be consider as minor enhancement on current master branch, because with all do respect, I sill think this changes are reasonable and without bad tasteful, also use case driven behind.

Thanks for your time.

tschneidereit commented 12 years ago

I understand that you really care about the change, so I will take a look at how to properly implement it in Swiftsuspenders 1. The problem with your current patch is that it might cause an extra method call in a very sensitive place and it adds another public method to the already crowded Injector class.

I'll get back to you after I had a deeper look, ok?

imcotton commented 12 years ago

That will be nice, and thanks for your understanding.

I know that you and also RL team has been very busied on the big next v2, I'm so appreciate that you could take the time out and back on this, thanks very much.

tschneidereit commented 12 years ago

I'm sorry to say, but I've finally decided against taking this change for 1.x. Even though it is a very small change, it just has too much of a performance impact to do it for a version of the project that's not otherwise tended to, anymore.

Please either update to version 2 - which is really very stable now - or use your own fork of 1.x.