From 69bbb16f42525b904e4aa262be9b47d007d3f7a6 Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Thu, 25 Oct 2012 00:33:36 -0700 Subject: [PATCH 1/5] fix($compile): prevent double attr interpolation w/ templateUrl This fixes the issue that caused two attr interpolation observers to be registered for the same attribute as a result of isolate scope definition with attr (@) property for this attribute. Duplicate observers would then fight with each other updating the model. The issue occured only when this directive was used in a repeater because that's when we clone the template node which caused the two observers to point to two different sets of $attr instances. Closes #1166, #836 --- src/ng/compile.js | 2 +- test/ng/compileSpec.js | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 3fa4691ccbe4..ae926923069c 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -899,7 +899,7 @@ function $CompileProvider($provide) { origAsyncDirective = directives.shift(), // The fact that we have to copy and patch the directive seems wrong! derivedSyncDirective = extend({}, origAsyncDirective, { - controller: null, templateUrl: null, transclude: null + controller: null, templateUrl: null, transclude: null, scope: null }); $compileNode.html(''); diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index b5e4c4507504..75af01810e12 100644 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -1048,6 +1048,39 @@ describe('$compile', function() { expect($exceptionHandler.errors).toEqual([]); }); }); + + + it('should resume delayed compilation without duplicates when in a repeater', function() { + // this is a test for a regression + // scope creation, isolate watcher setup, controller instantiation, etc should happen + // only once even if we are dealing with delayed compilation of a node due to templateUrl + // and the template node is in a repeater + + var controllerSpy = jasmine.createSpy('controller'); + + module(function($compileProvider) { + $compileProvider.directive('delayed', valueFn({ + controller: controllerSpy, + templateUrl: 'delayed.html', + scope: { + title: '@' + } + })); + }); + + inject(function($templateCache, $compile, $rootScope) { + $rootScope.coolTitle = 'boom!'; + $templateCache.put('delayed.html', '
{{title}}
'); + element = $compile( + '
|
' + )($rootScope); + + $rootScope.$apply(); + + expect(controllerSpy.callCount).toBe(2); + expect(element.text()).toBe('boom!1|boom!2|'); + }); + }); }); From 9c74a652519ce2a2caa864fd3d936fff70bdacd7 Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Thu, 25 Oct 2012 00:47:47 -0700 Subject: [PATCH 2/5] refactor(): simplify nodeLinkFn --- src/ng/compile.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index ae926923069c..9b6121316e49 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -706,12 +706,12 @@ function $CompileProvider($provide) { } $element = attrs.$$element; - if (newScopeDirective && isObject(newScopeDirective.scope)) { + if (newIsolatedScopeDirective) { var LOCAL_REGEXP = /^\s*([@=&])\s*(\w*)\s*$/; var parentScope = scope.$parent || scope; - forEach(newScopeDirective.scope, function(definiton, scopeName) { + forEach(newIsolatedScopeDirective.scope, function(definiton, scopeName) { var match = definiton.match(LOCAL_REGEXP) || [], attrName = match[2]|| scopeName, mode = match[1], // @, =, or & @@ -734,7 +734,7 @@ function $CompileProvider($provide) { // reset the change, or we will throw this exception on every $digest lastValue = scope[scopeName] = parentGet(parentScope); throw Error(NON_ASSIGNABLE_MODEL_EXPRESSION + attrs[attrName] + - ' (directive: ' + newScopeDirective.name + ')'); + ' (directive: ' + newIsolatedScopeDirective.name + ')'); }; lastValue = scope[scopeName] = parentGet(parentScope); scope.$watch(function parentValueWatch() { @@ -765,7 +765,7 @@ function $CompileProvider($provide) { default: { throw Error('Invalid isolate scope definition for directive ' + - newScopeDirective.name + ': ' + definiton); + newIsolatedScopeDirective.name + ': ' + definiton); } } }); From f3d6fc8484f8afde3cc13d3bcd64a5977b68ede7 Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Thu, 25 Oct 2012 00:56:34 -0700 Subject: [PATCH 3/5] style($compile): better fn names for debugging --- src/ng/compile.js | 47 ++++++++++++++++++++++++----------------------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 9b6121316e49..1ee0fc6f2f46 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -310,26 +310,26 @@ function $CompileProvider($provide) { //================================ - function compile($compileNode, transcludeFn, maxPriority) { - if (!($compileNode instanceof jqLite)) { + function compile($compileNodes, transcludeFn, maxPriority) { + if (!($compileNodes instanceof jqLite)) { // jquery always rewraps, where as we need to preserve the original selector so that we can modify it. - $compileNode = jqLite($compileNode); + $compileNodes = jqLite($compileNodes); } // We can not compile top level text elements since text nodes can be merged and we will // not be able to attach scope data to them, so we will wrap them in - forEach($compileNode, function(node, index){ + forEach($compileNodes, function(node, index){ if (node.nodeType == 3 /* text node */) { - $compileNode[index] = jqLite(node).wrap('').parent()[0]; + $compileNodes[index] = jqLite(node).wrap('').parent()[0]; } }); - var compositeLinkFn = compileNodes($compileNode, transcludeFn, $compileNode, maxPriority); - return function(scope, cloneConnectFn){ + var compositeLinkFn = compileNodes($compileNodes, transcludeFn, $compileNodes, maxPriority); + return function publicLinkFn(scope, cloneConnectFn){ assertArg(scope, 'scope'); // important!!: we must call our jqLite.clone() since the jQuery one is trying to be smart // and sometimes changes the structure of the DOM. var $linkNode = cloneConnectFn - ? JQLitePrototype.clone.call($compileNode) // IMPORTANT!!! - : $compileNode; + ? JQLitePrototype.clone.call($compileNodes) // IMPORTANT!!! + : $compileNodes; $linkNode.data('$scope', scope); safeAddClass($linkNode, 'ng-scope'); if (cloneConnectFn) cloneConnectFn($linkNode, scope); @@ -432,13 +432,14 @@ function $CompileProvider($provide) { /** - * Looks for directives on the given node ands them to the directive collection which is sorted. + * Looks for directives on the given node and adds them to the directive collection which is + * sorted. * - * @param node node to search - * @param directives an array to which the directives are added to. This array is sorted before + * @param node Node to search. + * @param directives An array to which the directives are added to. This array is sorted before * the function returns. - * @param attrs the shared attrs object which is used to populate the normalized attributes. - * @param {number=} max directive priority + * @param attrs The shared attrs object which is used to populate the normalized attributes. + * @param {number=} maxPriority Max directive priority. */ function collectDirectives(node, directives, attrs, maxPriority) { var nodeType = node.nodeType, @@ -527,7 +528,7 @@ function $CompileProvider($provide) { preLinkFns = [], postLinkFns = [], newScopeDirective = null, - newIsolatedScopeDirective = null, + newIsolateScopeDirective = null, templateDirective = null, $compileNode = templateAttrs.$$element = jqLite(compileNode), directive, @@ -549,10 +550,10 @@ function $CompileProvider($provide) { } if (directiveValue = directive.scope) { - assertNoDuplicate('isolated scope', newIsolatedScopeDirective, directive, $compileNode); + assertNoDuplicate('isolated scope', newIsolateScopeDirective, directive, $compileNode); if (isObject(directiveValue)) { safeAddClass($compileNode, 'ng-isolate-scope'); - newIsolatedScopeDirective = directive; + newIsolateScopeDirective = directive; } safeAddClass($compileNode, 'ng-scope'); newScopeDirective = newScopeDirective || directive; @@ -706,12 +707,12 @@ function $CompileProvider($provide) { } $element = attrs.$$element; - if (newIsolatedScopeDirective) { + if (newIsolateScopeDirective) { var LOCAL_REGEXP = /^\s*([@=&])\s*(\w*)\s*$/; var parentScope = scope.$parent || scope; - forEach(newIsolatedScopeDirective.scope, function(definiton, scopeName) { + forEach(newIsolateScopeDirective.scope, function(definiton, scopeName) { var match = definiton.match(LOCAL_REGEXP) || [], attrName = match[2]|| scopeName, mode = match[1], // @, =, or & @@ -734,7 +735,7 @@ function $CompileProvider($provide) { // reset the change, or we will throw this exception on every $digest lastValue = scope[scopeName] = parentGet(parentScope); throw Error(NON_ASSIGNABLE_MODEL_EXPRESSION + attrs[attrName] + - ' (directive: ' + newIsolatedScopeDirective.name + ')'); + ' (directive: ' + newIsolateScopeDirective.name + ')'); }; lastValue = scope[scopeName] = parentGet(parentScope); scope.$watch(function parentValueWatch() { @@ -765,7 +766,7 @@ function $CompileProvider($provide) { default: { throw Error('Invalid isolate scope definition for directive ' + - newIsolatedScopeDirective.name + ': ' + definiton); + newIsolateScopeDirective.name + ': ' + definiton); } } }); @@ -991,7 +992,7 @@ function $CompileProvider($provide) { if (interpolateFn) { directives.push({ priority: 0, - compile: valueFn(function(scope, node) { + compile: valueFn(function textInterpolateLinkFn(scope, node) { var parent = node.parent(), bindings = parent.data('$binding') || []; bindings.push(interpolateFn); @@ -1014,7 +1015,7 @@ function $CompileProvider($provide) { directives.push({ priority: 100, - compile: valueFn(function(scope, element, attr) { + compile: valueFn(function attrInterpolateLinkFn(scope, element, attr) { var $$observers = (attr.$$observers || (attr.$$observers = {})); if (name === 'class') { From 13d36eab13359165ca119083c32848958efed933 Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Thu, 25 Oct 2012 00:58:50 -0700 Subject: [PATCH 4/5] fix($compile): compilation should not resurse below empty nodes if a node doesn't have children then don't try to compile these non-existent children --- src/ng/compile.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 1ee0fc6f2f46..fd4318495abe 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -380,7 +380,7 @@ function $CompileProvider($provide) { ? applyDirectivesToNode(directives, nodeList[i], attrs, transcludeFn, $rootElement) : null; - childLinkFn = (nodeLinkFn && nodeLinkFn.terminal) + childLinkFn = (nodeLinkFn && nodeLinkFn.terminal || !nodeList[i].childNodes.length) ? null : compileNodes(nodeList[i].childNodes, nodeLinkFn ? nodeLinkFn.transclude : transcludeFn); From 336c70c489113140fed7d38d9afa607ab5c643ae Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Thu, 25 Oct 2012 01:00:21 -0700 Subject: [PATCH 5/5] fix($compile): don't look for class directives in empty string if className is undefined or empty string, don't bother looking for directives in there --- src/ng/compile.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index fd4318495abe..9ed7f1079a31 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -474,7 +474,7 @@ function $CompileProvider($provide) { // use class as directive className = node.className; - if (isString(className)) { + if (isString(className) && className !== '') { while (match = CLASS_DIRECTIVE_REGEXP.exec(className)) { nName = directiveNormalize(match[2]); if (addDirective(directives, nName, 'C', maxPriority)) {