Skip to content
This repository has been archived by the owner on Apr 13, 2022. It is now read-only.

feat(directive): Add ability to inject required controllers into the controller function #5893

Closed
ghost opened this issue Jan 20, 2014 · 25 comments

Comments

@ghost
Copy link

ghost commented Jan 20, 2014

quick overview
You have the directives A, B and C
B requires A,
C requires B

The methods in C need to call methods in the controller of B.
The methods in the controller of B need call methods in the controller of A.

The controller of B cannot know the controller of A because it can only be passed into the link-function of B.

more details
http://stackoverflow.com/questions/21231294/angularjs-inject-required-directive-controller-into-the-controller-instead-of-t/21231428

Actually there is a workaround that is too complicated for such an easy problem.

Even the ngModelController does use another workaround with a feature that seems to be undocumented: the binding of directive controllers to the directive-elements data property:

var parentForm = $element.inheritedData('$formController') || nullFormCtrl,

@thorn0
Copy link
Contributor

thorn0 commented Dec 8, 2014

This issue should be reconsidered. Now that we have bindToController, it's inconsistent and not clear why required controllers are accessible in the link function, but not in the controller constructor. If somebody wants to create directives the controllers of which interact with each other, they have to initialize the controller with a special call from the link function now.

m.directive('childComponent', function() {
  return {
    require: ['childComponent', '^parentComponent'],
    controller: function($scope) {
      this.init = function(parentComponent) {
        this.parentComponent = parentComponent;
        // ...
      };
    },
    link: function(scope, el, attrs, ctrls) {
      var childComponent = ctrls[0],
        parentComponent = ctrls[1];
      childComponent.init(parentComponent);
    },
    // ...
  };
});

Why can't this look like this?

m.directive('childComponent', function() {
  return {
    require: '^parentComponent',
    controller: function($scope, parentComponent) {
      this.parentComponent = parentComponent;
      // ...
    },
    // ...
  };
});

@shahata
Copy link
Contributor

shahata commented Dec 9, 2014

