fix($compile): do not rebind parent bound transclude functions #9605

dlongley · 2014-10-14T03:41:03Z

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 #9413

mary-poppins · 2014-10-14T04:09:36Z

I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let us know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

tbosch · 2014-10-14T04:20:49Z

src/ng/compile.js

@@ -1953,7 +1953,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
          var transcludeControllers;

          // No scope passed in:
-          if (!isScope(scope)) {
+          if (!isScope(scope) && !isUndefined(scope)) {


Do we still need this when we are not wrapping anything?

Yes, it's still necessary, but not for the reason I originally thought. Let me take a closer look.

I think there may be a recursion case that its handling where there's still wrapping going on. I'll take another look when I get a chance.

tbosch · 2014-10-14T04:25:51Z

Could you also change the docs for $compile:

deprecate the transcludeFn that can be passed into $compile
add transcludeFn in resulting linking function of calling $compile

tbosch · 2014-10-14T04:31:36Z

Btw, the transcludeFn that is passed into the compile function of directives has been deprecated with the following explanation that you can use for the deprecation explanation above as well:

Note: The transclude function that is passed to the compile function is deprecated, as it e.g. does not know about the right outer scope. Please use the transclude function that is passed to the link function instead.

dlongley · 2014-10-14T06:37:13Z

Btw, I signed the CLA with an account that lists the same email address as my github account, so I'm not sure why mary-poppins is complaining. It isn't the primary email address, but it is listed.

dlongley · 2014-10-14T06:39:49Z

@tbosch, I was thinking that deprecating the transcludeFn for $compile and adding it for the resulting linking function would be done in another PR that exposes the new public API. That new PR would also switch the publicLinkFn to use an options hash parameter. I thought it would be cleaner to do that way, but, if you'd prefer, I can include it in this PR.

dlongley · 2014-10-14T07:13:17Z

Fixed the last case of wrapping ... there was some tricky recursion that wasn't handled before.

lgalfaso · 2014-10-14T07:32:38Z

@dlongley I see that you updated the PR for a fix but you did not added/modified any of the existing tests. Can you please add a test that shows this recursion case that you are talking about

dlongley · 2014-10-14T13:48:09Z

@lgalfaso, done.

dlongley · 2014-10-14T15:00:22Z

@tbosch, if you'd prefer me to add a commit to this PR with the doc change, I'll do so -- and also use the options parameter design discussed in #9579. Otherwise I can do that in another PR.

tbosch · 2014-10-14T16:24:39Z

src/ng/compile.js

+          // a `controllersBoundTransclude` function, so prevent double-wrapping
+          // by using the previously-wrapped boundTranscludeFn.
+          var unwrappedFn = boundTranscludeFn.$$boundTransclude || boundTranscludeFn;
+          controllersBoundTransclude.$$boundTransclude = unwrappedFn;


This second assign for $$boundTransclude is not needed as we already did it above

Fixing this.

tbosch · 2014-10-14T16:29:48Z

Could you just add a second commit to this PR that adds the options hash parameter to the publicLinkFn and also adds the documentation changes?

tbosch · 2014-10-14T16:31:47Z

test/ng/compileSpec.js

@@ -5124,6 +5124,122 @@ describe('$compile', function() {
      });


+      // see issue https://github.com/angular/angular.js/issues/9413
+      describe('passing a parent bound transclude function to the link ' +


Please think about how to simplify this test so that it only tests the changed behavior and is easier to understand?

That is just a grouping for the two tests nested in it that are both related to passing a parent bound transclude function. Is that not a good summary title for those two tests?

I was following the example below ("removing a transcluded element") that then has nested tests below it.

Sorry, I did not mean the test suite but the tests themselves.

E.g. do we need a directive with terminal and priority and another directive with isolate scope to test this?

Ok, terminal, priority, and the isolate scope have been removed. The recursive directive test was added per @lgalfaso's request.

And do we really need 2 directives to test this?

I can reuse the toggle directive (instead of the recursive directive), but that may end up being less clear with double-toggling. What's being tested is recursive transclusion.

I thought about this for a bit. I think the second directive is necessary to make clear what's being tested. This test is specifically for recursive transclusion, not for directive recursion.

tbosch · 2014-10-14T16:34:24Z

Sorry about the CLA hickup, our process should verify you soon.

dlongley · 2014-10-14T16:37:04Z

Could you just add a second commit to this PR that adds the options hash parameter to the publicLinkFn and also adds the documentation changes?

Yeah, will do.

tbosch · 2014-10-15T17:15:09Z

Thanks. Sorry, but this will probably have to wait until after ngEurope (2 weeks)...

dlongley · 2014-10-15T17:59:05Z

@tbosch, ok, thanks, no problem. Just wanted to be clear that I think it's ready when you guys are.

dlongley · 2014-10-29T15:23:22Z

@tbosch -- I've sorted out the conflicts with this PR and it's ready for auto-merging again.

dlongley · 2014-11-04T18:10:20Z

@tbosch, could this be merged now and make it into 1.3.2?

caitp · 2014-11-04T18:17:32Z

The fix itself looks good to me --- the tests are really long and hard to read (as noted in the discussion). They can be simplified at least a little bit once #9871 lands, so that the content of the tests is more focused on what is actually being tested.

Still need review from @tbosch though.

caitp · 2014-11-04T18:18:41Z

test/ng/compileSpec.js

+            };
+          });
+          directive('toggle', function() {
+            return {


use valueFn({ ... your directive ... }) here to make it slightly smaller and easier to read (instead of function() { return { ... }; }

petebacondarwin · 2014-11-05T14:54:31Z

@caitp - do you think this is ready to merge? @tbosch can you review?
If not sure then we should leave it in Milestone 1.3.x and look at it next week. I would like to limit what we need to achieve in 1.3.2.

caitp · 2014-11-05T14:58:28Z

i said before, the fix looks good to me, but should probably get approval from tbosch

petebacondarwin · 2014-11-05T17:07:50Z

test/ng/compileSpec.js

+            children = children.concat(getChildScopes(childScope));
+          } while ((childScope = childScope.$$nextSibling));
+          return children;
+        }


countScopes() and getChildScopes() are already available in this file. I will move the other copy and remove these.

Igor is adding these to ngMock --- so I wouldn't worry about it. We should land Igor's PR for that though

Landed #9871 --- please just use (Scope.$countChildScopes() + 1) now

@lgalfaso

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

dlongley · 2014-11-05T18:46:38Z

Uses Scope.$countChildScopes from #9871 now.

petebacondarwin · 2014-11-05T18:58:31Z

Gotcha!

caitp · 2014-11-05T19:14:39Z

alright, I don't think the tests are really getting any simpler, which is too bad, but it's hard to do that with the compiler. LGTM

petebacondarwin · 2014-11-05T20:42:18Z

Merging.

petebacondarwin · 2014-11-05T21:31:54Z

I squashed the two commits into one because the first actually fails without the second and it was too complicated to try to work out which bits needed to be moved between the commits to get them both to pass. The change to the new ngMock helpers, I put into a separate commit.

dlongley · 2014-11-05T21:34:26Z

Awesome! Thanks, everyone.

petebacondarwin · 2014-11-05T21:36:25Z

Landed as 841c090

mary-poppins added cla: no and removed cla: yes labels Oct 14, 2014

tbosch reviewed Oct 14, 2014
View reviewed changes

tbosch self-assigned this Oct 14, 2014

tbosch added this to the Backlog milestone Oct 14, 2014

tbosch added post-1.3.0 component: $compile type: bug frequency: low severity: inconvenient severity: confusing and removed severity: inconvenient labels Oct 14, 2014

dlongley force-pushed the fix-rebound-transcluded-scope branch from 3859ea0 to 4cdb797 Compare October 14, 2014 07:09

dlongley force-pushed the fix-rebound-transcluded-scope branch from 4cdb797 to 1011393 Compare October 14, 2014 13:47

tbosch reviewed Oct 14, 2014
View reviewed changes

dlongley force-pushed the fix-rebound-transcluded-scope branch from 1011393 to 83857c3 Compare October 14, 2014 16:59

petebacondarwin force-pushed the master branch from 4dd5a20 to 998c61c Compare October 19, 2014 10:58

dlongley force-pushed the fix-rebound-transcluded-scope branch from 76662bd to 98f65da Compare October 27, 2014 02:49

caitp reviewed Nov 4, 2014
View reviewed changes

dlongley force-pushed the fix-rebound-transcluded-scope branch from 98f65da to 1b567f5 Compare November 4, 2014 18:58

petebacondarwin modified the milestones: 1.3.x, Backlog Nov 4, 2014

caitp modified the milestones: 1.3.2, 1.3.x Nov 4, 2014

petebacondarwin reviewed Nov 5, 2014
View reviewed changes

dlongley force-pushed the fix-rebound-transcluded-scope branch from 1b567f5 to bb4e94f Compare November 5, 2014 18:42

dlongley added 2 commits November 5, 2014 13:43

feat($compile): expose publicLinkFn options

ff06bbd

Use `options` object hash style parameter for publicLinkFn. Expose `options` parameter as part of the public API. Closes angular#9413

dlongley force-pushed the fix-rebound-transcluded-scope branch from bb4e94f to ff06bbd Compare November 5, 2014 18:43

petebacondarwin assigned petebacondarwin and unassigned tbosch Nov 5, 2014

petebacondarwin closed this Nov 5, 2014

fix($compile): do not rebind parent bound transclude functions #9605

fix($compile): do not rebind parent bound transclude functions #9605

Conversation

dlongley commented Oct 14, 2014

mary-poppins commented Oct 14, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbosch commented Oct 14, 2014

tbosch commented Oct 14, 2014

dlongley commented Oct 14, 2014

dlongley commented Oct 14, 2014

dlongley commented Oct 14, 2014

lgalfaso commented Oct 14, 2014

dlongley commented Oct 14, 2014

dlongley commented Oct 14, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbosch commented Oct 14, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbosch commented Oct 14, 2014

dlongley commented Oct 14, 2014

tbosch commented Oct 15, 2014

dlongley commented Oct 15, 2014

dlongley commented Oct 29, 2014

dlongley commented Nov 4, 2014

caitp commented Nov 4, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Nov 5, 2014

caitp commented Nov 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlongley commented Nov 5, 2014

petebacondarwin commented Nov 5, 2014

caitp commented Nov 5, 2014

petebacondarwin commented Nov 5, 2014

petebacondarwin commented Nov 5, 2014

dlongley commented Nov 5, 2014

petebacondarwin commented Nov 5, 2014