Skip to content

Commit

Permalink
fix($compile): link parents before traversing
Browse files Browse the repository at this point in the history 
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
c173ca4, which changed the behavior of the compiler to traverse
before executing the parent linking function. That was wrong and also caused the angular#3792 issue, which
this change fixes.

Closes angular#3792
Closes angular#3923
Closes angular#3935
Closes angular#3927
  • Loading branch information
@vojtajina @jamesdaily
vojtajina authored and jamesdaily committed Jan 27, 2014
1 parent 4d0859a commit cae20c1
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 32 deletions.
54 changes: 30 additions & 24 deletions src/ng/compile.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -536,7 +536,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)
Expand Down Expand Up @@ -747,12 +747,15 @@ 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.
* @param {Array.<Function>} preLinkFns
* @param {Array.<Function>} postLinkFns
* @returns linkFn
*/
function applyDirectivesToNode(directives, compileNode, templateAttrs, transcludeFn, jqCollection, originalReplaceDirective) {
function applyDirectivesToNode(directives, compileNode, templateAttrs, transcludeFn, jqCollection,
originalReplaceDirective, preLinkFns, postLinkFns) {
var terminalPriority = -Number.MAX_VALUE,
preLinkFns = [],
postLinkFns = [],
newScopeDirective = null,
newIsolateScopeDirective = null,
templateDirective = null,
Expand Down Expand Up @@ -784,18 +787,24 @@ 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;

// skip the check for directives with async templates, we'll check the derived sync directive when
// the template arrives
if (!directive.templateUrl) {
assertNoDuplicate('new/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);
Expand Down Expand Up @@ -874,8 +883,9 @@ function $CompileProvider($provide) {
if (directive.replace) {
replaceDirective = 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 {
Expand Down Expand Up @@ -1170,16 +1180,16 @@ function $CompileProvider($provide) {
}


function compileTemplateUrl(directives, beforeTemplateNodeLinkFn, $compileNode, tAttrs,
$rootElement, childTranscludeFn) {
function compileTemplateUrl(directives, $compileNode, tAttrs,
$rootElement, childTranscludeFn, preLinkFns, postLinkFns) {
var linkQueue = [],
afterTemplateNodeLinkFn,
afterTemplateChildLinkFn,
beforeTemplateCompileNode = $compileNode[0],
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)
Expand Down Expand Up @@ -1213,7 +1223,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];
Expand All @@ -1235,10 +1246,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;
}).
Expand All @@ -1253,9 +1261,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);
}
};
}
Expand Down
52 changes: 44 additions & 8 deletions test/ng/compileSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1046,7 +1046,10 @@ describe('$compile', function() {
priority: priority,
compile: function() {
log(name + '-C');
return function() { log(name + '-L'); }
return {
pre: function() { log(name + '-PreL'); },
post: function() { log(name + '-PostL'); }
}
}
}, options || {}));
});
Expand Down Expand Up @@ -1075,7 +1078,8 @@ describe('$compile', function() {
$rootScope.$digest();
expect(log).toEqual(
'first-C; FLUSH; second-C; last-C; third-C; ' +
'third-L; first-L; second-L; last-L');
'first-PreL; second-PreL; last-PreL; third-PreL; ' +
'third-PostL; first-PostL; second-PostL; last-PostL');

var span = element.find('span');
expect(span.attr('first')).toEqual('');
Expand All @@ -1099,7 +1103,8 @@ describe('$compile', function() {
$rootScope.$digest();
expect(log).toEqual(
'iFirst-C; FLUSH; iSecond-C; iThird-C; iLast-C; ' +
'iFirst-L; iSecond-L; iThird-L; iLast-L');
'iFirst-PreL; iSecond-PreL; iThird-PreL; iLast-PreL; ' +
'iFirst-PostL; iSecond-PostL; iThird-PostL; iLast-PostL');

var div = element.find('div');
expect(div.attr('i-first')).toEqual('');
Expand All @@ -1124,7 +1129,8 @@ describe('$compile', function() {
$rootScope.$digest();
expect(log).toEqual(
'first-C; FLUSH; second-C; last-C; third-C; ' +
'third-L; first-L; second-L; last-L');
'first-PreL; second-PreL; last-PreL; third-PreL; ' +
'third-PostL; first-PostL; second-PostL; last-PostL');

var span = element.find('span');
expect(span.attr('first')).toEqual('');
Expand All @@ -1149,7 +1155,8 @@ describe('$compile', function() {
$rootScope.$digest();
expect(log).toEqual(
'iFirst-C; FLUSH; iSecond-C; iThird-C; iLast-C; ' +
'iFirst-L; iSecond-L; iThird-L; iLast-L');
'iFirst-PreL; iSecond-PreL; iThird-PreL; iLast-PreL; ' +
'iFirst-PostL; iSecond-PostL; iThird-PostL; iLast-PostL');

var div = element.find('div');
expect(div.attr('i-first')).toEqual('');
Expand Down Expand Up @@ -1264,6 +1271,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',
'<div ng-switch="i">' +
'<div ng-switch-when="1">i = 1</div>' +
'<div ng-switch-default>I dont know what `i` is.</div>' +
'</div>');

element = $compile('<div simple></div>')($rootScope);

$rootScope.$apply(function() {
$rootScope.i = 1;
});

expect(element.html()).toContain('i = 1');
});
});
});


Expand Down Expand Up @@ -1482,7 +1518,7 @@ describe('$compile', function() {
function($rootScope, $compile) {
expect(function(){
$compile('<div class="iscope-a; scope-b"></div>');
}).toThrowMinErr('$compile', 'multidir', 'Multiple directives [iscopeA, scopeB] asking for isolated scope on: ' +
}).toThrowMinErr('$compile', 'multidir', 'Multiple directives [iscopeA, scopeB] asking for new/isolated scope on: ' +
'<div class="iscope-a; scope-b ng-isolate-scope ng-scope">');
})
);
Expand Down Expand Up @@ -2824,7 +2860,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.
Expand All @@ -2850,7 +2886,7 @@ describe('$compile', function() {

element = $compile('<div trans><span>unicorn!</span></div>')($rootScope);
$rootScope.$apply();
expect(log).toEqual('pre(unicorn!); post(unicorn!)');
expect(log).toEqual('pre(); post(unicorn!)');
});
});
});
Expand Down

0 comments on commit cae20c1

Please sign in to comment.