Skip to content

Commit

Permalink
fix($compile): abort compilation when duplicate element transclusion
Browse files Browse the repository at this point in the history 
Issue an error and abort compilation when two directives that ask for transclusion are found
on a single element. This configuration is not supported and we previously failed to issue
the error because in the case of element transclusion the compilation is re-started and this
caused the compilation context to be lost.

The ngRepeat directive has been special-cased to bypass this warning because it knows how to
handle this scenario internally.

This is not an ideal solution to the problem of multiple transclusions per element, we are
hoping to have this configuration supported by the compiler in the future. See angular#4357.

Closes angular#3893
Closes angular#4217
Closes angular#3307
  • Loading branch information
@IgorMinar @jamesdaily
IgorMinar authored and jamesdaily committed Jan 27, 2014
1 parent dcd36f1 commit aceef56
Show file tree
Hide file tree
Showing 3 changed files with 142 additions and 29 deletions.
50 changes: 33 additions & 17 deletions src/ng/compile.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -469,7 +469,7 @@ function $CompileProvider($provide) {

//================================

function compile($compileNodes, transcludeFn, maxPriority, ignoreDirective) {
function compile($compileNodes, transcludeFn, maxPriority, ignoreDirective, previousCompileContext) {
if (!($compileNodes instanceof jqLite)) {
// jquery always rewraps, whereas we need to preserve the original selector so that we can modify it.
$compileNodes = jqLite($compileNodes);
Expand All @@ -481,7 +481,7 @@ function $CompileProvider($provide) {
$compileNodes[index] = node = jqLite(node).wrap('<span></span>').parent()[0];
}
});
var compositeLinkFn = compileNodes($compileNodes, transcludeFn, $compileNodes, maxPriority, ignoreDirective);
var compositeLinkFn = compileNodes($compileNodes, transcludeFn, $compileNodes, maxPriority, ignoreDirective, previousCompileContext);
return function publicLinkFn(scope, cloneConnectFn){
assertArg(scope, 'scope');
// important!!: we must call our jqLite.clone() since the jQuery one is trying to be smart
Expand Down Expand Up @@ -528,7 +528,7 @@ function $CompileProvider($provide) {
* @param {number=} max directive priority
* @returns {?function} A composite linking function of all of the matched directives or null.
*/
function compileNodes(nodeList, transcludeFn, $rootElement, maxPriority, ignoreDirective) {
function compileNodes(nodeList, transcludeFn, $rootElement, maxPriority, ignoreDirective, previousCompileContext) {
var linkFns = [],
nodeLinkFn, childLinkFn, directives, attrs, linkFnFound;

Expand All @@ -539,7 +539,7 @@ function $CompileProvider($provide) {
directives = collectDirectives(nodeList[i], [], attrs, i == 0 ? maxPriority : undefined, ignoreDirective);

nodeLinkFn = (directives.length)
? applyDirectivesToNode(directives, nodeList[i], attrs, transcludeFn, $rootElement, null, [], [])
? applyDirectivesToNode(directives, nodeList[i], attrs, transcludeFn, $rootElement, null, [], [], previousCompileContext)
: null;

childLinkFn = (nodeLinkFn && nodeLinkFn.terminal || !nodeList[i].childNodes || !nodeList[i].childNodes.length)
Expand All @@ -550,6 +550,7 @@ function $CompileProvider($provide) {
linkFns.push(nodeLinkFn);
linkFns.push(childLinkFn);
linkFnFound = (linkFnFound || nodeLinkFn || childLinkFn);
previousCompileContext = null; //use the previous context only for the first element in the virtual group
}

// return a linking function if we have found anything, null otherwise
Expand Down Expand Up @@ -750,23 +751,26 @@ function $CompileProvider($provide) {
* scope argument is auto-generated to the new child of the transcluded parent scope.
* @param {JQLite} jqCollection If we are working on the root of the compile tree then this
* argument has the root jqLite array so that we can replace nodes on it.
* @param {Object=} ignoreDirective An optional directive that will be ignored when compiling
* @param {Object=} originalReplaceDirective An optional directive that will be ignored when compiling
* the transclusion.
* @param {Array.<Function>} preLinkFns
* @param {Array.<Function>} postLinkFns
* @param {Object} previousCompileContext Context used for previous compilation of the current node
* @returns linkFn
*/
function applyDirectivesToNode(directives, compileNode, templateAttrs, transcludeFn, jqCollection,
originalReplaceDirective, preLinkFns, postLinkFns) {
originalReplaceDirective, preLinkFns, postLinkFns, previousCompileContext) {
previousCompileContext = previousCompileContext || {};

var terminalPriority = -Number.MAX_VALUE,
newScopeDirective = null,
newIsolateScopeDirective = null,
templateDirective = null,
newScopeDirective,
newIsolateScopeDirective = previousCompileContext.newIsolateScopeDirective,
templateDirective = previousCompileContext.templateDirective,
$compileNode = templateAttrs.$$element = jqLite(compileNode),
directive,
directiveName,
$template,
transcludeDirective,
transcludeDirective = previousCompileContext.transcludeDirective,
replaceDirective = originalReplaceDirective,
childTranscludeFn = transcludeFn,
controllerDirectives,
Expand Down Expand Up @@ -815,19 +819,27 @@ function $CompileProvider($provide) {
}

if (directiveValue = directive.transclude) {
assertNoDuplicate('transclusion', transcludeDirective, directive, $compileNode);
transcludeDirective = directive;
// Special case ngRepeat so that we don't complain about duplicate transclusion, ngRepeat knows how to handle
// this on its own.
if (directiveName !== 'ngRepeat') {
assertNoDuplicate('transclusion', transcludeDirective, directive, $compileNode);
transcludeDirective = directive;
}

if (directiveValue == 'element') {
terminalPriority = directive.priority;
$template = groupScan(compileNode, attrStart, attrEnd)
$template = groupScan(compileNode, attrStart, attrEnd);
$compileNode = templateAttrs.$$element =
jqLite(document.createComment(' ' + directiveName + ': ' + templateAttrs[directiveName] + ' '));
compileNode = $compileNode[0];
replaceWith(jqCollection, jqLite(sliceArgs($template)), compileNode);

childTranscludeFn = compile($template, transcludeFn, terminalPriority,
replaceDirective && replaceDirective.name);
replaceDirective && replaceDirective.name, {
newIsolateScopeDirective: newIsolateScopeDirective,
transcludeDirective: transcludeDirective,
templateDirective: templateDirective
});
} else {
$template = jqLite(JQLiteClone(compileNode)).contents();
$compileNode.html(''); // clear contents
Expand Down Expand Up @@ -889,7 +901,11 @@ function $CompileProvider($provide) {
}

nodeLinkFn = compileTemplateUrl(directives.splice(i, directives.length - i), $compileNode,
templateAttrs, jqCollection, childTranscludeFn, preLinkFns, postLinkFns);
templateAttrs, jqCollection, childTranscludeFn, preLinkFns, postLinkFns, {
newIsolateScopeDirective: newIsolateScopeDirective,
transcludeDirective: transcludeDirective,
templateDirective: templateDirective
});
ii = directives.length;
} else if (directive.compile) {
try {
Expand Down Expand Up @@ -1188,7 +1204,7 @@ function $CompileProvider($provide) {


function compileTemplateUrl(directives, $compileNode, tAttrs,
$rootElement, childTranscludeFn, preLinkFns, postLinkFns) {
$rootElement, childTranscludeFn, preLinkFns, postLinkFns, previousCompileContext) {
var linkQueue = [],
afterTemplateNodeLinkFn,
afterTemplateChildLinkFn,
Expand Down Expand Up @@ -1231,7 +1247,7 @@ function $CompileProvider($provide) {
directives.unshift(derivedSyncDirective);

afterTemplateNodeLinkFn = applyDirectivesToNode(directives, compileNode, tAttrs,
childTranscludeFn, $compileNode, origAsyncDirective, preLinkFns, postLinkFns);
childTranscludeFn, $compileNode, origAsyncDirective, preLinkFns, postLinkFns, previousCompileContext);
forEach($rootElement, function(node, i) {
if (node == compileNode) {
$rootElement[i] = $compileNode[0];
Expand Down
84 changes: 72 additions & 12 deletions test/ng/compileSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1042,10 +1042,12 @@ describe('$compile', function() {
templateUrl: 'template.html'
}));
});
inject(function($compile){
inject(function($compile, $httpBackend){
$httpBackend.whenGET('template.html').respond('<p>template.html</p>');
expect(function() {
$compile('<div><div class="sync async"></div></div>');
}).toThrowMinErr('$compile', 'multidir', 'Multiple directives [sync, async] asking for template on: '+
$httpBackend.flush();
}).toThrowMinErr('$compile', 'multidir', 'Multiple directives [async, sync] asking for template on: '+
'<div class="sync async">');
});
});
Expand Down Expand Up @@ -1205,7 +1207,7 @@ describe('$compile', function() {
));


it('should work when directive is a repeater', inject(
it('should work when directive is in a repeater', inject(
function($compile, $httpBackend, $rootScope) {
$httpBackend.expect('GET', 'hello.html').
respond('<span>i=<span ng-transclude></span>;</span>');
Expand Down Expand Up @@ -1317,7 +1319,7 @@ describe('$compile', function() {
});


describe('template as function', function() {
describe('templateUrl as function', function() {

beforeEach(module(function() {
directive('myDirective', valueFn({
Expand Down Expand Up @@ -2745,23 +2747,81 @@ describe('$compile', function() {
});


it('should only allow one transclude per element', function() {
it('should only allow one content transclusion per element', function() {
module(function() {
directive('first', valueFn({
scope: {},
restrict: 'CA',
transclude: 'content'
transclude: true
}));
directive('second', valueFn({
restrict: 'CA',
transclude: 'content'
transclude: true
}));
});
inject(function($compile) {
expect(function() {
$compile('<div class="first second"></div>');
$compile('<div first="" second=""></div>');
}).toThrowMinErr('$compile', 'multidir', /Multiple directives \[first, second\] asking for transclusion on: <div .+/);
});
});


it('should only allow one element transclusion per element', function() {
module(function() {
directive('first', valueFn({
transclude: 'element'
}));
directive('second', valueFn({
transclude: 'element'
}));
});
inject(function($compile) {
expect(function() {
$compile('<div first second></div>');
}).toThrowMinErr('$compile', 'multidir', 'Multiple directives [first, second] asking for transclusion on: ' +
'<div class="first second ng-isolate-scope ng-scope">');
'<!-- first: -->');
});
});


it('should only allow one element transclusion per element when directives have different priorities', function() {
// we restart compilation in this case and we need to remember the duplicates during the second compile
// regression #3893
module(function() {
directive('first', valueFn({
transclude: 'element',
priority: 100
}));
directive('second', valueFn({
transclude: 'element'
}));
});
inject(function($compile) {
expect(function() {
$compile('<div first second></div>');
}).toThrowMinErr('$compile', 'multidir', /Multiple directives \[first, second\] asking for transclusion on: <div .+/);
});
});


it('should only allow one element transclusion per element when async replace directive is in the mix', function() {
module(function() {
directive('template', valueFn({
templateUrl: 'template.html',
replace: true
}));
directive('first', valueFn({
transclude: 'element',
priority: 100
}));
directive('second', valueFn({
transclude: 'element'
}));
});
inject(function($compile, $httpBackend) {
$httpBackend.expectGET('template.html').respond('<p second>template.html</p>');
$compile('<div template first></div>');
expect(function() {
$httpBackend.flush();
}).toThrowMinErr('$compile', 'multidir', /Multiple directives \[first, second\] asking for transclusion on: <p .+/);
});
});

Expand Down
37 changes: 37 additions & 0 deletions test/ng/directive/ngRepeatSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -976,6 +976,43 @@ describe('ngRepeat', function() {
});


describe('compatibility', function() {

it('should allow mixing ngRepeat and another element transclusion directive', function() {
$compileProvider.directive('elmTrans', valueFn({
transclude: 'element',
controller: function($transclude, $scope, $element) {
$transclude(function(transcludedNodes) {
$element.after(']]').after(transcludedNodes).after('[[');
});
}
}));

inject(function($compile, $rootScope) {
element = $compile('<div><div ng-repeat="i in [1,2]" elm-trans>{{i}}</div></div>')($rootScope);
$rootScope.$digest();
expect(element.text()).toBe('[[1]][[2]]')
});
});


it('should allow mixing ngRepeat with ngInclude', inject(function($compile, $rootScope, $httpBackend) {
$httpBackend.whenGET('someTemplate.html').respond('<p>some template; </p>');
element = $compile('<div><div ng-repeat="i in [1,2]" ng-include="\'someTemplate.html\'"></div></div>')($rootScope);
$rootScope.$digest();
$httpBackend.flush();
expect(element.text()).toBe('some template; some template; ');
}));


it('should allow mixing ngRepeat with ngIf', inject(function($compile, $rootScope) {
element = $compile('<div><div ng-repeat="i in [1,2,3,4]" ng-if="i % 2 == 0">{{i}};</div></div>')($rootScope);
$rootScope.$digest();
expect(element.text()).toBe('2;4;');
}));
});


describe('ngRepeatStart', function () {
it('should grow multi-node repeater', inject(function($compile, $rootScope) {
$rootScope.show = false;
Expand Down

0 comments on commit aceef56

Please sign in to comment.