feat(directive): support injecting required controllers into a directive controller #6040
Conversation
|
I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS. Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match. If you signed the CLA as a corporation, please let us know the company's name. Thanks a bunch! PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR. |
|
signed |
| var requiredControllers; | ||
| if (directive.require && directive.require !== directive.name) { | ||
| // the directive requires its own controller. this would make an error | ||
| // this does not prevent require: "^sameDirective" |
There was a problem hiding this comment.
Why would this make an error? It's perfectly legal for a directive to require its own controller
... Oh, I guess you mean like a circular dependency issue when injecting these into the controller itself. I see.
There was a problem hiding this comment.
@caitp at this point you want to instantiate the controller and need to resolve the injection locals here. Before it is instantiated, it is not in the data-property of the dom-element. This means it cannot be required / injected in this way. I don't see any reason why a controller should require itself (keyword: this!). I see valid ways there the controller requries a parent-controller of the same kind like it is by the form directives, but this does not prevent that.
There was a problem hiding this comment.
Oh, you certainly can get the controller of sibling directives from the expandostore in the controller closure! Unless you're using certain versions of jQuery which are currently slightly broken (in combination with angular, such as v2.0.x), this does work and I have apps showcasing it.
But at any rate, this is a feature and it's going to be some time before it gets merged, if it gets merged at all. There are already tools available to do this stuff.
|
So, I'm not sure we really need this, because you can already get the other required controllers with jqLite.data() or jqLite.controller(). I can see how injecting them would be a bit less work, but I'm not too sure it's a great idea. |
|
@caitp As far as I know it is undocumented to do this and what is more importent it is an inconsistent api at this point. |
ghost
added
petebacondarwin
force-pushed
the
master
branch
from
02dc2aa
to
fd2d6c0
Compare
jeffbcross
force-pushed
the
master
branch
2 times, most recently
from
cad9560
to
f294244
Compare
jeffbcross
force-pushed
the
master
branch
4 times, most recently
from
e8dc429
to
e83fab9
Compare
petebacondarwin
force-pushed
the
master
branch
from
4dd5a20
to
998c61c
Compare
Solves #5893
This should not break any existing applications, but there is an API-change: you can inject $requiredControllers in your directive controller, too.