Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix($compile): do not bind twice transclude functions #9579

Closed
wants to merge 2 commits into from
Closed

fix($compile): do not bind twice transclude functions #9579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
57ee249
refactor($compile): change the parameters orders of transclude functions
lgalfaso Oct 13, 2014
5425777
fix($compile): do not bind twice transclude functions
lgalfaso Oct 13, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1153,7 +1153,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
maxPriority, ignoreDirective, previousCompileContext);
compile.$$addScopeClass($compileNodes);
var namespace = null;
return function publicLinkFn(scope, cloneConnectFn, transcludeControllers, parentBoundTranscludeFn, futureParentElement){
return function publicLinkFn(scope, cloneConnectFn, futureParentElement, transcludeControllers, parentBoundTranscludeFn){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change the order of these parameters?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see it makes it simpler to call later on but I worry that this is not really necessary and is obscuring the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At boundTranscludeFn we do not know if we have controllersBoundTransclude or a publicLinkFn. Reordering the parameters so they match was the simplest alternative

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that if we are going to refactor the params then we should go with an option object style approach:

transcludeFn(scope, cloneConnectFn, {
  transcludeControllers: ...,
  parentBoundTranscludeFn: ...,
  futureParentElement: ...
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

assertArg(scope, 'scope');
if (!namespace) {
namespace = detectNamespaceForChildElements(futureParentElement);
Expand Down Expand Up @@ -1317,16 +1317,23 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

function createBoundTranscludeFn(scope, transcludeFn, previousBoundTranscludeFn, elementTransclusion) {

var boundTranscludeFn = function(transcludedScope, cloneFn, controllers, futureParentElement, containingScope) {
// preserve previously bound scope
if (transcludeFn.$$bound) {
scope = transcludeFn.$$bound;
}

var boundTranscludeFn = function(transcludedScope, cloneFn, futureParentElement, controllers, containingScope) {

if (!transcludedScope) {
transcludedScope = scope.$new(false, containingScope);
transcludedScope.$$transcluded = true;
}

return transcludeFn(transcludedScope, cloneFn, controllers, previousBoundTranscludeFn, futureParentElement);
return transcludeFn(transcludedScope, cloneFn, futureParentElement, controllers, previousBoundTranscludeFn);
};

boundTranscludeFn.$$bound = scope;

return boundTranscludeFn;
}

Expand Down Expand Up @@ -1793,7 +1800,10 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
isolateScope = scope.$new(true);
}

transcludeFn = boundTranscludeFn && controllersBoundTransclude;
if (boundTranscludeFn) {
transcludeFn = controllersBoundTransclude;
transcludeFn.$$bound = boundTranscludeFn.$$bound;
}
if (controllerDirectives) {
// TODO: merge `controllers` and `elementControllers` into single object.
controllers = {};
Expand Down Expand Up @@ -1953,7 +1963,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
var transcludeControllers;

// No scope passed in:
if (!isScope(scope)) {
if (!isScope(scope) && !isUndefined(scope)) {
futureParentElement = cloneAttachFn;
cloneAttachFn = scope;
scope = undefined;
Expand All @@ -1965,7 +1975,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
if (!futureParentElement) {
futureParentElement = hasElementTranscludeDirective ? $element.parent() : $element;
}
return boundTranscludeFn(scope, cloneAttachFn, transcludeControllers, futureParentElement, scopeToChild);
return boundTranscludeFn(scope, cloneAttachFn, futureParentElement, transcludeControllers, scopeToChild);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/ng/directive/ngInclude.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ var ngIncludeFillContentDirective = ['$compile',
$compile(jqLiteBuildFragment(ctrl.template, document).childNodes)(scope,
function namespaceAdaptedClone(clone) {
$element.append(clone);
}, undefined, undefined, $element);
}, $element);
return;
}

Expand Down
73 changes: 73 additions & 0 deletions test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5124,6 +5124,79 @@ describe('$compile', function() {
});


it('should bind the tranclude function to the original scope when used ' +
'in a future `$compile` call', function() {

function countScopes($rootScope) {
return [$rootScope].concat(
getChildScopes($rootScope)
).length;
}

function getChildScopes(scope) {
var children = [];
if (!scope.$$childHead) { return children; }
var childScope = scope.$$childHead;
do {
children.push(childScope);
children = children.concat(getChildScopes(childScope));
} while ((childScope = childScope.$$nextSibling));
return children;
}


module(function() {
directive('isolate', function() {
return {
scope: {}
};
});
directive('usesTransclude', function($compile) {
return {
scope: {foo: '='},
transclude: true,
template: '<div><div ng-if="foo"><div ng-transclude></div></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 foo="foo">' +
'<div uses-transclude foo="foo">' +
'<span ng-if="outer">Success</span><span ng-if="!outer">Error</span>' +
'</div>' +
'</div>' +
'</div>')($rootScope);
$rootScope.foo = false;
$rootScope.$digest();
expect(countScopes($rootScope)).toBe(2);
expect(element.text()).toBe('');
$rootScope.foo = true;
$rootScope.$digest();
expect(countScopes($rootScope)).toBe(8);
expect(element.text()).toBe('Success');
$rootScope.foo = false;
$rootScope.$digest();
expect(countScopes($rootScope)).toBe(2);
expect(element.text()).toBe('');
$rootScope.foo = true;
$rootScope.$digest();
expect(countScopes($rootScope)).toBe(8);
expect(element.text()).toBe('Success');
});
});


// see issue https://github.com/angular/angular.js/issues/9095
describe('removing a transcluded element', function() {

Expand Down