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

Commit 5f70d61

Browse files
vojtajinavojtajina
committed
refactor(ngInclude): remove scope attribute
The purpose of allowing the scope to be specified was to enable the $route service to work together with ngInclude. However the functionality of creating scopes was in the recent past moved from the $route service to the ngView directive, so currently there is no valid use case for specifying the scope for ngInclude. In fact, allowing the scope to be defined can under certain circumstances lead to memory leaks. Breaks ngInclude does not have scope attribute anymore.
1 parent 06d0955 commit 5f70d61

2 files changed

Lines changed: 56 additions & 70 deletions

File tree

‎src/ng/directive/ngInclude.js‎

Lines changed: 31 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,6 @@
1515
*
1616
* @param {string} ng-include|src angular expression evaluating to URL. If the source is a string constant,
1717
* make sure you wrap it in quotes, e.g. `src="'myPartialTemplate.html'"`.
18-
* @param {Scope=} [scope=new_child_scope] optional expression which evaluates to an
19-
* instance of angular.module.ng.$rootScope.Scope to set the HTML fragment to.
2018
* @param {string=} onload Expression to evaluate when a new partial is loaded.
2119
*
2220
* @param {string=} autoscroll Whether `ng-include` should call {@link angular.module.ng.$anchorScroll
@@ -78,54 +76,48 @@ var ngIncludeDirective = ['$http', '$templateCache', '$anchorScroll', '$compile'
7876
return {
7977
restrict: 'EA',
8078
compile: function(element, attr) {
81-
var srcExp = attr.ngInclude || attr.src,
82-
scopeExp = attr.scope || '',
79+
var srcExp = attr.ngInclude || attr.src,
8380
onloadExp = attr.onload || '',
8481
autoScrollExp = attr.autoscroll;
8582

8683
return function(scope, element, attr) {
8784
var changeCounter = 0,
8885
childScope;
8986

90-
function incrementChange() { changeCounter++;}
91-
scope.$watch(srcExp, incrementChange);
92-
scope.$watch(function() {
93-
var includeScope = scope.$eval(scopeExp);
94-
if (includeScope) return includeScope.$id;
95-
}, incrementChange);
96-
scope.$watch(function() {return changeCounter;}, function(newChangeCounter) {
97-
var src = scope.$eval(srcExp),
98-
useScope = scope.$eval(scopeExp);
87+
var clearContent = function() {
88+
if (childScope) {
89+
childScope.$destroy();
90+
childScope = null;
91+
}
92+
93+
element.html('');
94+
};
95+
96+
scope.$watch(srcExp, function(src) {
97+
var thisChangeId = ++changeCounter;
98+
99+
if (src) {
100+
$http.get(src, {cache: $templateCache}).success(function(response) {
101+
if (thisChangeId !== changeCounter) return;
99102

100-
function clearContent() {
101-
// if this callback is still desired
102-
if (newChangeCounter === changeCounter) {
103103
if (childScope) childScope.$destroy();
104-
childScope = null;
105-
element.html('');
106-
}
107-
}
104+
childScope = scope.$new();
105+
106+
element.html(response);
107+
$compile(element.contents())(childScope);
108+
109+
if (isDefined(autoScrollExp) && (!autoScrollExp || scope.$eval(autoScrollExp))) {
110+
$anchorScroll();
111+
}
108112

109-
if (src) {
110-
$http.get(src, {cache: $templateCache}).success(function(response) {
111-
// if this callback is still desired
112-
if (newChangeCounter === changeCounter) {
113-
element.html(response);
114-
if (childScope) childScope.$destroy();
115-
childScope = useScope ? useScope : scope.$new();
116-
$compile(element.contents())(childScope);
117-
if (isDefined(autoScrollExp) && (!autoScrollExp || scope.$eval(autoScrollExp))) {
118-
$anchorScroll();
119-
}
120-
scope.$emit('$includeContentLoaded');
121-
scope.$eval(onloadExp);
122-
}
123-
}).error(clearContent);
124-
} else {
125-
clearContent();
126-
}
113+
scope.$emit('$includeContentLoaded');
114+
scope.$eval(onloadExp);
115+
}).error(function() {
116+
if (thisChangeId === changeCounter) clearContent();
117+
});
118+
} else clearContent();
127119
});
128120
};
129121
}
130-
}
122+
};
131123
}];

‎test/ng/directive/ngIncludeSpec.js‎

Lines changed: 25 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,10 @@ describe('ng-include', function() {
1818

1919
it('should include on external file', inject(putIntoCache('myUrl', '{{name}}'),
2020
function($rootScope, $compile) {
21-
element = jqLite('<ng:include src="url" scope="childScope"></ng:include>');
21+
element = jqLite('<ng:include src="url"></ng:include>');
2222
jqLite(document.body).append(element);
2323
element = $compile(element)($rootScope);
24-
$rootScope.childScope = $rootScope.$new();
25-
$rootScope.childScope.name = 'misko';
24+
$rootScope.name = 'misko';
2625
$rootScope.url = 'myUrl';
2726
$rootScope.$digest();
2827
expect(element.text()).toEqual('misko');
@@ -46,10 +45,9 @@ describe('ng-include', function() {
4645
it('should remove previously included text if a falsy value is bound to src', inject(
4746
putIntoCache('myUrl', '{{name}}'),
4847
function($rootScope, $compile) {
49-
element = jqLite('<ng:include src="url" scope="childScope"></ng:include>');
48+
element = jqLite('<ng:include src="url"></ng:include>');
5049
element = $compile(element)($rootScope);
51-
$rootScope.childScope = $rootScope.$new();
52-
$rootScope.childScope.name = 'igor';
50+
$rootScope.name = 'igor';
5351
$rootScope.url = 'myUrl';
5452
$rootScope.$digest();
5553

@@ -62,23 +60,6 @@ describe('ng-include', function() {
6260
}));
6361

6462

65-
it('should allow this for scope', inject(putIntoCache('myUrl', '{{"abc"}}'),
66-
function($rootScope, $compile) {
67-
element = jqLite('<ng:include src="url" scope="this"></ng:include>');
68-
element = $compile(element)($rootScope);
69-
$rootScope.url = 'myUrl';
70-
$rootScope.$digest();
71-
72-
// TODO(misko): because we are using scope==this, the eval gets registered
73-
// during the flush phase and hence does not get called.
74-
// I don't think passing 'this' makes sense. Does having scope on ng-include makes sense?
75-
// should we make scope="this" illegal?
76-
$rootScope.$digest();
77-
78-
expect(element.text()).toEqual('abc');
79-
}));
80-
81-
8263
it('should fire $includeContentLoaded event after linking the content', inject(
8364
function($rootScope, $compile, $templateCache) {
8465
var contentLoadedSpy = jasmine.createSpy('content loaded').andCallFake(function() {
@@ -111,20 +92,33 @@ describe('ng-include', function() {
11192
}));
11293

11394

114-
it('should destroy old scope', inject(putIntoCache('myUrl', 'my partial'),
115-
function($rootScope, $compile) {
116-
element = jqLite('<ng:include src="url"></ng:include>');
117-
element = $compile(element)($rootScope);
95+
it('should create child scope and destroy old one', inject(
96+
function($rootScope, $compile, $httpBackend) {
97+
$httpBackend.whenGET('url1').respond('partial {{$parent.url}}');
98+
$httpBackend.whenGET('url2').respond(404);
11899

119-
expect($rootScope.$$childHead).toBeFalsy();
100+
element = $compile('<ng:include src="url"></ng:include>')($rootScope);
101+
expect(element.children().scope()).toBeFalsy();
120102

121-
$rootScope.url = 'myUrl';
103+
$rootScope.url = 'url1';
104+
$rootScope.$digest();
105+
$httpBackend.flush();
106+
expect(element.children().scope()).toBeTruthy();
107+
expect(element.text()).toBe('partial url1');
108+
109+
$rootScope.url = 'url2';
110+
$rootScope.$digest();
111+
$httpBackend.flush();
112+
expect(element.children().scope()).toBeFalsy();
113+
expect(element.text()).toBe('');
114+
115+
$rootScope.url = 'url1';
122116
$rootScope.$digest();
123-
expect($rootScope.$$childHead).toBeTruthy();
117+
expect(element.children().scope()).toBeTruthy();
124118

125119
$rootScope.url = null;
126120
$rootScope.$digest();
127-
expect($rootScope.$$childHead).toBeFalsy();
121+
expect(element.children().scope()).toBeFalsy();
128122
}));
129123

130124

0 commit comments

Comments
 (0)
⚡