ng-transclude should not create new sibling scope.

[mbykovskyy]

This is more of a change request and I would like to see what other people think.

In my humble opinion ng-transclude should not create it's own scope or at least have a way to prevent it from doing so. The reason behind this is that a directive which requests transclusion already has means to specify whether or not it wants to have a scope or an isolated scope or no scope at all. It uses ng-transclude directive to mark where it wants to insert the content. When ng-transclude creates it's own sibling scope it kind of breaks the expectations of the directive which defines what kind of scope it wants and there comes the manifestation of the popular 'value' vs 'object.value' confusion.

Here's an example of where new scope doesn't make sense in my opinion:

ui.directive('box', function() {
    return {
        restrict: 'E',
        transclude: true,
        template: '<div ng-transclude/>',
        replace: true,
        scope: {}
    };
});

All this directive wants is replace the <box>content</box> with a <div>content</div> and the content to have the isolated scope.

Creating nested structure of directives like this leads to a scope tree pollution. Here's a plunker example (http://plnkr.co/edit/DwukVGGprFFjQuVY8yTz) of three nested directives which create a scope tree structure like this:

< Scope (002) : ng-app
    < Scope (003) : ng-controller
        < Scope (004) : box
        < Scope (005) : ng-transclude
            < Scope (006) : box
            < Scope (007) : ng-transclude
                < Scope (008) : box
                < Scope (009) : ng-transclude

This behaviour doesn't seem to add any value to its purpose but creates a lot of confusion among beginners.

At the moment I use the following workaround which achieves exactly what the previous example does:

ui.directive('box', function() {
    return {
        restrict: 'E',
        transclude: true,
        template: '<div/>',
        replace: true,
        scope: {},
        link: function(scope, element, attrs, transclude) {
            transclude(scope.$parent, function(content) {
                element.append(content);
            });
        }
    };
});

Here's a plunker example (http://plnkr.co/edit/46v6IBLkhS71L1WbUDFl) which illustrates this concept. It leaves the scope tree nice and tidy:

< Scope (002) : ng-app
    < Scope (003) : ng-controller
        < Scope (004) : box
        < Scope (005) : box
        < Scope (006) : box

And the 2-way binding works the way many expect when they bind 'value' rather than 'object.value'. (I believe that the fact that passing just 'value' works in some cases but not the other and blaming the nature of prototypal inheritance in javascript is not a good excuse. The fact that many people find this behaviour unexpected indicates that there's an architectural flaw.)

I would love to hear what other people think and use cases where they think that creating a new sibling scope for ng-transclude makes sense.

[caitp]

Where does transclude create a new scope? http://plnkr.co/edit/EuHaBR26JgAegQKvwOGH?p=preview I'm not seeing it

[mbykovskyy]

I'm talking about ng-transclude directive. What you have in your example is exactly what my work around does.

[IgorMinar]

this is a valid request. we were considering this for 1.2 but it was getting close to the final release and didn't want to introduce this breaking change.

we should consider it for 1.3

[mbykovskyy]

Nice one! I'm glad that ye've already been considering it.

[troch]

+1 for this. I think my issue is related to this: I use ng-transclude in a directive with forms and I have to go thru scope.$$childHead for accessing the form validation object but I don't have an issue to access my models.

Here is an example: http://fiddle.jshell.net/39cgW/3/

[klamping]

+1 running into this issue today and I hate throwing $parent everywhere.

[caitp]

So, in order to find a solution for this, it seems there are two possibilities

1. alter the ngTransclude directive to specify its scope (it could actually be shrunk down quite a lot more than this, I think --- no controller needed)

or

2. don't create a new scope when the scope isn't specified

So, we could save a few bytes with option 1) and that's nice, 2) would be the smallest solution (3 line deletion or so) and it's not clear to me that there use cases where creating a new scope implicitly makes sense there (maybe there are, but it seems completely contrary to the way transclusion is described in the docs)

---

Or, if you wanted to be super fancy, perhaps you could avoid breaking changes entirely by allowing ngTransclude to specify that it wants a new scope or not, via the attribute value.

Thoughts?

[troch]

I don't get the difference between your 1 and 2 !

[klamping]

Just my opinion, but I think backwards compatibility will be important for this change. I imagine there are lots of apps out there (mine included) that have worked around this scope issue by using things like $parent and scope.$$childHead. Any update which alters this behavior will cause some headaches (but maybe it's better to cause headaches sooner than later).

That said, from a theoretical standpoint, I think it makes more sense for ng-tranclude to have the same scope as the directive by default. The point of transcluding content is that you want it a seamless part of the content. There are times when I have lots of nesting of directives with transcludes, but I still want them to behave as one large component. Having them with all different scopes makes this very tricky.

All just my thoughts. At the very least, just having the option will be a step above the current situation. :)

[caitp]

@troch to clarify, we inject the following into the ngTranscludeDirective's controller:

        // This is the function that is injected as `$transclude`.
        function controllersBoundTransclude(scope, cloneAttachFn) {
          var transcludeControllers;

          // no scope passed
          if (arguments.length < 2) {
            cloneAttachFn = scope;
            scope = undefined;
          }

          if (hasElementTranscludeDirective) {
            transcludeControllers = elementControllers;
          }

          return boundTranscludeFn(scope, cloneAttachFn, transcludeControllers);
        }

