From 1d14760c6d3eefb676f5670bc323b2a7cadcdbfa Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Wed, 30 Nov 2011 14:47:28 -0500 Subject: [PATCH] fix(ng:include): prevent race conditions by ignoring stale http callbacks This fix is similar to what I've done in ng:view, if a new template has been requested before the callback for the previous template returned, ignore it. Otherwise weird race conditions happen and users might end up getting the content for the previous include rendered instead of the most recent one. --- src/widgets.js | 49 ++++++++++++++++++++++----------------------- test/widgetsSpec.js | 36 +++++++++++++++++++++++++++------ 2 files changed, 54 insertions(+), 31 deletions(-) diff --git a/src/widgets.js b/src/widgets.js index 618de04dc3e9..155b8c08c4d9 100644 --- a/src/widgets.js +++ b/src/widgets.js @@ -94,42 +94,38 @@ angularWidget('ng:include', function(element){ function($http, $templateCache, $autoScroll, element) { var scope = this, changeCounter = 0, - releaseScopes = [], - childScope, - oldScope; + childScope; function incrementChange() { changeCounter++;} this.$watch(srcExp, incrementChange); - this.$watch(function(scope){ - var newScope = scope.$eval(scopeExp); - if (newScope !== oldScope) { - oldScope = newScope; - incrementChange(); - } - }); - this.$watch(function() {return changeCounter;}, function(scope) { + this.$watch(function() { + var includeScope = scope.$eval(scopeExp); + if (includeScope) return includeScope.$id; + }, incrementChange); + this.$watch(function() {return changeCounter;}, function(scope, newChangeCounter) { var src = scope.$eval(srcExp), useScope = scope.$eval(scopeExp); function clearContent() { - childScope = null; - element.html(''); + // if this callback is still desired + if (newChangeCounter === changeCounter) { + if (childScope) childScope.$destroy(); + childScope = null; + element.html(''); + } } - while(releaseScopes.length) { - releaseScopes.pop().$destroy(); - } if (src) { $http.get(src, {cache: $templateCache}).success(function(response) { - element.html(response); - if (useScope) { - childScope = useScope; - } else { - releaseScopes.push(childScope = scope.$new()); + // if this callback is still desired + if (newChangeCounter === changeCounter) { + element.html(response); + if (childScope) childScope.$destroy(); + childScope = useScope ? useScope : scope.$new(); + compiler.compile(element)(childScope); + $autoScroll(); + scope.$eval(onloadExp); } - compiler.compile(element)(childScope); - $autoScroll(); - scope.$eval(onloadExp); }).error(clearContent); } else { clearContent(); @@ -574,7 +570,10 @@ angularWidget('ng:view', function(element) { var template = $route.current && $route.current.template; function clearContent() { - element.html(''); + // ignore callback if another route change occured since + if (newChangeCounter == changeCounter) { + element.html(''); + } } if (template) { diff --git a/test/widgetsSpec.js b/test/widgetsSpec.js index a3f7282b74e1..d2867d0937a7 100644 --- a/test/widgetsSpec.js +++ b/test/widgetsSpec.js @@ -67,7 +67,7 @@ describe('widget', function() { it('should include on external file', inject(putIntoCache('myUrl', '{{name}}'), function($rootScope, $compile, $browser) { var element = jqLite(''); - var element = $compile(element)($rootScope); + element = $compile(element)($rootScope); $rootScope.childScope = $rootScope.$new(); $rootScope.childScope.name = 'misko'; $rootScope.url = 'myUrl'; @@ -81,7 +81,7 @@ describe('widget', function() { putIntoCache('myUrl', '{{name}}'), function($rootScope, $compile, $browser) { var element = jqLite(''); - var element = $compile(element)($rootScope); + element = $compile(element)($rootScope); $rootScope.childScope = $rootScope.$new(); $rootScope.childScope.name = 'igor'; $rootScope.url = 'myUrl'; @@ -100,7 +100,7 @@ describe('widget', function() { it('should allow this for scope', inject(putIntoCache('myUrl', '{{"abc"}}'), function($rootScope, $compile, $browser) { var element = jqLite(''); - var element = $compile(element)($rootScope); + element = $compile(element)($rootScope); $rootScope.url = 'myUrl'; $rootScope.$digest(); $browser.defer.flush(); @@ -119,7 +119,7 @@ describe('widget', function() { putIntoCache('myUrl', 'my partial'), function($rootScope, $compile, $browser) { var element = jqLite(''); - var element = $compile(element)($rootScope); + element = $compile(element)($rootScope); expect($rootScope.loaded).not.toBeDefined(); @@ -135,7 +135,7 @@ describe('widget', function() { it('should destroy old scope', inject(putIntoCache('myUrl', 'my partial'), function($rootScope, $compile, $browser) { var element = jqLite(''); - var element = $compile(element)($rootScope); + element = $compile(element)($rootScope); expect($rootScope.$$childHead).toBeFalsy(); @@ -199,6 +199,30 @@ describe('widget', function() { $browser.defer.flush(); expect(element.text()).toBe('my partial'); })); + + it('should discard pending xhr callbacks if a new template is requested before the current ' + + 'finished loading', inject(function($rootScope, $compile, $httpBackend) { + var element = jqLite(""), + log = []; + + $rootScope.templateUrl = 'myUrl1'; + $rootScope.logger = function(msg) { + log.push(msg); + } + $compile(element)($rootScope); + expect(log.join('; ')).toEqual(''); + + $httpBackend.expect('GET', 'myUrl1').respond('
{{logger("url1")}}
'); + $rootScope.$digest(); + expect(log.join('; ')).toEqual(''); + $rootScope.templateUrl = 'myUrl2'; + $httpBackend.expect('GET', 'myUrl2').respond('
{{logger("url2")}}
'); + $rootScope.$digest(); + $httpBackend.flush(); // now that we have two requests pending, flush! + + expect(log.join('; ')).toEqual('url2; url2'); // it's here twice because we go through at + // least two digest cycles + })); })); @@ -645,7 +669,7 @@ describe('widget', function() { $location.path('/bar'); $httpBackend.expect('GET', 'myUrl2').respond('
{{1+1}}
'); $rootScope.$digest(); - $httpBackend.flush(); // now that we have to requests pending, flush! + $httpBackend.flush(); // now that we have two requests pending, flush! expect($rootScope.$element.text()).toEqual('2'); }));