From a70e2833ea276107b11aafea96ef4a6724ad4d83 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] 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); }); });