The directive calls this function with no scope, and as such, the scope is undefined... Then in boundTranscludeFn, it creates a new scope if the transcludeScope is falsy...

So, what I'm saying is, for 1), we can simply specify the current scope for the transclude function (since this directive is a neighboring directive of anything that might have an isolate scope, this should still give us the original scope).

Alternatively, 2), don't create a new scope and just default to the current scope(in createBoundTranscludeFn) (potentially breaking change, and potentially breaking lots of tests).

Both are pretty simple to do.

[marfarma]

+1

[freshsun]

+1

[CWSpear]

+1

[schovi]

Definitely +1

[sukei]

+1

[apuchkov]

+1

[boneskull]

+1

[fabiosussetto]

+1 please

[caitp]

You know what would be pretty cool, although maybe not totally doable in time for 1.3, ES6 proxies could make transclusion really nice --- keeping up with the properties of the transclusion scope, but having the correct position in the hierarchy.

If the Proxy implementation isn't available, could probably just fall-back to scope.$new(), so it might actually be possible to make this work fairly early on. The tricky thing is that the spec is a bit wobbly.

So, you'd still get unwanted scopes if you want the scope hierarchy to be very clean, but at least you'd have the side effect of data bindings not breaking unexpectedly. I dunno.

[codeaotc]

+1

[alan-morey]

+1

[blackgate]

+1

[mhtsbt]

+1

[guilleferrer]

+1

[sgarbesi]

+1

[amitgupta1202]

+1 seems transcluded directive creates scope even when asked not to do so. example http://plnkr.co/edit/Wn81IBkE87vtigXvjmIa?p=preview

[ljwagerfield]

+1

[valorbreak]

+1

[todthomson]

+1

[balmychan]

+1

[Polooo2]

+1

[princejwesley]

+1

[nadar]

+1

[kasrakhosravi]

+1

[nikkwong]

+1—are there any valid workarounds for this?

[SanderElias]

@nikkwong, there are a couple workarounds posted in this thread. I know I did link in a plunk that showed a workaround.

[petebacondarwin]

OK, so even if this arrives it will not be until 1.5.x

But before that I have a concern. The primary reason for creating a new scope (call it a sibling of the isolate scope, or more accurately a child of the original scope from where the transcluded content comes) is to prevent memory leaks.

Compare this plunker:
http://plnkr.co/edit/3NVxdYGy1AFDvD0M2BYI?p=preview

with a version that reuses the scope from where the original transclusion comes:
http://plnkr.co/edit/MXFz2awcqwQQ7R882Xwz?p=preview

In the second version, as you toggle the transcluded content on and off the number of watchers keeps going up - a memory leak.

[troch]

@petebacondarwin it should definitely be a new scope so it can be destroyed with its watchers. I think most of the grief about transclusion is because ng-transclude would create a sibling scope of the containing scope, making it not possible to access its variables through prototypal inheritence. Or am I wrong?

[amitgupta1202]

We reailzed that the issue is the way angular 2 way binding works, definietly ng-transclude creates a child scope, but there will be numerious occasions where you will find your self in same situation, for example ng-repeat creates child scope. After looking at the angular code, we realized that issue with 2 way binding issue doesnt work property with the object attribute, but works fine with objects itself

Problem : http://plnkr.co/edit/Wn81IBkE87vtigXvjmIa?p=preview

Solution: http://plnkr.co/edit/KShClgQVwIjscXPzVRwR?p=preview

Its not perfect but knowing this solved lot of our issues, never do two way binding on an attribute

Note that this solution is already suggested by mbykovskyy, when he raised issue, I am just giving an example as it took me a while to figure it out.

[SanderElias]

@petebacondarwin It will only be an temporary leak, until the holding scope will get destroyed. Here is (a plunk)[http://plnkr.co/edit/Q587WQnX0u0u7JjhtCxa?p=preview] that shows that if you switch out the transclude-holding-scope, everything will gets released just fine.
Still it's a point that needs attention. Perhaps a large Waring in the docs about this possible leak might be enough to fix it?

[petebacondarwin]

Well any JS leak is only temporary until you refresh the browser ;-)
On 8 Sep 2015 16:41, "Sander Elias" notifications@github.com wrote:

@petebacondarwin https://github.com/petebacondarwin It will only be an
temporary leak, until the holding scope will get destroyed. Here is (a
plunk)[http://plnkr.co/edit/Q587WQnX0u0u7JjhtCxa?p=preview] that shows
that if you switch out the transclude-holding-scope, everything will gets
released just fine.
Still it's a point that needs attention. Perhaps a large Waring in the
docs about this possible leak might be enough to fix it?

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

[frfancha]

@SanderElias Users of our Angular application are busy from begin to end of the day.
We already see that the memory usage is increasing during the day and we must be very careful in what we put on the page, introducing more possible leaks is risky.

[petebacondarwin]

@troch - transclusion actually creates a childscope of the scope where the transcluded content is originally found. This was broken a few versions back (definitely in 1.2) where instead it just created a child of the parent of the current directives scope. This meant that deeply nested transclusions actually got the wrong transclusion scope.

[lgalfaso]

I have to agree with @petebacondarwin, and even when the current behavior is not 100% intuitive, then the current behavior works best to prevent any leak. I am inclined to close this issue as Wont Fix

[mbykovskyy]

Good thing I switched to Ember years ago. :)