In general I agree that this is inconvenient, but I think that the main issue with passing the required controller in the controller constructor is that then you will not be able to have symbiotic directive (a require b and b require a - example: http://plnkr.co/edit/RrBKdrExc4CaYIqyXJhq?p=preview). You will have circular dependency instead, and I don't think we want to break this kind of directives.

@thorn0
Copy link
Contributor

thorn0 commented Dec 9, 2014

It's possible not to break the directives of such kind. Because in JS, it's possible to create objects before calling their constructors, and that's how actually bindToController is implemented.

@shahata
Copy link
Contributor

shahata commented Dec 9, 2014

Still constructor of controller a will not be able to invoke stuff that are defined in constructor of directive b if it didn't get constructed yet, which is pretty confusing.

@thorn0
Copy link
Contributor

thorn0 commented Dec 9, 2014

That's true, however such symbiotic directives look like an edge case and not the best design choice, don't they? And if you decide to create alike symbiotic services that use each other, it will look confusing enough as well. The DI system won't allow you to do this without a workaround. So can this really be a stopper for this feature?

@johnlindquist
Copy link
Contributor

I think a "resolve", which works similar to how routing handles controllers, makes more sense.

For example, "require" uses $element.controller('directiveName') to look up controllers of other directives.

You can simply inject $element into your controller and do it yourself:

pseudo non-working off-the-cuff code below:

function FooCtrl($element){
var otherCtrl = $element.controller('other');
}

but I'd rather have a resolve to remove the $element dependency (for testing, etc)

.directive('foo', function(){
  return {
    resolve: {
      other: function($element){
        return $element.controller('other')
      }
    },
    controller: 'FooCtrl as foo'
    }
  })

//other found on resolve
.controller('FooCtrl', function(other){
  other.doStuff()
}

@kentcdodds
Copy link
Member

Hmmm... I sort of like the idea of resolve in directives. I'm torn because the DDO is so complex already, adding resolve could make it worse. Also, what does that do to the visibility of the api for a directive? If there are some things that are resolved into the controller for a directive then you can't just look at the html and know what the api to the directive is anymore, you have to know what the dependencies in the resolve are as well which may not be an issue, but I think it could. Also, if resolve is asynchronous, then what does the DOM look like while it's being resolved? If it's not, then it leads to an even further confusing api.

Those are concerns, not solutions. I think a good solution to this would be to simply add the controller to locals when the controller is instantiated via the controller name as @thorn0 recommended. I feel like that is a reasonable api and it doesn't complicate the DDO further.

Still haven't totally thrown out the value of resolve though. Would be interested to hear responses to the concerns I mentioned.

@caitp
Copy link
Contributor

caitp commented Dec 16, 2014

i don't think it would be wise to add yet another way to make synchronous compilation asynchronous...

At the very least, it should't be done without combing through the compiler, identifying every single use case that it needs to support, and re-writing it into something sane and maintainable. Adding more sync vs async craziness at this point is just not gonna fly :(

@johnlindquist
Copy link
Contributor

Yeah, it's wishful thinking more than anything. I think the similarities
between router/template/controller and directive/template/controller should
also mean they have similar APIs. I completely understand the
legacy/complexity makes it difficult.
On Dec 15, 2014 8:05 PM, "Caitlin Potter" notifications@github.com wrote:

i don't think it would be wise to add yet another way to make synchronous
compilation asynchronous...

At the very least, it should't be done without combing through the
compiler, identifying every single use case that it needs to support, and
re-writing it into something sane and maintainable. Adding more sync vs
async craziness at this point is just not gonna fly :(


Reply to this email directly or view it on GitHub
#5893 (comment).

@thorn0
Copy link
Contributor

thorn0 commented Jan 13, 2015

I feel like that is a reasonable api and it doesn't complicate the DDO further.

Actually, it simplifies the DDO. Compare the two snippets I wrote above.

@ewinslow
Copy link
Contributor

@kentcdodds see #2095

@kentcdodds
Copy link
Member

@thorn0, I was referring to it making the DDO more complex for module.directive users, not angular.component. Either way 👍

@ewinslow, I decided against asynchronous directives. There are several reasons it just wouldn't work well.

@piernik
Copy link

piernik commented Feb 8, 2015

I think that injecting required controller as local is "have to" in new version.

@SonofNun15
Copy link

👍
We are wrestling with this same issue. We try to do very little in link functions because they are harder to test. We tried putting scope.myController = requiredController in the prelink function so that the controller of the required directive would be available in the child directive's controller, but it turns out that the controller runs before both the pre AND post link functions, so this isn't going to work.

@piernik
Copy link

piernik commented May 21, 2015

Any news on this?

@programmist
Copy link

+1

2 similar comments
@djdmbrwsk
Copy link

+1

@hardsetting
Copy link

+1

@thorn0
Copy link
Contributor

thorn0 commented Aug 19, 2015

To simplify the implementation, we can introduce a restriction that only the controllers from the element's parents (^^ and ?^^) can be injected.

@ciccia
Copy link

ciccia commented Oct 5, 2015

+1

@abyx
Copy link

abyx commented Oct 7, 2015

👍

2 similar comments
@DumboJet
Copy link

+1

@ifeltsweet
Copy link

+1

@poshest
Copy link
Contributor

poshest commented Dec 19, 2015

+1, and also for it to be added to the new .component

@petebacondarwin
Copy link
Member

@hannahhoward had an interesting take on doing this in #12342

@petebacondarwin petebacondarwin modified the milestones: 1.5.x - migration-facilitation, Ice Box Jan 12, 2016
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Jan 13, 2016
…oller construction

This enables option three of angular#13510 (comment)
by allowing the creator of directive controllers using ES6 classes to have a hook
that is called when the bindings are definitely available.

Moreover this will help solve the problem of accessing `require`d controllers
from controller instances without resorting to wiring up in a `link` function.
See angular#5893
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Jan 14, 2016
…oller construction

This enables option three of angular#13510 (comment)
by allowing the creator of directive controllers using ES6 classes to have a hook
that is called when the bindings are definitely available.

Moreover this will help solve the problem of accessing `require`d controllers
from controller instances without resorting to wiring up in a `link` function.
See angular#5893
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Jan 14, 2016
…oller construction

This enables option three of angular#13510 (comment)
by allowing the creator of directive controllers using ES6 classes to have a hook
that is called when the bindings are definitely available.

Moreover this will help solve the problem of accessing `require`d controllers
from controller instances without resorting to wiring up in a `link` function.
See angular#5893
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Jan 14, 2016
…oller construction

This enables option three of angular#13510 (comment)
by allowing the creator of directive controllers using ES6 classes to have a hook
that is called when the bindings are definitely available.

Moreover this will help solve the problem of accessing `require`d controllers
from controller instances without resorting to wiring up in a `link` function.
See angular#5893
Narretz pushed a commit to Narretz/angular.js that referenced this issue Jan 19, 2016
…oller construction

This enables option three of angular#13510 (comment)
by allowing the creator of directive controllers using ES6 classes to have a hook
that is called when the bindings are definitely available.

Moreover this will help solve the problem of accessing `require`d controllers
from controller instances without resorting to wiring up in a `link` function.
See angular#5893
Narretz pushed a commit to Narretz/angular.js that referenced this issue Jan 19, 2016
…oller construction

This enables option three of angular#13510 (comment)
by allowing the creator of directive controllers using ES6 classes to have a hook
that is called when the bindings are definitely available.

Moreover this will help solve the problem of accessing `require`d controllers
from controller instances without resorting to wiring up in a `link` function.
See angular#5893
petebacondarwin added a commit that referenced this issue Jan 19, 2016
…oller construction

This enables option three of #13510 (comment)
by allowing the creator of directive controllers using ES6 classes to have a hook
that is called when the bindings are definitely available.

Moreover this will help solve the problem of accessing `require`d controllers
from controller instances without resorting to wiring up in a `link` function.
See #5893

Closes #13763
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests