Wrong scope used after passing transclude function to $compile

[dlongley]

I've run into something unexpected as demonstrated in this plunker. Specifically, I believe that transcluded elements are being linked to the wrong scope after a manual call to $compile that passes a transclude function. If the transclude function is called on its own -- the cloned elements are linked to the proper scope -- if it's passed to $compile and then the resulting link function is called, the wrong scope is used.

The plunker creates two directives, "lazy-compile" that lazy compiles its contents when a certain "trigger" variable is made truthy. It does this by, during its compilation phase, removing its contents and storing them in the template cache (so there's only ever one copy). Then, it sets up a bind-once watch for the trigger variable. Once the variable is true, the contents are fetched from the cache, cloned, re-appended to the element, and then $compile is called on them, passing any transclude function, if defined.

The second directive, "uses-transclude", is just a widget that uses transclusion and the lazy compilation directive in its template.

On the main page, when a link is clicked, the lazy compilation is triggered. This works properly in the plunker -- but only because of an ugly hack that you can see in script.js. The code there has some comments about what's going on. Essentially, if $compile is called and the transclude function is passed to it, when you run the resulting link function the transcluded elements are linked to the wrong scope. However, if you call the transclude function directly -- the cloned elements are linked to the proper scope. I would expect the same scope to be used in both cases. So, to resolve this problem, the lazy compile directive calls the transclude function itself, saves the scope's parent (prototypical parent, not $parent) from the cloned elements, and then rewrites the transclude function to ensure this same parent scope is used when the transclude function is called later in $compile.

The plunker prints out the "wrong scope" and the "right scope" via console.log once the compilation link is clicked.

Why are the scopes not the same when doing $compile(el, transcludeFn)(scope) vs. transcludeFn(function(clone) {})? Shouldn't the proper scope travel along with the transclude function closure and always be used regardless of where the content is inserted?

[lgalfaso]

@dlongley I am aware of the fact that for transcluded functions, a scope may inherit from a scope that is not it's parent. Now, I looked at the plunker and I am still not able to understand what the specific issue is. In this example, if I change the version of angular to use 1.3-rc1, it shows the same result. Would it be possible for you to update the plunker so it shows the specific issue that rc4 broke?

[jeffbcross]

+1 what @lgalfaso said. A failing unit test would be even better.

Possibly related to this commit (with breaking changes)? fb0c77f

[dlongley]

@lgalfaso, sorry for the confusion; rc4 didn't break this, it's been broken for some time -- I'm unaware of how far back it goes. I've had this code around since before rc4 and I just had notes in there because I changed it to comply with changes to rc4. The problem persists. I'll make some changes to the plunker to indicate the problem a little bit better shortly.

[dlongley]

@lgalfaso -- I've updated the plunker: http://plnkr.co/edit/LTAbdvt1Az1mG362yFy3?p=preview

There are two links to click now:

The first uses the default angular implementation where the transclude function is passed to $compile as expected. This results in the wrong scope (its prototypical parent is incorrect) getting linked to the transcluded elements.

The second includes the patch (a hack really) in the plunker to use the proper scope. It does this by calling the transclude function manually and keeping track of the proper prototypical parent scope and then rewrites the transclude function to use this prototypical parent instead. Angular should be using this parent (prototypical) and, it does, if you call the transclude function manually, but it doesn't if you pass the transclude function to $compile.

The code switches on whether or not a "fix" attribute is set to "true" (see snippet below). The transclude function is rewritten in the patch/hack to use the proper scope.

if(attrs.fix === 'true') {
  transcludeFn = fixTranscludeScope(transcludeFn);
}

// compile cached template and link
$compile(el, transcludeFn)(scope);

[dlongley]

@lgalfaso, @tbosch:

It looks like the transclude function passed to $compile ends up getting passed to createBoundTranscludeFn on line 1303 of compile.js. This occurs when there is no parentBoundTranscludeFn but a transcludeFn has been passed (correct behavior, I think). However, the passed transclude function then gets re-wrapped (it has already been wrapped) and bound to the wrong scope via createBoundTranscludeFn.

The createBoundTranscludeFn function (defined on line 1318) is expecting a transclude function with a different signature. The transclude function passed to $compile is a controllersBoundTransclude function (it has already been bound to a scope and related controllers, etc). and has a signature of function(angular.Scope, cloneAttachFn, parentElement).

The transclude function that is passed is called instead with five parameters (transcludedScope, cloneFn, controllers, previousBoundTranscludeFn, futureParentElement). This matches the publicLinkFn signature (which I presume is the kind of function createBoundTranscludeFn is expecting). Note that there's no argument swapping/manipulation code to deal with these differences; it just appears to be incorrect usage, especially given how the bound behavior might change.

So it looks like several things are going wrong in this particular case that are related to one another:

1. The compiler is mistakenly re-binding a transclude function that is already properly bound.
2. The compiler is passing the transclude function around incorrectly; it is treating it like a publicLinkFn, when it is actually a function that has already been bound to a scope and to controllers (which is the process a publicLinkFn goes through at some point).
3. This results in the transclude function getting called with a transcluded scope that inherits from the wrong prototypical parent. When the transclude function is called, it has been wrapped in a boundTranscludeFn that is given no transcludedScope which causes it to inherit from the bound scope (which is now incorrect and does not match the previously bound scope).
4. The already-bound transclude function has no way to receive the containingScope -- so when creating a transcluded scope as a prototypical child of its bound scope, the new scope will leak.

With this information, I tried a different hack (from the plunker) to resolve the problem (and to avoid having to call the transcludeFn manually to find the scope):

// rewrite transclude to destroy and replace invalid bound transcluded scope
transcludeFn = function(scope, cloneAttachFn, futureParentElement) {
  scope.$destroy();
  return transcludeFn(cloneAttachFn, futureParentElement);
};
// now compile
$compile(scope, transcludeFn);

Which will cause the right scope to be used, but results in a number of other problems because the function arguments don't match properly. For example, as mentioned, the containingScope won't be passed which will result in a memory leak.

Instead, it seems like the compiler should be able to detect whether the transcludeFn is already bound -- and it should be able to specify which containingScope should be used instead. There are a variety of different ways to change the code to make this happen; I'm not sure which is the most appropriate.

[dlongley]

If the above diagnosis of the problem is accurate, one of the causes of the confusion may be that the variable name transcludeFn seems to be used regardless of the type of "transclude function" (bound/controllersBound/publicLink/etc). If true, and they can't all be treated the same way, it may help to do some minor refactoring to use more context-specific names.

[lgalfaso]

It took me a few hours to understand it all, here is a simplified version of the original plunker http://plnkr.co/edit/tWYU90uaRgkvmP9Y5ndG?p=preview
I also created a test based on this example

it('should bind the tranclude function to the original scope when used ' +
    'in a future `$compile` call', function() {
  module(function() {
    directive('usesTransclude', function($compile) {
      return {
        scope: {},
        transclude: true,
        template: '<div><div ng-transclude></div></div>',
        compile: function(tElement, tAttrs) {
          var content = tElement.contents();
          tElement.empty();
          return function(scope, element, attrs, ctrls, transcludeFn) {
            element.append(content);
            $compile(content, transcludeFn)(scope);
          }
        }
      };
    });
  });
  inject(function($compile) {
    element = $compile(
        '<div>' +
          '<div ng-init="outer=true"></div>' +
          '<div uses-transclude>' +
            '<span ng-if="outer">Success</span><span ng-if="!outer">Error</span>' +
          '</div>' +
        '</div>')($rootScope);
    $rootScope.$digest();
    expect(element.text()).toBe('Success');
  });
});

and here is easy to see what the misconception is. The directive usesTransclude asks for an isolate scope, and then it continues the compilation on this new isolate scope (including the transclude function).

Now, this is not what you want to do as you want the parent scope (the one that is able to see outer) to be the scope that is used in the compilation. Remember that scope is isolated. The right way to do this would be for the $compile call to be done on the parent scope (the one that is able to see outer) this is $compile(content, transcludeFn)(scope.$parent);

With this change, there is no need for the fix to kick in.

With all this information, I am highly tempted to think that this issue is invalid.

@petebacondarwin WDYT?

[petebacondarwin]

I believe that @dlongley's analysis is correct.

The problem is that the 2nd (transcludeFn) and 3rd (maxPriority) parameters of $compile are not really part of its public API. They are only there because $compile use recursion into itself to compile sub branches of the HTML. In order to do this it needs to pass down the current transclude function.

You can see that this transcludeFn gets used only by compileNodes, which states that its parameter is:

/*
 * @param {function(angular.Scope, cloneAttachFn=)} transcludeFn A linking function, where the
 *        scope argument is auto-generated to the new child of the transcluded parent scope.
 */

The compiler expects any transcludeFn that is passed in to be unbound, i.e. it is the transclusion from the parent of the current directive and so it will attach that scope. From the directive developers point of view we should only be calling $compile with a piece of HTML (or one or more DOM elements) that are to be compiled.

The question is whether this is a use case that we should be supporting. I was trying to think if there was another way of achieving this lazy compilation that @dlongley is trying to do but I cannot think of anything.

@IgorMinar and @tbosch - did you look at this? It seems to me that a change to the compiler would not be too hard to handle this case...