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

Commit

Permalink
perf($compile): Lazily compile the transclude function
Browse files Browse the repository at this point in the history
For transcluded directives, the transclude function can be lazily compiled
most of the time since the contents will not be needed until the
`transclude` function was actually invoked.  For example, the `transclude`
function that is passed to `ng-if` or `ng-switch-when` does not need to be
invoked until the condition that it's bound to has been matched.  For
complex trees or switch statements, this can represent significant
performance gains since compilation of branches is deferred, and that
compilation may never actually happen if it isn't needed.

There are two instances where compilation will not be lazy; when we scan
ahead in the array of directives to be processed and find at least two of
the following:

* A directive that is transcluded and does not allow multiple transclusion
* A directive that has templateUrl and replace: true
* A directive that has a template and replace: true

In both of those cases, we will need to continue eager compilation in
order to generate the multiple transclusion exception at the correct time.
  • Loading branch information
dcherman authored and lgalfaso committed Sep 19, 2015
1 parent 20cf7d5 commit 652b83e
Show file tree
Hide file tree
Showing 2 changed files with 263 additions and 2 deletions.
58 changes: 56 additions & 2 deletions src/ng/compile.js
Original file line number Diff line number Diff line change
Expand Up @@ -1646,6 +1646,37 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
};
}

/**
* A function generator that is used to support both eager and lazy compilation
* linking function.
* @param eager
* @param $compileNodes
* @param transcludeFn
* @param maxPriority
* @param ignoreDirective
* @param previousCompileContext
* @returns {Function}
*/
function compilationGenerator(eager, $compileNodes, transcludeFn, maxPriority, ignoreDirective, previousCompileContext) {
if (eager) {
return compile($compileNodes, transcludeFn, maxPriority, ignoreDirective, previousCompileContext);
}

var compiled;

return function() {
if (!compiled) {
compiled = compile($compileNodes, transcludeFn, maxPriority, ignoreDirective, previousCompileContext);

// Null out all of these references in order to make them eligible for garbage collection
// since this is a potentially long lived closure
$compileNodes = transcludeFn = previousCompileContext = null;
}

return compiled.apply(this, arguments);
};
}

