From 04215ae15dd883ccd5c50084243f0d8300b75f96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Tue, 12 Aug 2014 12:40:17 -0400 Subject: [PATCH 1/2] feat($templateRequest): introduce the $templateRequest service This handy service is designed to download and cache template contents and to throw an error when a template request fails. BREAKING CHANGE Angular will now throw a $compile minErr each a template fails to download for ngView, directives and ngMessage template requests. This changes the former behavior of silently ignoring failed HTTP requests--or when the template itself is empty. Please ensure that all directive, ngView and ngMessage code now properly addresses this scenario. NgInclude is uneffected from this change. --- angularFiles.js | 1 + src/AngularPublic.js | 2 + src/ng/compile.js | 11 ++-- src/ng/directive/ngInclude.js | 10 ++-- src/ng/templateRequest.js | 53 +++++++++++++++++ src/ngMessages/messages.js | 8 +-- src/ngRoute/route.js | 8 +-- test/ng/directive/ngIncludeSpec.js | 1 + test/ng/templateRequestSpec.js | 89 ++++++++++++++++++++++++++++ test/ngRoute/directive/ngViewSpec.js | 11 ++-- test/ngRoute/routeSpec.js | 35 +++++------ 11 files changed, 187 insertions(+), 42 deletions(-) create mode 100644 src/ng/templateRequest.js create mode 100644 test/ng/templateRequestSpec.js diff --git a/angularFiles.js b/angularFiles.js index 6c6dc1e59af5..924fcd487fb2 100755 --- a/angularFiles.js +++ b/angularFiles.js @@ -34,6 +34,7 @@ var angularFiles = { 'src/ng/sanitizeUri.js', 'src/ng/sce.js', 'src/ng/sniffer.js', + 'src/ng/templateRequest.js', 'src/ng/timeout.js', 'src/ng/urlUtils.js', 'src/ng/window.js', diff --git a/src/AngularPublic.js b/src/AngularPublic.js index e859a0da7577..14c8383a9b99 100644 --- a/src/AngularPublic.js +++ b/src/AngularPublic.js @@ -78,6 +78,7 @@ $SceDelegateProvider, $SnifferProvider, $TemplateCacheProvider, + $TemplateRequestProvider, $TimeoutProvider, $$RAFProvider, $$AsyncCallbackProvider, @@ -227,6 +228,7 @@ function publishExternalAPI(angular){ $sceDelegate: $SceDelegateProvider, $sniffer: $SnifferProvider, $templateCache: $TemplateCacheProvider, + $templateRequest: $TemplateRequestProvider, $timeout: $TimeoutProvider, $window: $WindowProvider, $$rAF: $$RAFProvider, diff --git a/src/ng/compile.js b/src/ng/compile.js index ac0eab395690..6c40d9b0db7a 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -669,9 +669,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { }; this.$get = [ - '$injector', '$interpolate', '$exceptionHandler', '$http', '$templateCache', '$parse', + '$injector', '$interpolate', '$exceptionHandler', '$templateRequest', '$parse', '$controller', '$rootScope', '$document', '$sce', '$animate', '$$sanitizeUri', - function($injector, $interpolate, $exceptionHandler, $http, $templateCache, $parse, + function($injector, $interpolate, $exceptionHandler, $templateRequest, $parse, $controller, $rootScope, $document, $sce, $animate, $$sanitizeUri) { var Attributes = function(element, attributesToCopy) { @@ -1827,8 +1827,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { $compileNode.empty(); - $http.get($sce.getTrustedResourceUrl(templateUrl), {cache: $templateCache}). - success(function(content) { + $templateRequest($sce.getTrustedResourceUrl(templateUrl)) + .then(function(content) { var compileNode, tempTemplateAttrs, $template, childBoundTranscludeFn; content = denormalizeTemplate(content); @@ -1903,9 +1903,6 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { childBoundTranscludeFn); } linkQueue = null; - }). - error(function(response, code, headers, config) { - throw $compileMinErr('tpload', 'Failed to load template: {0}', config.url); }); return function delayedNodeLinkFn(ignoreChildLinkFn, scope, node, rootElement, boundTranscludeFn) { diff --git a/src/ng/directive/ngInclude.js b/src/ng/directive/ngInclude.js index 8aea896b2dd8..e78e72bc1910 100644 --- a/src/ng/directive/ngInclude.js +++ b/src/ng/directive/ngInclude.js @@ -169,8 +169,8 @@ * @description * Emitted when a template HTTP request yields an erronous response (status < 200 || status > 299) */ -var ngIncludeDirective = ['$http', '$templateCache', '$anchorScroll', '$animate', '$sce', - function($http, $templateCache, $anchorScroll, $animate, $sce) { +var ngIncludeDirective = ['$templateRequest', '$anchorScroll', '$animate', '$sce', + function($templateRequest, $anchorScroll, $animate, $sce) { return { restrict: 'ECA', priority: 400, @@ -215,7 +215,9 @@ var ngIncludeDirective = ['$http', '$templateCache', '$anchorScroll', '$animate' var thisChangeId = ++changeCounter; if (src) { - $http.get(src, {cache: $templateCache}).success(function(response) { + //set the 2nd param to true to ignore the template request error so that the inner + //contents and scope can be cleaned up. + $templateRequest(src, true).then(function(response) { if (thisChangeId !== changeCounter) return; var newScope = scope.$new(); ctrl.template = response; @@ -236,7 +238,7 @@ var ngIncludeDirective = ['$http', '$templateCache', '$anchorScroll', '$animate' currentScope.$emit('$includeContentLoaded'); scope.$eval(onloadExp); - }).error(function() { + }, function() { if (thisChangeId === changeCounter) { cleanupLastIncludeContent(); scope.$emit('$includeContentError'); diff --git a/src/ng/templateRequest.js b/src/ng/templateRequest.js new file mode 100644 index 000000000000..7155718e89a4 --- /dev/null +++ b/src/ng/templateRequest.js @@ -0,0 +1,53 @@ +'use strict'; + +var $compileMinErr = minErr('$compile'); + +/** + * @ngdoc service + * @name $templateRequest + * + * @description + * The `$templateRequest` service downloads the provided template using `$http` and, upon success, + * stores the contents inside of `$templateCache`. If the HTTP request fails or the response data + * of the HTTP request is empty then a `$compile` error will be thrown (the exception can be thwarted + * by setting the 2nd parameter of the function to true). + * + * @param {string} tpl The HTTP request template URL + * @param {boolean=} ignoreRequestError Whether or not to ignore the exception when the request fails or the template is empty + * + * @return {Promise} the HTTP Promise for the given. + * + * @property {number} totalPendingRequests total amount of pending template requests being downloaded. + */ +function $TemplateRequestProvider() { + this.$get = ['$templateCache', '$http', '$q', function($templateCache, $http, $q) { + function handleRequestFn(tpl, ignoreRequestError) { + var self = handleRequestFn; + self.totalPendingRequests++; + + return $http.get(tpl, { cache : $templateCache }) + .then(function(response) { + var html = response.data; + if(!html || html.length === 0) { + return handleError(); + } + + self.totalPendingRequests--; + $templateCache.put(tpl, html); + return html; + }, handleError); + + function handleError() { + self.totalPendingRequests--; + if (!ignoreRequestError) { + throw $compileMinErr('tpload', 'Failed to load template: {0}', tpl); + } + return $q.reject(); + } + } + + handleRequestFn.totalPendingRequests = 0; + + return handleRequestFn; + }]; +} diff --git a/src/ngMessages/messages.js b/src/ngMessages/messages.js index ecf5bc21f435..3254390a5a51 100644 --- a/src/ngMessages/messages.js +++ b/src/ngMessages/messages.js @@ -228,8 +228,8 @@ angular.module('ngMessages', []) * * */ - .directive('ngMessages', ['$compile', '$animate', '$http', '$templateCache', - function($compile, $animate, $http, $templateCache) { + .directive('ngMessages', ['$compile', '$animate', '$templateRequest', + function($compile, $animate, $templateRequest) { var ACTIVE_CLASS = 'ng-active'; var INACTIVE_CLASS = 'ng-inactive'; @@ -296,8 +296,8 @@ angular.module('ngMessages', []) var tpl = $attrs.ngMessagesInclude || $attrs.include; if(tpl) { - $http.get(tpl, { cache: $templateCache }) - .success(function processTemplate(html) { + $templateRequest(tpl) + .then(function processTemplate(html) { var after, container = angular.element('
').html(html); angular.forEach(container.children(), function(elm) { elm = angular.element(elm); diff --git a/src/ngRoute/route.js b/src/ngRoute/route.js index 53b1927d6ad5..b140ddfbc1b8 100644 --- a/src/ngRoute/route.js +++ b/src/ngRoute/route.js @@ -226,10 +226,9 @@ function $RouteProvider(){ '$routeParams', '$q', '$injector', - '$http', - '$templateCache', + '$templateRequest', '$sce', - function($rootScope, $location, $routeParams, $q, $injector, $http, $templateCache, $sce) { + function($rootScope, $location, $routeParams, $q, $injector, $templateRequest, $sce) { /** * @ngdoc service @@ -556,8 +555,7 @@ function $RouteProvider(){ templateUrl = $sce.getTrustedResourceUrl(templateUrl); if (angular.isDefined(templateUrl)) { next.loadedTemplateUrl = templateUrl; - template = $http.get(templateUrl, {cache: $templateCache}). - then(function(response) { return response.data; }); + template = $templateRequest(templateUrl); } } if (angular.isDefined(template)) { diff --git a/test/ng/directive/ngIncludeSpec.js b/test/ng/directive/ngIncludeSpec.js index 1d2f1ec4c89a..4dca3f803dc8 100644 --- a/test/ng/directive/ngIncludeSpec.js +++ b/test/ng/directive/ngIncludeSpec.js @@ -199,6 +199,7 @@ describe('ngInclude', function() { $rootScope.url = 'url2'; $rootScope.$digest(); $httpBackend.flush(); + expect($rootScope.$$childHead).toBeFalsy(); expect(element.text()).toBe(''); diff --git a/test/ng/templateRequestSpec.js b/test/ng/templateRequestSpec.js new file mode 100644 index 000000000000..efc6a182116c --- /dev/null +++ b/test/ng/templateRequestSpec.js @@ -0,0 +1,89 @@ +'use strict'; + +describe('$templateRequest', function() { + + it('should download the provided template file', + inject(function($rootScope, $templateRequest, $httpBackend) { + + $httpBackend.expectGET('tpl.html').respond('
abc
'); + + var content; + $templateRequest('tpl.html').then(function(html) { content = html; }); + + $rootScope.$digest(); + $httpBackend.flush(); + + expect(content).toBe('
abc
'); + })); + + it('should cache the request using $templateCache to prevent extra downloads', + inject(function($rootScope, $templateRequest, $templateCache) { + + $templateCache.put('tpl.html', 'matias'); + + var content; + $templateRequest('tpl.html').then(function(html) { content = html; }); + + $rootScope.$digest(); + expect(content).toBe('matias'); + })); + + it('should throw an error when the template is not found', + inject(function($rootScope, $templateRequest, $httpBackend) { + + $httpBackend.expectGET('tpl.html').respond(404); + + $templateRequest('tpl.html'); + + $rootScope.$digest(); + + expect(function() { + $rootScope.$digest(); + $httpBackend.flush(); + }).toThrowMinErr('$compile', 'tpload', 'Failed to load template: tpl.html'); + })); + + it('should throw an error when the template is empty', + inject(function($rootScope, $templateRequest, $httpBackend) { + + $httpBackend.expectGET('tpl.html').respond(''); + + $templateRequest('tpl.html'); + + $rootScope.$digest(); + + expect(function() { + $rootScope.$digest(); + $httpBackend.flush(); + }).toThrowMinErr('$compile', 'tpload', 'Failed to load template: tpl.html'); + })); + + it('should keep track of how many requests are going on', + inject(function($rootScope, $templateRequest, $httpBackend) { + + $httpBackend.expectGET('a.html').respond('a'); + $httpBackend.expectGET('b.html').respond('c'); + $templateRequest('a.html'); + $templateRequest('b.html'); + + expect($templateRequest.totalPendingRequests).toBe(2); + + $rootScope.$digest(); + $httpBackend.flush(); + + expect($templateRequest.totalPendingRequests).toBe(0); + + $httpBackend.expectGET('c.html').respond(404); + $templateRequest('c.html'); + + expect($templateRequest.totalPendingRequests).toBe(1); + $rootScope.$digest(); + + try { + $httpBackend.flush(); + } catch(e) {} + + expect($templateRequest.totalPendingRequests).toBe(0); + })); + +}); diff --git a/test/ngRoute/directive/ngViewSpec.js b/test/ngRoute/directive/ngViewSpec.js index a832a7b32cc8..6113a2ca2ec8 100644 --- a/test/ngRoute/directive/ngViewSpec.js +++ b/test/ngRoute/directive/ngViewSpec.js @@ -56,7 +56,7 @@ describe('ngView', function() { }); - it('should instantiate controller for empty template', function() { + it('should not instantiate the associated controller when an empty template is downloaded', function() { var log = [], controllerScope, Ctrl = function($scope) { controllerScope = $scope; @@ -70,11 +70,12 @@ describe('ngView', function() { inject(function($route, $rootScope, $templateCache, $location) { $templateCache.put('/tpl.html', [200, '', {}]); $location.path('/some'); - $rootScope.$digest(); - expect(controllerScope.$parent).toBe($rootScope); - expect(controllerScope).toBe($route.current.scope); - expect(log).toEqual(['ctrl-init']); + expect(function() { + $rootScope.$digest(); + }).toThrowMinErr('$compile', 'tpload', 'Failed to load template: /tpl.html'); + + expect(controllerScope).toBeUndefined(); }); }); diff --git a/test/ngRoute/routeSpec.js b/test/ngRoute/routeSpec.js index 5dcf96edcb32..220d4f47b631 100644 --- a/test/ngRoute/routeSpec.js +++ b/test/ngRoute/routeSpec.js @@ -671,35 +671,36 @@ describe('$route', function() { }); - it('should drop in progress route change when new route change occurs and old fails', function() { - module(function($routeProvider) { + it('should throw an error when a template is empty or not found', function() { + module(function($routeProvider, $exceptionHandlerProvider) { + $exceptionHandlerProvider.mode('log'); $routeProvider. when('/r1', { templateUrl: 'r1.html' }). - when('/r2', { templateUrl: 'r2.html' }); + when('/r2', { templateUrl: 'r2.html' }). + when('/r3', { templateUrl: 'r3.html' }); }); - inject(function($route, $httpBackend, $location, $rootScope) { - var log = ''; - $rootScope.$on('$routeChangeError', function(e, next, last, error) { - log += '$failed(' + next.templateUrl + ', ' + error.status + ');'; - }); - $rootScope.$on('$routeChangeStart', function(e, next) { log += '$before(' + next.templateUrl + ');'; }); - $rootScope.$on('$routeChangeSuccess', function(e, next) { log += '$after(' + next.templateUrl + ');'; }); - + inject(function($route, $httpBackend, $location, $rootScope, $exceptionHandler) { $httpBackend.expectGET('r1.html').respond(404, 'R1'); - $httpBackend.expectGET('r2.html').respond('R2'); - $location.path('/r1'); $rootScope.$digest(); - expect(log).toBe('$before(r1.html);'); + $httpBackend.flush(); + expect($exceptionHandler.errors.pop().message).toContain("[$compile:tpload] Failed to load template: r1.html"); + + $httpBackend.expectGET('r2.html').respond(''); $location.path('/r2'); $rootScope.$digest(); - expect(log).toBe('$before(r1.html);$before(r2.html);'); $httpBackend.flush(); - expect(log).toBe('$before(r1.html);$before(r2.html);$after(r2.html);'); - expect(log).not.toContain('$after(r1.html);'); + expect($exceptionHandler.errors.pop().message).toContain("[$compile:tpload] Failed to load template: r2.html"); + + $httpBackend.expectGET('r3.html').respond('abc'); + $location.path('/r3'); + $rootScope.$digest(); + + $httpBackend.flush(); + expect($exceptionHandler.errors.length).toBe(0); }); }); From 71bb4f8c69dbab35eb59a116b55bd4b46c8e7f4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Wed, 27 Aug 2014 20:31:07 -0400 Subject: [PATCH 2/2] fix($animate): ensure guarded animations consider AJAX requests upon bootstrap Prior to this fix when an Angular application is bootstrapped it would only place an animation guard to prevent animations from running when the application starts for the first two digest cycles. However, if any controllers or directives, that are executed upon boostrap, trigger any remote code to be downloaded (via $http) then the guard does not put that into consideration. This fix now properly addresses that circumstance and removes the guard once all outbound HTTP requests are complete when an Angular application is bootstrapped. Closes #8275 Closes #5262 --- src/ngAnimate/animate.js | 52 ++++++++++++++++++++++++-------- test/ng/directive/ngClassSpec.js | 5 +-- test/ngAnimate/animateSpec.js | 42 ++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 17 deletions(-) diff --git a/src/ngAnimate/animate.js b/src/ngAnimate/animate.js index 2b2de1abe367..d8c65712ca79 100644 --- a/src/ngAnimate/animate.js +++ b/src/ngAnimate/animate.js @@ -73,6 +73,16 @@ * When the `on` expression value changes and an animation is triggered then each of the elements within * will all animate without the block being applied to child elements. * + * ## Are animations run when the application starts? + * No they are not. When an application is bootstrapped Angular will disable animations from running to avoid + * a frenzy of animations from being triggered as soon as the browser has rendered the screen. For this to work, + * Angular will wait for two digest cycles until enabling animations. From there on, any animation-triggering + * layout changes in the application will trigger animations as normal. + * + * In addition, upon bootstrap, if the routing system or any directives or load remote data (via $http) then Angular + * will automatically extend the wait time to enable animations once **all** of the outbound HTTP requests + * are complete. + * *

CSS-defined Animations

* The animate service will automatically apply two CSS classes to the animated element and these two CSS classes * are designed to contain the start and end CSS styling. Both CSS transitions and keyframe animations are supported @@ -396,24 +406,40 @@ angular.module('ngAnimate', ['ng']) } $provide.decorator('$animate', - ['$delegate', '$$q', '$injector', '$sniffer', '$rootElement', '$$asyncCallback', '$rootScope', '$document', - function($delegate, $$q, $injector, $sniffer, $rootElement, $$asyncCallback, $rootScope, $document) { + ['$delegate', '$$q', '$injector', '$sniffer', '$rootElement', '$$asyncCallback', '$rootScope', '$document', '$templateRequest', + function($delegate, $$q, $injector, $sniffer, $rootElement, $$asyncCallback, $rootScope, $document, $templateRequest) { - var globalAnimationCounter = 0; $rootElement.data(NG_ANIMATE_STATE, rootAnimateState); // disable animations during bootstrap, but once we bootstrapped, wait again - // for another digest until enabling animations. The reason why we digest twice - // is because all structural animations (enter, leave and move) all perform a - // post digest operation before animating. If we only wait for a single digest - // to pass then the structural animation would render its animation on page load. - // (which is what we're trying to avoid when the application first boots up.) - $rootScope.$$postDigest(function() { - $rootScope.$$postDigest(function() { - rootAnimateState.running = false; - }); - }); + // for another digest until enabling animations. Enter, leave and move require + // a follow-up digest so having a watcher here is enough to let both digests pass. + // However, when any directive or view templates are downloaded then we need to + // handle postpone enabling animations until they are fully completed and then... + var watchFn = $rootScope.$watch( + function() { return $templateRequest.totalPendingRequests; }, + function(val, oldVal) { + if (oldVal === 0) { + if (val === 0) { + $rootScope.$$postDigest(onApplicationReady); + } + } else if(val === 0) { + // ...when the template has been downloaded we digest twice again until the + // animations are set to enabled (since enter, leave and move require a + // follow-up). + $rootScope.$$postDigest(function() { + $rootScope.$$postDigest(onApplicationReady); + }); + } + } + ); + function onApplicationReady() { + rootAnimateState.running = false; + watchFn(); + } + + var globalAnimationCounter = 0; var classNameFilter = $animateProvider.classNameFilter(); var isAnimatableClassName = !classNameFilter ? function() { return true; } diff --git a/test/ng/directive/ngClassSpec.js b/test/ng/directive/ngClassSpec.js index a00708f18d5d..2d4f28cce701 100644 --- a/test/ng/directive/ngClassSpec.js +++ b/test/ng/directive/ngClassSpec.js @@ -427,10 +427,7 @@ describe('ngClass animations', function() { }); inject(function($compile, $rootScope, $browser, $rootElement, $animate, $timeout, $document) { - // Enable animations by triggering the first item in the postDigest queue - digestQueue.shift()(); - - // wait for the 2nd animation bootstrap digest to pass + // Animations are enabled right away since there are no remote HTTP template requests $rootScope.$digest(); digestQueue.shift()(); diff --git a/test/ngAnimate/animateSpec.js b/test/ngAnimate/animateSpec.js index a8f67b1087e4..ef1e0fecab0b 100644 --- a/test/ngAnimate/animateSpec.js +++ b/test/ngAnimate/animateSpec.js @@ -50,6 +50,48 @@ describe("ngAnimate", function() { }); }); + it("should disable animations for two digests until all pending HTTP requests are complete during bootstrap", function() { + var animateSpy = jasmine.createSpy(); + module(function($animateProvider, $compileProvider) { + $compileProvider.directive('myRemoteDirective', function() { + return { + templateUrl : 'remote.html' + }; + }); + $animateProvider.register('.my-structrual-animation', function() { + return { + enter : animateSpy, + leave : animateSpy + }; + }); + }); + inject(function($rootScope, $compile, $animate, $rootElement, $document, $httpBackend) { + + $httpBackend.whenGET('remote.html').respond(200, 'content'); + + var element = $compile('
...
')($rootScope); + $rootElement.append(element); + jqLite($document[0].body).append($rootElement); + + // running this twice just to prove that the dual post digest is run + $rootScope.$digest(); + $rootScope.$digest(); + + $animate.enter(element, $rootElement); + $rootScope.$digest(); + + expect(animateSpy).not.toHaveBeenCalled(); + + $httpBackend.flush(); + $rootScope.$digest(); + + $animate.leave(element); + $rootScope.$digest(); + + expect(animateSpy).toHaveBeenCalled(); + }); + }); + //we use another describe block because the before/after operations below //are used across all animations tests and we don't want that same behavior