Wrong scope used after passing transclude function to $compile #9413
Comments
|
@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? |
|
@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. |
|
@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 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 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); |
|
It looks like the transclude function passed to The The transclude function that is passed is called instead with five parameters So it looks like several things are going wrong in this particular case that are related to one another:
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 Instead, it seems like the compiler should be able to detect whether the |
|
If the above diagnosis of the problem is accurate, one of the causes of the confusion may be that the variable name |
|
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 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 Now, this is not what you want to do as you want the parent scope (the one that is able to see 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? |
|
I believe that @dlongley's analysis is correct. The problem is that the 2nd ( You can see that this 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... |
|
@petebacondarwin Lazy compilation is currently needed to implement recursive transcluding directives. I once wrote an issue about the lack of support for recursive transcluding directives here. I'd like either native recursion or lazy compilation to be implemented. It can be either because I'm not aware of another use of lazy compilation, I think people only use it to implement recursive transcluding directives. |
|
@callmeStriking, another simple use case is performance. You may want to prevent or delay compilation of certain parts of a complex interface until a particular event is triggered. |
This seems like it would only work in a very specific-case; namely when the |
Do not double bound transclude functions that are passed to the compiler that are already bounded Closes angular#9413
|
Created a PR to have the discussion if the |
Do not double bind transclude functions that are passed to the compiler that were already bound Closes angular#9413
Do not double bind transclude functions that are passed to the compiler that were already bound Closes angular#9413
Do not double bind transclude functions that are passed to the compiler that were already bound Worked with @dlongley on the implementation Closes angular#9413
|
Hi, However, there is a simpler workaround: Create the following wrapper around the (see this punker: http://plnkr.co/edit/7quB43McPIaSo5YzguWo?p=preview) @lgalfaso To support this use case without this wrapper, the correct way would be to pass the Closing this, but thanks for all the analyzing work! |
|
@tbosch, that workaround does not work -- I mentioned it earlier #9413 (comment). It leaks because of the new usage of containing scopes as $parents. |
|
@dlongley Yes, you are right. But really, the right way to do this would be the way I explained above: So let's do the following:
Then your example would be fixed using Reopening this. Note: This is not a breaking change as only the first 2 arguments of the |
|
I.e. we can deal with this sometime later in 1.3... |
|
To remove all of the wrapping (and also the problem with the
So we would keep the original @dlongley Could you try this approach as well? Note: As discussed in #9579 we might want to change the arguments of the |
|
@tbosch, sure, I'll give it a shot soon. |
|
@tbosch -- I think it's working. No memory leaks either. I'm going to clean this up and if I can get the tests running, do a PR with these changes. This PR will not switch to using |
Do not rebind parent bound transclude functions passed to the link function returned from `$compile`. Change parameter order of ```publicLinkFn``` to allow parent bound transclude functions to be passed (do not make public yet). The current `$compile` public API documentation indicates that a transclude function may be passed as a second parameter, but it is not clear that this is not the same function that is given to directive link functions as the `transcludeFn` parameter. This parameter must be passed in order to implement, for example, lazy compilation. In order to pass the `transcludeFn` parameter given to a directive's link function, it must be passed to the result of a call to `$compile` (i.e. `publicLinkFn`). Doing so, however, would cause two bugs. First, the passed transclude function would get rebound, improperly changing its scope from its original binding. Second, the `containingScope` would not be updated and the wrong `$parent` would be assigned to the `transcludedScope` resulting in a memory leak. This patch fixes both of these issues. This patch does not expose a new public API for `publicLinkFn`, rather, it leaves this to a later change that better considers how that public API should look. Thanks to @lgalfaso, @tbosch, and @petebacondarwin. Related to angular#9413
Do not rebind parent bound transclude functions passed to the link function returned from `$compile`. Change parameter order of ```publicLinkFn``` to allow parent bound transclude functions to be passed (do not make public yet). The current `$compile` public API documentation indicates that a transclude function may be passed as a second parameter, but it is not clear that this is not the same function that is given to directive link functions as the `transcludeFn` parameter. This parameter must be passed in order to implement, for example, lazy compilation. In order to pass the `transcludeFn` parameter given to a directive's link function, it must be passed to the result of a call to `$compile` (i.e. `publicLinkFn`). Doing so, however, would cause two bugs. First, the passed transclude function would get rebound, improperly changing its scope from its original binding. Second, the `containingScope` would not be updated and the wrong `$parent` would be assigned to the `transcludedScope` resulting in a memory leak. This patch fixes both of these issues. This patch does not expose a new public API for `publicLinkFn`, rather, it leaves this to a later change that better considers how that public API should look. Thanks to @lgalfaso, @tbosch, and @petebacondarwin. Related to angular#9413
Do not rebind parent bound transclude functions passed to the link function returned from `$compile`. Change parameter order of ```publicLinkFn``` to allow parent bound transclude functions to be passed (do not make public yet). The current `$compile` public API documentation indicates that a transclude function may be passed as a second parameter, but it is not clear that this is not the same function that is given to directive link functions as the `transcludeFn` parameter. This parameter must be passed in order to implement, for example, lazy compilation. In order to pass the `transcludeFn` parameter given to a directive's link function, it must be passed to the result of a call to `$compile` (i.e. `publicLinkFn`). Doing so, however, would cause two bugs. First, the passed transclude function would get rebound, improperly changing its scope from its original binding. Second, the `containingScope` would not be updated and the wrong `$parent` would be assigned to the `transcludedScope` resulting in a memory leak. This patch fixes both of these issues. This patch does not expose a new public API for `publicLinkFn`, rather, it leaves this to a later change that better considers how that public API should look. Thanks to @lgalfaso, @tbosch, and @petebacondarwin. Related to angular#9413
Do not rebind parent bound transclude functions passed to the link function returned from `$compile`. Change parameter order of ```publicLinkFn``` to allow parent bound transclude functions to be passed (do not make public yet). The current `$compile` public API documentation indicates that a transclude function may be passed as a second parameter, but it is not clear that this is not the same function that is given to directive link functions as the `transcludeFn` parameter. This parameter must be passed in order to implement, for example, lazy compilation. In order to pass the `transcludeFn` parameter given to a directive's link function, it must be passed to the result of a call to `$compile` (i.e. `publicLinkFn`). Doing so, however, would cause two bugs. First, the passed transclude function would get rebound, improperly changing its scope from its original binding. Second, the `containingScope` would not be updated and the wrong `$parent` would be assigned to the `transcludedScope` resulting in a memory leak. This patch fixes both of these issues. This patch does not expose a new public API for `publicLinkFn`, rather, it leaves this to a later change that better considers how that public API should look. Thanks to @lgalfaso, @tbosch, and @petebacondarwin. Related to angular#9413
Use `options` object hash style parameter for publicLinkFn. Expose `options` parameter as part of the public API. Closes angular#9413.
Do not rebind parent bound transclude functions passed to the link function returned from `$compile`. Change parameter order of ```publicLinkFn``` to allow parent bound transclude functions to be passed (do not make public yet). The current `$compile` public API documentation indicates that a transclude function may be passed as a second parameter, but it is not clear that this is not the same function that is given to directive link functions as the `transcludeFn` parameter. This parameter must be passed in order to implement, for example, lazy compilation. In order to pass the `transcludeFn` parameter given to a directive's link function, it must be passed to the result of a call to `$compile` (i.e. `publicLinkFn`). Doing so, however, would cause two bugs. First, the passed transclude function would get rebound, improperly changing its scope from its original binding. Second, the `containingScope` would not be updated and the wrong `$parent` would be assigned to the `transcludedScope` resulting in a memory leak. This patch fixes both of these issues. This patch does not expose a new public API for `publicLinkFn`, rather, it leaves this to a later change that better considers how that public API should look. Thanks to @lgalfaso, @tbosch, and @petebacondarwin. Related to angular#9413
Use `options` object hash style parameter for publicLinkFn. Expose `options` parameter as part of the public API. Closes angular#9413
Do not rebind parent bound transclude functions passed to the link function returned from `$compile`. Change parameter order of ```publicLinkFn``` to allow parent bound transclude functions to be passed (do not make public yet). The current `$compile` public API documentation indicates that a transclude function may be passed as a second parameter, but it is not clear that this is not the same function that is given to directive link functions as the `transcludeFn` parameter. This parameter must be passed in order to implement, for example, lazy compilation. In order to pass the `transcludeFn` parameter given to a directive's link function, it must be passed to the result of a call to `$compile` (i.e. `publicLinkFn`). Doing so, however, would cause two bugs. First, the passed transclude function would get rebound, improperly changing its scope from its original binding. Second, the `containingScope` would not be updated and the wrong `$parent` would be assigned to the `transcludedScope` resulting in a memory leak. This patch fixes both of these issues. This patch does not expose a new public API for `publicLinkFn`, rather, it leaves this to a later change that better considers how that public API should look. Thanks to @lgalfaso, @tbosch, and @petebacondarwin. Related to angular#9413
Use `options` object hash style parameter for publicLinkFn. Expose `options` parameter as part of the public API. Closes angular#9413
Do not rebind parent bound transclude functions passed to the link function returned from `$compile`. Change parameter order of ```publicLinkFn``` to allow parent bound transclude functions to be passed (do not make public yet). The current `$compile` public API documentation indicates that a transclude function may be passed as a second parameter, but it is not clear that this is not the same function that is given to directive link functions as the `transcludeFn` parameter. This parameter must be passed in order to implement, for example, lazy compilation. In order to pass the `transcludeFn` parameter given to a directive's link function, it must be passed to the result of a call to `$compile` (i.e. `publicLinkFn`). Doing so, however, would cause two bugs. First, the passed transclude function would get rebound, improperly changing its scope from its original binding. Second, the `containingScope` would not be updated and the wrong `$parent` would be assigned to the `transcludedScope` resulting in a memory leak. This patch fixes both of these issues. This patch does not expose a new public API for `publicLinkFn`, rather, it leaves this to a later change that better considers how that public API should look. Thanks to @lgalfaso, @tbosch, and @petebacondarwin. Related to angular#9413
Use `options` object hash style parameter for publicLinkFn. Expose `options` parameter as part of the public API. Closes angular#9413
Do not rebind parent bound transclude functions passed to the link function returned from `$compile`. Change parameter order of ```publicLinkFn``` to allow parent bound transclude functions to be passed (do not make public yet). The current `$compile` public API documentation indicates that a transclude function may be passed as a second parameter, but it is not clear that this is not the same function that is given to directive link functions as the `transcludeFn` parameter. This parameter must be passed in order to implement, for example, lazy compilation. In order to pass the `transcludeFn` parameter given to a directive's link function, it must be passed to the result of a call to `$compile` (i.e. `publicLinkFn`). Doing so, however, would cause two bugs. First, the passed transclude function would get rebound, improperly changing its scope from its original binding. Second, the `containingScope` would not be updated and the wrong `$parent` would be assigned to the `transcludedScope` resulting in a memory leak. This patch fixes both of these issues. This patch does not expose a new public API for `publicLinkFn`, rather, it leaves this to a later change that better considers how that public API should look. Thanks to @lgalfaso, @tbosch, and @petebacondarwin. Related to angular#9413
Use `options` object hash style parameter for publicLinkFn. Expose `options` parameter as part of the public API. Closes angular#9413
Do not rebind parent bound transclude functions passed to the link function returned from `$compile`. Change parameter order of ```publicLinkFn``` to allow parent bound transclude functions to be passed (do not make public yet). The current `$compile` public API documentation indicates that a transclude function may be passed as a second parameter, but it is not clear that this is not the same function that is given to directive link functions as the `transcludeFn` parameter. This parameter must be passed in order to implement, for example, lazy compilation. In order to pass the `transcludeFn` parameter given to a directive's link function, it must be passed to the result of a call to `$compile` (i.e. `publicLinkFn`). Doing so, however, would cause two bugs. First, the passed transclude function would get rebound, improperly changing its scope from its original binding. Second, the `containingScope` would not be updated and the wrong `$parent` would be assigned to the `transcludedScope` resulting in a memory leak. This patch fixes both of these issues. This patch does not expose a new public API for `publicLinkFn`, rather, it leaves this to a later change that better considers how that public API should look. Thanks to @lgalfaso, @tbosch, and @petebacondarwin. Related to angular#9413
Use `options` object hash style parameter for publicLinkFn. Expose `options` parameter as part of the public API. Closes angular#9413
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
$compileand 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
$compileis 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$compileis 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.logonce 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?The text was updated successfully, but these errors were encountered: