From e1c1bca37d570573221e4e476f0983f5b0d44545 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Tue, 24 May 2016 14:59:32 +0100 Subject: [PATCH 1/5] fix($route): don't process route change controllers and templates for `redirectTo` routes If a route defines a `redirectTo` value, the current route should stop processing the route and instead wait to process the route of the redirect. Currently, what is happening is that Angular detects a redirectTo value and updates $location, but then continues processing the route that was intended to be redirected. Templates are loaded, resolves are processed, ng-view then updates the view with a new template and instantiates the controller all for a route that should have been redirected. A common use case for assigning a function to the redirectTo is to validation authentication, permission check, or some other logic to determine if the route should continue or redirect else where. In the end, this happens, but in between, unexpected results may occur by updating the view and instantiating controllers for logic that wasn't expected to be executed. http://plnkr.co/edit/8QlA0ouuePH3p35Ntmjy?p=preview This commit checks for a url change after a potential redirect, it the url changed, and is not `null` nor `undefined`, exit out and don't process current route change. Closes #3332 BREAKING CHANGE The $route service no longer instantiates controllers nor gets templates for routes that have a `redirectTo` that is a defined value and different to the current route url. --- src/ngRoute/route.js | 13 +++++++--- test/ngRoute/routeSpec.js | 51 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 60 insertions(+), 4 deletions(-) diff --git a/src/ngRoute/route.js b/src/ngRoute/route.js index f023967a22a..2cd76502086 100644 --- a/src/ngRoute/route.js +++ b/src/ngRoute/route.js @@ -588,12 +588,19 @@ function $RouteProvider() { $route.current = nextRoute; if (nextRoute) { if (nextRoute.redirectTo) { + var url = $location.url(); + var newUrl; if (angular.isString(nextRoute.redirectTo)) { - $location.path(interpolate(nextRoute.redirectTo, nextRoute.params)).search(nextRoute.params) + $location.path(interpolate(nextRoute.redirectTo, nextRoute.params)) + .search(nextRoute.params) .replace(); + newUrl = $location.url(); } else { - $location.url(nextRoute.redirectTo(nextRoute.pathParams, $location.path(), $location.search())) - .replace(); + newUrl = nextRoute.redirectTo(nextRoute.pathParams, $location.path(), $location.search()); + $location.url(newUrl).replace(); + } + if (isDefined(newUrl) && url !== newUrl) { + return; //exit out and don't process current next value, wait for next location change from redirect } } } diff --git a/test/ngRoute/routeSpec.js b/test/ngRoute/routeSpec.js index b6527438af2..87a38749372 100644 --- a/test/ngRoute/routeSpec.js +++ b/test/ngRoute/routeSpec.js @@ -1,6 +1,6 @@ 'use strict'; -describe('$route', function() { +fdescribe('$route', function() { var $httpBackend, element; @@ -1061,6 +1061,55 @@ describe('$route', function() { .toEqual(['http://server/#!/bar/id3?extra=eId', true, null]); }); }); + + it('should not instantiate controller or process template for a redirected route', function() { + var firstController = jasmine.createSpy('first controller spy'); + var firstTemplate = jasmine.createSpy('first template spy').and.returnValue('redirected view'); + var secondController = jasmine.createSpy('second controller spy'); + var secondTemplate = jasmine.createSpy('second template spy').and.returnValue('redirected view'); + module(function($routeProvider) { + $routeProvider.when('/redirect', { + template: firstTemplate, + redirectTo: '/redirected', + controller: firstController + }); + $routeProvider.when('/redirected', { + template: secondTemplate, + controller: secondController + }); + }); + inject(function($route, $location, $rootScope, $compile) { + var element = $compile('
')($rootScope); + $location.path('/redirect'); + $rootScope.$digest(); + expect(firstController).not.toHaveBeenCalled(); + expect(firstTemplate).not.toHaveBeenCalled(); + expect(secondController).toHaveBeenCalled(); + expect(secondTemplate).toHaveBeenCalled(); + dealoc(element); + }); + }); + + it('should not redirect transition if `redirectTo` returns `undefined`', function() { + var controller = jasmine.createSpy('first controller spy'); + var templateFn = jasmine.createSpy('first template spy').and.returnValue('redirected view'); + module(function($routeProvider) { + $routeProvider.when('/redirect/to/undefined', { + template: templateFn, + redirectTo: function() {}, + controller: controller + }); + }); + inject(function($route, $location, $rootScope, $compile) { + var element = $compile('
')($rootScope); + $location.path('/redirect/to/undefined'); + $rootScope.$digest(); + expect(controller).toHaveBeenCalled(); + expect(templateFn).toHaveBeenCalled(); + expect($location.path()).toEqual('/redirect/to/undefined'); + dealoc(element); + }); + }); }); From 06c0a2bf8be09dcd5586d15783dc4ef646922a94 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Tue, 24 May 2016 12:25:16 +0100 Subject: [PATCH 2/5] refactor($route): move getting the template into its own function --- src/ngRoute/route.js | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/ngRoute/route.js b/src/ngRoute/route.js index 2cd76502086..cf37f56b10d 100644 --- a/src/ngRoute/route.js +++ b/src/ngRoute/route.js @@ -616,19 +616,7 @@ function $RouteProvider() { $injector.get(value) : $injector.invoke(value, null, null, key); }); - if (angular.isDefined(template = nextRoute.template)) { - if (angular.isFunction(template)) { - template = template(nextRoute.params); - } - } else if (angular.isDefined(templateUrl = nextRoute.templateUrl)) { - if (angular.isFunction(templateUrl)) { - templateUrl = templateUrl(nextRoute.params); - } - if (angular.isDefined(templateUrl)) { - nextRoute.loadedTemplateUrl = $sce.valueOf(templateUrl); - template = $templateRequest(templateUrl); - } - } + template = getTemplateFor(nextRoute); if (angular.isDefined(template)) { locals['$template'] = template; } @@ -653,6 +641,25 @@ function $RouteProvider() { } + function getTemplateFor(route) { + var template, templateUrl; + if (angular.isDefined(template = route.template)) { + if (angular.isFunction(template)) { + template = template(route.params); + } + } else if (angular.isDefined(templateUrl = route.templateUrl)) { + if (angular.isFunction(templateUrl)) { + templateUrl = templateUrl(route.params); + } + if (angular.isDefined(templateUrl)) { + route.loadedTemplateUrl = $sce.valueOf(templateUrl); + template = $templateRequest(templateUrl); + } + } + return template; + } + + /** * @returns {Object} the current active route, by matching it against the URL */ From 3aecef9afe6eacc4f67d1278fe29e2ad585f9e22 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Tue, 24 May 2016 12:29:12 +0100 Subject: [PATCH 3/5] refactor($route): move resolving locals into its own function --- src/ngRoute/route.js | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/ngRoute/route.js b/src/ngRoute/route.js index cf37f56b10d..87a1b72639c 100644 --- a/src/ngRoute/route.js +++ b/src/ngRoute/route.js @@ -608,15 +608,8 @@ function $RouteProvider() { $q.when(nextRoute). then(function() { if (nextRoute) { - var locals = angular.extend({}, nextRoute.resolve), - template, templateUrl; - - angular.forEach(locals, function(value, key) { - locals[key] = angular.isString(value) ? - $injector.get(value) : $injector.invoke(value, null, null, key); - }); - - template = getTemplateFor(nextRoute); + var locals = resolveLocalsFor(nextRoute); + var template = getTemplateFor(nextRoute); if (angular.isDefined(template)) { locals['$template'] = template; } @@ -640,6 +633,16 @@ function $RouteProvider() { } } + function resolveLocalsFor(route) { + var locals = angular.extend({}, route.resolve); + angular.forEach(locals, function(value, key) { + locals[key] = angular.isString(value) ? + $injector.get(value) : + $injector.invoke(value, null, null, key); + }); + return locals; + } + function getTemplateFor(route) { var template, templateUrl; From 7db30b1802edee83978ae76bcaa63cde1ee7ec50 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Tue, 24 May 2016 14:44:15 +0100 Subject: [PATCH 4/5] refactor($route): consolidate route locals processing into a single handler --- src/ngRoute/route.js | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/src/ngRoute/route.js b/src/ngRoute/route.js index 87a1b72639c..756de5012ee 100644 --- a/src/ngRoute/route.js +++ b/src/ngRoute/route.js @@ -606,16 +606,7 @@ function $RouteProvider() { } $q.when(nextRoute). - then(function() { - if (nextRoute) { - var locals = resolveLocalsFor(nextRoute); - var template = getTemplateFor(nextRoute); - if (angular.isDefined(template)) { - locals['$template'] = template; - } - return $q.all(locals); - } - }). + then(resolveLocals). then(function(locals) { // after route change if (nextRoute === $route.current) { @@ -633,14 +624,20 @@ function $RouteProvider() { } } - function resolveLocalsFor(route) { - var locals = angular.extend({}, route.resolve); - angular.forEach(locals, function(value, key) { - locals[key] = angular.isString(value) ? - $injector.get(value) : - $injector.invoke(value, null, null, key); - }); - return locals; + function resolveLocals(route) { + if (route) { + var locals = angular.extend({}, route.resolve); + angular.forEach(locals, function(value, key) { + locals[key] = angular.isString(value) ? + $injector.get(value) : + $injector.invoke(value, null, null, key); + }); + var template = getTemplateFor(route); + if (angular.isDefined(template)) { + locals['$template'] = template; + } + return $q.all(locals); + } } From 0206de768e8a323ac25f2fa8d0c042aacfdb06d1 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Tue, 24 May 2016 16:33:14 +0100 Subject: [PATCH 5/5] fix($route): don't process route change controllers and templates for `redirectTo` routes If a route defines a `redirectTo` value, the current route should stop processing the route and instead wait to process the route of the redirect. Currently, what is happening is that Angular detects a redirectTo value and updates $location, but then continues processing the route that was intended to be redirected. Templates are loaded, resolves are processed, ng-view then updates the view with a new template and instantiates the controller all for a route that should have been redirected. A common use case for assigning a function to the redirectTo is to validation authentication, permission check, or some other logic to determine if the route should continue or redirect else where. In the end, this happens, but in between, unexpected results may occur by updating the view and instantiating controllers for logic that wasn't expected to be executed. http://plnkr.co/edit/8QlA0ouuePH3p35Ntmjy?p=preview This commit checks for a url change after a potential redirect, it the url changed, and is not `null` nor `undefined`, exit out and don't process current route change. Closes #3332 BREAKING CHANGE The $route service no longer instantiates controllers nor calls resolves or template functions for routes that have a `redirectTo` unless the `redirectTo` is a function that returns `undefined`. --- src/ngRoute/route.js | 8 +++++++- test/ngRoute/routeSpec.js | 13 +++++++++++-- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/ngRoute/route.js b/src/ngRoute/route.js index 756de5012ee..3d86e14767f 100644 --- a/src/ngRoute/route.js +++ b/src/ngRoute/route.js @@ -136,6 +136,12 @@ function $RouteProvider() { * The custom `redirectTo` function is expected to return a string which will be used * to update `$location.path()` and `$location.search()`. * + * Routes that specify `redirectTo` will not have their controllers, template functions + * or resolves called, the `$location` will be changed to the redirect url and route + * processing will stop. The exception to this is if the `redirectTo` is a function that + * returns `undefined`. In this case the route transition occurs as though their was no + * redirection. + * * - `[reloadOnSearch=true]` - `{boolean=}` - reload route when only `$location.search()` * or `$location.hash()` changes. * @@ -599,7 +605,7 @@ function $RouteProvider() { newUrl = nextRoute.redirectTo(nextRoute.pathParams, $location.path(), $location.search()); $location.url(newUrl).replace(); } - if (isDefined(newUrl) && url !== newUrl) { + if (angular.isDefined(newUrl) && url !== newUrl) { return; //exit out and don't process current next value, wait for next location change from redirect } } diff --git a/test/ngRoute/routeSpec.js b/test/ngRoute/routeSpec.js index 87a38749372..aa1d283fda8 100644 --- a/test/ngRoute/routeSpec.js +++ b/test/ngRoute/routeSpec.js @@ -1,6 +1,6 @@ 'use strict'; -fdescribe('$route', function() { +describe('$route', function() { var $httpBackend, element; @@ -1062,19 +1062,23 @@ fdescribe('$route', function() { }); }); - it('should not instantiate controller or process template for a redirected route', function() { + it('should not process route bits', function() { var firstController = jasmine.createSpy('first controller spy'); var firstTemplate = jasmine.createSpy('first template spy').and.returnValue('redirected view'); + var firstResolve = jasmine.createSpy('first resolve spy'); var secondController = jasmine.createSpy('second controller spy'); var secondTemplate = jasmine.createSpy('second template spy').and.returnValue('redirected view'); + var secondResolve = jasmine.createSpy('second resolve spy'); module(function($routeProvider) { $routeProvider.when('/redirect', { template: firstTemplate, redirectTo: '/redirected', + resolve: { value: firstResolve }, controller: firstController }); $routeProvider.when('/redirected', { template: secondTemplate, + resolve: { value: secondResolve }, controller: secondController }); }); @@ -1082,10 +1086,15 @@ fdescribe('$route', function() { var element = $compile('
')($rootScope); $location.path('/redirect'); $rootScope.$digest(); + expect(firstController).not.toHaveBeenCalled(); expect(firstTemplate).not.toHaveBeenCalled(); + expect(firstResolve).not.toHaveBeenCalled(); + expect(secondController).toHaveBeenCalled(); expect(secondTemplate).toHaveBeenCalled(); + expect(secondResolve).toHaveBeenCalled(); + dealoc(element); }); });