/**
* Once the directives have been collected, their compile functions are executed. This method
* is responsible for inlining directive templates as well as terminating the application
Expand Down Expand Up @@ -1690,6 +1721,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
replaceDirective = originalReplaceDirective,
childTranscludeFn = transcludeFn,
linkFn,
didScanForMultipleTransclusion = false,
mightHaveMultipleTransclusionError = false,
directiveValue;

// executes all directives on the current element
Expand Down Expand Up @@ -1732,6 +1765,27 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

directiveName = directive.name;

// If we encounter a condition that can result in transclusion on the directive,
// then scan ahead in the remaining directives for others that may cause a multiple
// transclusion error to be thrown during the compilation process. If a matching directive
// is found, then we know that when we encounter a transcluded directive, we need to eagerly
// compile the `transclude` function rather than doing it lazily in order to throw
// exceptions at the correct time
if (!didScanForMultipleTransclusion && ((directive.replace && (directive.templateUrl || directive.template))
|| (directive.transclude && !directive.$$tlb))) {
var candidateDirective;

for (var scanningIndex = i + 1; candidateDirective = directives[scanningIndex++];) {
if ((candidateDirective.transclude && !candidateDirective.$$tlb)
|| (candidateDirective.replace && (candidateDirective.templateUrl || candidateDirective.template))) {
mightHaveMultipleTransclusionError = true;
break;
}
}

didScanForMultipleTransclusion = true;
}

if (!directive.templateUrl && directive.controller) {
directiveValue = directive.controller;
controllerDirectives = controllerDirectives || createMap();
Expand Down Expand Up @@ -1761,7 +1815,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
compileNode = $compileNode[0];
replaceWith(jqCollection, sliceArgs($template), compileNode);

childTranscludeFn = compile($template, transcludeFn, terminalPriority,
childTranscludeFn = compilationGenerator(mightHaveMultipleTransclusionError, $template, transcludeFn, terminalPriority,
replaceDirective && replaceDirective.name, {
// Don't pass in:
// - controllerDirectives - otherwise we'll create duplicates controllers
Expand All @@ -1775,7 +1829,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
} else {
$template = jqLite(jqLiteClone(compileNode)).contents();
$compileNode.empty(); // clear contents
childTranscludeFn = compile($template, transcludeFn);
childTranscludeFn = compilationGenerator(mightHaveMultipleTransclusionError, $template, transcludeFn);
}
}

Expand Down
207 changes: 207 additions & 0 deletions test/ng/compileSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -6756,6 +6756,27 @@ describe('$compile', function() {
});
});

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


it('should support transcluded element on root content', function() {
var comment;
Expand Down Expand Up @@ -7052,6 +7073,192 @@ describe('$compile', function() {
});

});

it('should lazily compile the contents of directives that are transcluded', function() {
var innerCompilationCount = 0, transclude;

module(function() {
directive('trans', valueFn({
transclude: true,
controller: function($transclude) {
transclude = $transclude;
}
}));

directive('inner', valueFn({
template: '<span>FooBar</span>',
compile: function() {
innerCompilationCount +=1;
}
}));
});

inject(function($compile, $rootScope) {
element = $compile('<trans><inner></inner></trans>')($rootScope);
expect(innerCompilationCount).toBe(0);
transclude(function(child) { element.append(child); });
expect(innerCompilationCount).toBe(1);
expect(element.text()).toBe('FooBar');
});
});

it('should lazily compile the contents of directives that are transcluded with a template', function() {
var innerCompilationCount = 0, transclude;

module(function() {
directive('trans', valueFn({
transclude: true,
template: '<div>Baz</div>',
controller: function($transclude) {
transclude = $transclude;
}
}));

directive('inner', valueFn({
template: '<span>FooBar</span>',
compile: function() {
innerCompilationCount +=1;
}
}));
});

inject(function($compile, $rootScope) {
element = $compile('<trans><inner></inner></trans>')($rootScope);
expect(innerCompilationCount).toBe(0);
transclude(function(child) { element.append(child); });
expect(innerCompilationCount).toBe(1);
expect(element.text()).toBe('BazFooBar');
});
});

it('should lazily compile the contents of directives that are transcluded with a templateUrl', function() {
var innerCompilationCount = 0, transclude;

module(function() {
directive('trans', valueFn({
transclude: true,
templateUrl: 'baz.html',
controller: function($transclude) {
transclude = $transclude;
}
}));

directive('inner', valueFn({
template: '<span>FooBar</span>',
compile: function() {
innerCompilationCount +=1;
}
}));
});

inject(function($compile, $rootScope, $httpBackend) {
$httpBackend.expectGET('baz.html').respond('<div>Baz</div>');
element = $compile('<trans><inner></inner></trans>')($rootScope);
$httpBackend.flush();

expect(innerCompilationCount).toBe(0);
transclude(function(child) { element.append(child); });
expect(innerCompilationCount).toBe(1);
expect(element.text()).toBe('BazFooBar');
});
});

it('should lazily compile the contents of directives that are transclude element', function() {
var innerCompilationCount = 0, transclude;

module(function() {
directive('trans', valueFn({
transclude: 'element',
controller: function($transclude) {
transclude = $transclude;
}
}));

directive('inner', valueFn({
template: '<span>FooBar</span>',
compile: function() {
innerCompilationCount +=1;
}
}));
});

inject(function($compile, $rootScope) {
element = $compile('<div><trans><inner></inner></trans></div>')($rootScope);
expect(innerCompilationCount).toBe(0);
transclude(function(child) { element.append(child); });
expect(innerCompilationCount).toBe(1);
expect(element.text()).toBe('FooBar');
});
});

it('should lazily compile transcluded directives with ngIf on them', function() {
var innerCompilationCount = 0, outerCompilationCount = 0, transclude;

module(function() {
directive('outer', valueFn({
transclude: true,
compile: function() {
outerCompilationCount += 1;
},
controller: function($transclude) {
transclude = $transclude;
}
}));

directive('inner', valueFn({
template: '<span>FooBar</span>',
compile: function() {
innerCompilationCount +=1;
}
}));
});

inject(function($compile, $rootScope) {
$rootScope.shouldCompile = false;

element = $compile('<div><outer ng-if="shouldCompile"><inner></inner></outer></div>')($rootScope);
expect(outerCompilationCount).toBe(0);
expect(innerCompilationCount).toBe(0);
expect(transclude).toBeUndefined();
$rootScope.$apply('shouldCompile=true');
expect(outerCompilationCount).toBe(1);
expect(innerCompilationCount).toBe(0);
expect(transclude).toBeDefined();
transclude(function(child) { element.append(child); });
expect(outerCompilationCount).toBe(1);
expect(innerCompilationCount).toBe(1);
expect(element.text()).toBe('FooBar');
});
});

it('should eagerly compile multiple directives with transclusion and templateUrl/replace', function() {
var innerCompilationCount = 0;

module(function() {
directive('outer', valueFn({
transclude: true
}));

directive('outer', valueFn({
templateUrl: 'inner.html',
replace: true
}));

directive('inner', valueFn({
compile: function() {
innerCompilationCount +=1;
}
}));
});

inject(function($compile, $rootScope, $httpBackend) {
$httpBackend.expectGET('inner.html').respond('<inner></inner>');
element = $compile('<outer></outer>')($rootScope);
$httpBackend.flush();

expect(innerCompilationCount).toBe(1);
});
});
});


Expand Down

8 comments on commit 652b83e

@gunta
Copy link

@gunta gunta commented on 652b83e Oct 13, 2015

Choose a reason for hiding this comment

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

This commit seems to break a transcluded directive on my own code.

@lgalfaso
Copy link
Contributor

Choose a reason for hiding this comment

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

@gunta can you please create a plunker that shows the error

@sb8244
Copy link

@sb8244 sb8244 commented on 652b83e Feb 10, 2016

Choose a reason for hiding this comment

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

Is it a potential benefit to be able to opt out of the eager load? For instance, if I knew that a directive was going to break because of it (looking at you ui-select), being able to say eagerLoad: true in the directive.

This gives the flexibility to opt-out, but still has it on by default. It's fairly trivial given it just needs to pass mightHaveMultipleTransclusionError || directive.eagerLoad

@sb8244
Copy link

@sb8244 sb8244 commented on 652b83e Feb 10, 2016

Choose a reason for hiding this comment

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

A subtle breaking change like this that isn't intended to be a breaking change might classify as a bug. Either that, or it should be properly identified as something that can cause bugs. Whether the core team thinks [that isn't how it should be used] does not impact how people are using it and expect it to work. Opting out gives a grace period before it is deprecated.

@lgalfaso
Copy link
Contributor

Choose a reason for hiding this comment

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

@sb8244 do you have another example that is not ui-select? If you have one, then it would be nice to get one. The reason I am asking for another example is that ui-select is broken as it made some assumptions that were false even before Angular released 1.0. Here http://plnkr.co/edit/sRIW4LJ2VpSBJZAnODvY?p=preview is the proof that ui-select breaks with Angular 1.4.

Whether the core team thinks [that isn't how it should be used] does not impact how people are using it and expect it to work.

I find that statement very unfair:

  • The behavior is documented https://docs.angularjs.org/api/ng/service/$compile#-templateurl-. Developers are free to read or not to read the documentation, but when there are discrepancies between what the assumed to be true and how things work, they are not free to ignore that the behavior is documented
  • This change happen in a new version release, not in a patch release. Some breaking changes are needed to make things faster and to fix some bugs that need some underlying changes
  • People are free to upgrade to the latest release or stay with an older version, the same goes for all the other libraries that developers are using

Now, the Angular core team is doing many underlying changes trying to improve the experience for all Angular apps while keeping the API and behavior stable. When things break, effort is made to fix it. When some behavior is not properly documented, we do so. The issue you are raising does no fall into any of these categories as it was possible to trigger this behavior before Angular 1.5 and the behavior is documented.

@sb8244
Copy link

@sb8244 sb8244 commented on 652b83e Feb 10, 2016

Choose a reason for hiding this comment

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

Apologies for the strong statement. I've seen this particular one be closed on issues and swept off as a ui-select issue so that resolution: not-core happens. I actually don't think there is an open issue to discuss it at the moment.

My goal was to provide an alternative (opt-out). It seems that isn't the direction to go unless other apps have a problem. Totally cool.

One thing I love about angular is the breaking change list, it makes it very easy to follow along and I get a checklist of things to go through when I upgrade. While this isn't a breaking change, I think it highlights the fact that the change ends up breaking things (that weren't directly supported). A potential breaking change for this might look like:

Lazy compilation of the tranclude function forces asynchronous behavior in template rendering that, while suggested prior to 1.5.0, was not enforced. If your directive requires that the child template is rendered before the parent, you now must handle this asynchronously. See ngMessages for a way of doing this.

Thanks for 1.5.0, it looks like a great release. I don't want to turn a commit comment into a huge thing, so take it with a grain of salt.

~ Steve

@michi88
Copy link

Choose a reason for hiding this comment

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

I think the opt-out option would be good here. See the trivial case of animations fail here: #14074

@mattslocum
Copy link

Choose a reason for hiding this comment

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

This is also causing problems with my usage of ui-grid

Please sign in to comment.