From 50395d2dda25712afb0e379d7c1d9794c1a341c4 Mon Sep 17 00:00:00 2001 From: Vojta Jina Date: Sun, 8 Sep 2013 09:24:15 -0700 Subject: [PATCH 1/2] style($compile): rename replaceDirective variable, add jsdocs This variable is only used for telling `compile` to ignore a particular directive. It is a template directive (we currently pass even non-replacing template directives, but that's imho wrong). I believe this is easier to understand. Also add missing jsdocs comment for the ignoreDirective argument. --- src/ng/compile.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 7a2ee4e7fcf8..803bc7a530f4 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -734,9 +734,11 @@ 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 + * the transclusion. * @returns linkFn */ - function applyDirectivesToNode(directives, compileNode, templateAttrs, transcludeFn, jqCollection, originalReplaceDirective) { + function applyDirectivesToNode(directives, compileNode, templateAttrs, transcludeFn, jqCollection, ignoreDirective) { var terminalPriority = -Number.MAX_VALUE, preLinkFns = [], postLinkFns = [], @@ -748,7 +750,6 @@ function $CompileProvider($provide) { directiveName, $template, transcludeDirective, - replaceDirective = originalReplaceDirective, childTranscludeFn = transcludeFn, controllerDirectives, linkFn, @@ -801,7 +802,7 @@ function $CompileProvider($provide) { replaceWith(jqCollection, jqLite(sliceArgs($template)), compileNode); childTranscludeFn = compile($template, transcludeFn, terminalPriority, - replaceDirective && replaceDirective.name); + ignoreDirective && ignoreDirective.name); } else { $template = jqLite(JQLiteClone(compileNode)).contents(); $compileNode.html(''); // clear contents @@ -820,7 +821,7 @@ function $CompileProvider($provide) { directiveValue = denormalizeTemplate(directiveValue); if (directive.replace) { - replaceDirective = directive; + ignoreDirective = directive; $template = jqLite('
' + trim(directiveValue) + '
').contents(); @@ -859,7 +860,7 @@ function $CompileProvider($provide) { templateDirective = directive; if (directive.replace) { - replaceDirective = directive; + ignoreDirective = directive; } nodeLinkFn = compileTemplateUrl(directives.splice(i, directives.length - i), nodeLinkFn, $compileNode, templateAttrs, jqCollection, childTranscludeFn); From b438298e96072dc722373f3f4a62ba76e309727b Mon Sep 17 00:00:00 2001 From: Vojta Jina Date: Sun, 8 Sep 2013 10:05:25 -0700 Subject: [PATCH 2/2] fix($compile): link parents before traversing How did compiling a templateUrl (async) directive with `replace:true` work before this commit? 1/ apply all directives with higher priority than the templateUrl directive 2/ partially apply the templateUrl directive (create `beforeTemplateNodeLinkFn`) 3/ fetch the template 4/ apply second part of the templateUrl directive on the fetched template (`afterTemplateNodeLinkFn`) That is, the templateUrl directive is basically split into two parts (two `nodeLinkFn` functions), which has to be both applied. Normally we compose linking functions (`nodeLinkFn`) using continuation - calling the linking function of a parent element, passing the linking function of the child elements as an argument. The parent linking function then does: 1/ execute its pre-link functions 2/ call the child elements linking function (traverse) 3/ execute its post-link functions Now, we have two linking functions for the same DOM element level (because the templateUrl directive has been split). There has been multiple issues because of the order of these two linking functions (creating controller before setting up scope locals, running linking functions before instantiating controller, etc.). It is easy to fix one use case, but it breaks some other use case. It is hard to decide what is the "correct" order of these two linking functions as they are essentially on the same level. Running them side-by-side screws up pre/post linking functions for the high priority directives (those executed before the templateUrl directive). It runs post-linking functions before traversing: ```js beforeTemplateNodeLinkFn(null); // do not travers afterTemplateNodeLinkFn(afterTemplateChildLinkFn); ``` Composing them (in any order) screws up the order of post-linking functions. We could fix this by having post-linking functions to execute in reverse order (from the lowest priority to the highest) which might actually make a sense. **My solution is to remove this splitting.** This commit removes the `beforeTemplateNodeLinkFn`. The first run (before we have the template) only schedules fetching the template. The rest (creating scope locals, instantiating a controller, linking functions, etc) is done when processing the directive again (in the context of the already fetched template; this is the cloned `derivedSyncDirective`). We still need to pass-through the linking functions of the higher priority directives (those executed before the templateUrl directive), that's why I added `preLinkFns` and `postLinkFns` arguments to `applyDirectivesToNode`. This also changes the "$compile transclude should make the result of a transclusion available to the parent directive in post- linking phase (templateUrl)" unit test. It was testing that a parent directive can see the content of transclusion in its pre-link function. That is IMHO wrong (as the `ngTransclude` directive inserts the translusion in its linking function). This test was only passing because of c173ca412878d537b18df01f39e400ea48a4b398, which changed the behavior of the compiler to traverse before executing the parent linking function. That was wrong and also caused the #3792 issue, which this change fixes. Closes #3792 --- src/ng/compile.js | 48 +++++++++++++++++++++--------------------- test/ng/compileSpec.js | 42 ++++++++++++++++++++++++++++-------- 2 files changed, 57 insertions(+), 33 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 803bc7a530f4..44bd31983da6 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -526,7 +526,7 @@ function $CompileProvider($provide) { directives = collectDirectives(nodeList[i], [], attrs, i == 0 ? maxPriority : undefined, ignoreDirective); nodeLinkFn = (directives.length) - ? applyDirectivesToNode(directives, nodeList[i], attrs, transcludeFn, $rootElement) + ? applyDirectivesToNode(directives, nodeList[i], attrs, transcludeFn, $rootElement, null, [], []) : null; childLinkFn = (nodeLinkFn && nodeLinkFn.terminal || !nodeList[i].childNodes || !nodeList[i].childNodes.length) @@ -738,10 +738,9 @@ function $CompileProvider($provide) { * the transclusion. * @returns linkFn */ - function applyDirectivesToNode(directives, compileNode, templateAttrs, transcludeFn, jqCollection, ignoreDirective) { + function applyDirectivesToNode(directives, compileNode, templateAttrs, transcludeFn, + jqCollection, ignoreDirective, preLinkFns, postLinkFns) { var terminalPriority = -Number.MAX_VALUE, - preLinkFns = [], - postLinkFns = [], newScopeDirective = null, newIsolateScopeDirective = null, templateDirective = null, @@ -772,18 +771,22 @@ function $CompileProvider($provide) { } if (directiveValue = directive.scope) { - assertNoDuplicate('isolated scope', newIsolateScopeDirective, directive, $compileNode); - if (isObject(directiveValue)) { - safeAddClass($compileNode, 'ng-isolate-scope'); - newIsolateScopeDirective = directive; - } - safeAddClass($compileNode, 'ng-scope'); newScopeDirective = newScopeDirective || directive; + + if (!directive.templateUrl) { + assertNoDuplicate('isolated scope', newIsolateScopeDirective, directive, $compileNode); + if (isObject(directiveValue)) { + safeAddClass($compileNode, 'ng-isolate-scope'); + newIsolateScopeDirective = directive; + } + safeAddClass($compileNode, 'ng-scope'); + } } directiveName = directive.name; - if (directiveValue = directive.controller) { + if (!directive.templateUrl && directive.controller) { + directiveValue = directive.controller; controllerDirectives = controllerDirectives || {}; assertNoDuplicate("'" + directiveName + "' controller", controllerDirectives[directiveName], directive, $compileNode); @@ -862,8 +865,9 @@ function $CompileProvider($provide) { if (directive.replace) { ignoreDirective = directive; } - nodeLinkFn = compileTemplateUrl(directives.splice(i, directives.length - i), - nodeLinkFn, $compileNode, templateAttrs, jqCollection, childTranscludeFn); + + nodeLinkFn = compileTemplateUrl(directives.splice(i, directives.length - i), $compileNode, + templateAttrs, jqCollection, childTranscludeFn, preLinkFns, postLinkFns); ii = directives.length; } else if (directive.compile) { try { @@ -1144,8 +1148,8 @@ function $CompileProvider($provide) { } - function compileTemplateUrl(directives, beforeTemplateNodeLinkFn, $compileNode, tAttrs, - $rootElement, childTranscludeFn) { + function compileTemplateUrl(directives, $compileNode, tAttrs, + $rootElement, childTranscludeFn, preLinkFns, postLinkFns) { var linkQueue = [], afterTemplateNodeLinkFn, afterTemplateChildLinkFn, @@ -1153,7 +1157,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, scope: null, replace: null + templateUrl: null, transclude: null, replace: null }), templateUrl = (isFunction(origAsyncDirective.templateUrl)) ? origAsyncDirective.templateUrl($compileNode, tAttrs) @@ -1187,7 +1191,8 @@ function $CompileProvider($provide) { directives.unshift(derivedSyncDirective); - afterTemplateNodeLinkFn = applyDirectivesToNode(directives, compileNode, tAttrs, childTranscludeFn, $compileNode, origAsyncDirective); + afterTemplateNodeLinkFn = applyDirectivesToNode(directives, compileNode, tAttrs, + childTranscludeFn, $compileNode, origAsyncDirective, preLinkFns, postLinkFns); forEach($rootElement, function(node, i) { if (node == compileNode) { $rootElement[i] = $compileNode[0]; @@ -1209,10 +1214,7 @@ function $CompileProvider($provide) { replaceWith(linkRootElement, jqLite(beforeTemplateLinkNode), linkNode); } - afterTemplateNodeLinkFn( - beforeTemplateNodeLinkFn(afterTemplateChildLinkFn, scope, linkNode, $rootElement, controller), - scope, linkNode, $rootElement, controller - ); + afterTemplateNodeLinkFn(afterTemplateChildLinkFn, scope, linkNode, $rootElement, controller); } linkQueue = null; }). @@ -1227,9 +1229,7 @@ function $CompileProvider($provide) { linkQueue.push(rootElement); linkQueue.push(controller); } else { - afterTemplateNodeLinkFn(function() { - beforeTemplateNodeLinkFn(afterTemplateChildLinkFn, scope, node, rootElement, controller); - }, scope, node, rootElement, controller); + afterTemplateNodeLinkFn(afterTemplateChildLinkFn, scope, node, rootElement, controller); } }; } diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index d26438783d70..43bcd2433825 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -1264,6 +1264,35 @@ describe('$compile', function() { expect(element.text()).toBe('boom!1|boom!2|'); }); }); + + + it('should support templateUrl with replace', function() { + // a regression https://github.com/angular/angular.js/issues/3792 + module(function($compileProvider) { + $compileProvider.directive('simple', function() { + return { + templateUrl: '/some.html', + replace: true + }; + }); + }); + + inject(function($templateCache, $rootScope, $compile) { + $templateCache.put('/some.html', + '
' + + '
i = 1
' + + '
I dont know what `i` is.
' + + '
'); + + element = $compile('
')($rootScope); + + $rootScope.$apply(function() { + $rootScope.i = 1; + }); + + expect(element.html()).toContain('i = 1'); + }); + }); }); @@ -2824,7 +2853,7 @@ describe('$compile', function() { }); - it('should make the result of a transclusion available to the parent directive in pre- and post- linking phase (templateUrl)', + it('should make the result of a transclusion available to the parent directive in post- linking phase (templateUrl)', function() { // when compiling an async directive the transclusion is always processed before the directive // this is different compared to sync directive. delaying the transclusion makes little sense. @@ -2834,13 +2863,8 @@ describe('$compile', function() { return { transclude: true, templateUrl: 'trans.html', - link: { - pre: function($scope, $element) { - log('pre(' + $element.text() + ')'); - }, - post: function($scope, $element) { - log('post(' + $element.text() + ')'); - } + link: function($scope, $element) { + log('post(' + $element.text() + ')'); } }; }); @@ -2850,7 +2874,7 @@ describe('$compile', function() { element = $compile('
unicorn!
')($rootScope); $rootScope.$apply(); - expect(log).toEqual('pre(unicorn!); post(unicorn!)'); + expect(log).toEqual('post(unicorn!)'); }); }); });