Directive could not find parent directive's controller on AngularJS 1.2.0 when replace property is set to 'true'

[psysp]

Hello

While upgrading an application from AngularJS version 1.1.5 to 1.2.0 I ran into some issues which are related to inter-directive communication, done via directive's controller. In the application there's a parent directive and a child directive. At a certain point the child directive has to call a method of its parent directive's controller. Therefore the parent directive is marked as required in the child directive. One specialty about this scenario is that the parent directive uses a template which contains an ng-include directive to dynamically load a template which uses the child directive.

When running on version 1.2.0 the child directive throws an error with the following message: "Controller 'xxx', required by directive 'yyy', can't be found!"
After debugging and some more investigation I think the problem is related to the ng-include directive, but I'm not 100% sure why it doesn't work any more with version 1.2.0.

I managed to reproduce the problem on Plunker : http://plnkr.co/edit/COAkdNCYuMhpaHsWFopx

In the example I currently use version 1.2.0 so it won't work but you can easily switch to version 1.1.5 to see how it should work.

The error only happens when the 'replace' variabel is set to 'true'. If it is set to 'false' everything works as expected.

[tbosch]

Thanks for reporting this.

This problem was added by this commit in 1.2.0-rc3: 78656fe

Reason:

• ng-include replaces the node temporarily with a comment node until the source is loaded.
• We usually use $.data to store controllers at elements. However, this does not work for comment nodes. In that case, we are storing the controller as a direct property on the node (see https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L1472).
• To prevent memory leaks we remove this property from the node when we first read it (see https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L1335)
• The problem that was added by the commit above: Any directive that provides a controller now automatically requires it. By this, when applying the main directive the controller is read from the element and removed. After this, it's no more available to the child directive.

Possible solutions:

• patch the $.data method so that it is also able to store data on comment nodes. For jqLite we can just integrate this directly. However, for jQuery we really need to path the original method (setter and getter).

[tbosch]

@IgorMinar What do you think about this?

[tbosch]

jQuery added this check by this issue: http://bugs.jquery.com/ticket/8335. So the problem is that jQuery is not cleaning up comment nodes as it just a element.getElementsByTagName('') / querySelectorAll('') for finding the children to clean up when element.remove is called.

[tbosch]

A simpler case for this:
A directive that uses transclude: element and provides a controller, and a child directive requiring that controller.

See this plunk: http://plnkr.co/edit/iyUvYEjU8U52Quy94UKm?p=preview

[tbosch]

Actually patching / fixing $.data would result in a performance problem as we then had to find all comment nodes when $.remove was called. Better solution: Keep a context in the compiler that keeps these controllers until the link phase is finished (e.g. cleanup callbacks, ...). However, this won't be an easy change...

[vojtajina]

Btw, I don't think this was introduced by 78656fe as that commit is part of 1.0

[vojtajina]

I see, it was not issue before 1.2, because ng-include did not use to use transclusion before (which implies using a comment node).

[vojtajina]

Here's updated plunker with a workaround (see app.js:14-16) http://plnkr.co/edit/evNrjfkEmQx5iutkJ7Ub?p=preview

Discussed with Tobias and he's gonna make a fix (basically the workaround above) that we could put in 1.2.1.

Long term I believe we should get rid off this issue completely by not putting stuff on comment elements. I think at the time when we only have the comment placeholder, there's no reason for anybody to ask for the controller (neither there is any reason for the controller to be instantiated). Also, a transclusion can be applied multiple times (eg. ng-repeat), in which case we need to use a different controller per transclusion instance (clone) anyway - having "a comment node controller" is useless (basically another bug, that ng-repeat would currently give every instance the same "comment node controller").

[tbosch]

Here is a more practical example:
Directive main has a template like this <div ng-if="true"><div sub></div></div>. Directive sub is requiring the controller of the parent directive main, but right now, this throws an error.

[tbosch]

See this plunk: http://plnkr.co/edit/HdKTOzYVS6Odx2FWczBj?p=preview

[tbosch]

Btw: Same is true for ng-repeat.

We are cloning the element using a transclude, but as the directive has replace: true the cloned elements don't know about the other controllers on the original element right now.

The solution to the initial problem looks like this:

compile: function(element, attr, transclude) {
      return function(scope, element, attr, ctrl) {
        transclude(scope, function(clone) {
          clone.data('$mainController', ctrl);
        }); 
      }      
    }

I don't think it's possible to move this specific into the compiler for a generic solution, as there seems to be no possibility how the transclude function might access the controllers of the link phase as:

• the transclude function is created at compile time
• the link function of the directive might be called multiple times
• the transclude function might be called anytime during or after the link function has been called (e.g. using $timeout, $http, ...).

The example above is able to solve this by using the javascript closure.

A general solution could be to create a new transclude function for every call to the link function of a directive. Such a transclude function would then have access to the controllers of the linking phase and also the cloned element.

We already have something like this: $transclude is a function that is created in the link phase. We do this so that it has access to the current scope.

So in the end we could just extend $transclude to set the right controllers on the cloned elements and deprecate the transclude function that is given as argument to the compile function.

[psysp]

Thanks for looking into this issue but I'm still a little bit confused. Unfortunately my understanding about transclude-related the angular internals is not that good yet.

If I followed your discussion correctly the workarounds you mentioned needs to be applied to the directive that uses the transclude option. So in my initial scenario it would have to be applied to the ng-include directive but not to the main directive, correct?

[tbosch]

Hi,
here is an updated plunk for the initial plunk that is now working with Angular 1.2.1: http://plnkr.co/edit/oeWTQbzrBGSBzi1iu2WQ?p=preview

Please note that you have to wait until the ngInclude is loaded to change the DOM. Also, ngInclude will create a new element, so you can't just use the $element that was given to the controller...

[psysp]

Ah, I didn't know that 1.2.1 will be release so soon, that's why I asked for an workaround.

Great work!