Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ng-transclude should not create new sibling scope. #5489

Closed
mbykovskyy opened this issue Dec 20, 2013 · 69 comments

Comments

@mbykovskyy
Copy link

commented Dec 20, 2013

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

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2013

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

@mbykovskyy

This comment has been minimized.

Copy link
Author

commented Dec 20, 2013

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

@ghost ghost assigned IgorMinar Dec 20, 2013
@IgorMinar

This comment has been minimized.

Copy link
Member

commented Dec 20, 2013

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

This comment has been minimized.

Copy link
Author

commented Dec 20, 2013

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

@troch

This comment has been minimized.

Copy link

commented Dec 20, 2013

+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

This comment has been minimized.

Copy link

commented Dec 20, 2013

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

@caitp

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2013

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

  1. 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

This comment has been minimized.

Copy link

commented Dec 21, 2013

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

@klamping

This comment has been minimized.

Copy link

commented Dec 21, 2013

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

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2013

@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

This comment has been minimized.

Copy link

commented Dec 23, 2013

+1

2 similar comments
@freshsun

This comment has been minimized.

Copy link

commented Dec 25, 2013

+1

@CWSpear

This comment has been minimized.

Copy link
Contributor

commented Dec 25, 2013

+1

@schovi

This comment has been minimized.

Copy link

commented Jan 3, 2014

Definitely +1

@sukei

This comment has been minimized.

Copy link

commented Jan 3, 2014

+1

@apuchkov

This comment has been minimized.

Copy link

commented Jan 8, 2014

+1

1 similar comment
@boneskull

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2014

+1

@fabiosussetto

This comment has been minimized.

Copy link

commented Jan 9, 2014

+1 please

@caitp

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2014

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

This comment has been minimized.

Copy link

commented Jan 28, 2014

+1

@alan-morey

This comment has been minimized.

Copy link

commented Feb 27, 2014

+1

3 similar comments
@blackgate

This comment has been minimized.

Copy link

commented Mar 18, 2014

+1

@mhtsbt

This comment has been minimized.

Copy link

commented Mar 28, 2014

+1

@guilleferrer

This comment has been minimized.

Copy link

commented Apr 16, 2014

+1

@sergiofilhowz

This comment has been minimized.

Copy link

commented Feb 4, 2015

+1

1 similar comment
@sgarbesi

This comment has been minimized.

Copy link

commented Feb 8, 2015

+1

@amitgupta1202

This comment has been minimized.

Copy link

commented Feb 17, 2015

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

@ljwagerfield

This comment has been minimized.

Copy link

commented Feb 17, 2015

+1

7 similar comments
@valorbreak

This comment has been minimized.

Copy link

commented Mar 13, 2015

+1

@todthomson

This comment has been minimized.

Copy link

commented Mar 17, 2015

+1

@balmychan

This comment has been minimized.

Copy link

commented Mar 18, 2015

+1

@Polooo2

This comment has been minimized.

Copy link

commented May 5, 2015

+1

@princejwesley

This comment has been minimized.

Copy link

commented May 9, 2015

+1

@nadar

This comment has been minimized.

Copy link

commented May 18, 2015

+1

@kasrakhosravi

This comment has been minimized.

Copy link

commented Jun 3, 2015

+1

@nikkwong

This comment has been minimized.

Copy link

commented Jun 7, 2015

+1—are there any valid workarounds for this?

@SanderElias

This comment has been minimized.

Copy link
Member

commented Jun 12, 2015

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

rikukissa added a commit to solita/livijuku-front that referenced this issue Jun 16, 2015
Scopeton transcludaaminen komponentteihin jotka ei tarvi omaa scopea.
angular/angular.js#5489
@petebacondarwin petebacondarwin modified the milestones: 1.5.x - migration-facilitation, 1.3.x - superluminal-nudge Sep 8, 2015
@petebacondarwin

This comment has been minimized.

Copy link
Member

commented Sep 8, 2015

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

This comment has been minimized.

Copy link

commented Sep 8, 2015

@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

This comment has been minimized.

Copy link

commented Sep 8, 2015

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

This comment has been minimized.

Copy link
Member

commented Sep 8, 2015

@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

This comment has been minimized.

Copy link
Member

commented Sep 8, 2015

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

This comment has been minimized.

Copy link

commented Sep 8, 2015

@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

This comment has been minimized.

Copy link
Member

commented Sep 9, 2015

@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

This comment has been minimized.

Copy link
Member

commented Sep 10, 2015

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

This comment has been minimized.

Copy link
Author

commented Feb 17, 2016

Good thing I switched to Ember years ago. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.