Route continues to be processed even if providing a redirectTo #3332
Conversation
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
|
Sorry, quickly committed the wrong change. There needs to be a check if location was actually changed as a result of redirectTo and if it was changed, then return and exit out. |
…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 Fix is to check for url change post redirect, it url changed, exit out and don't process current route in route.js. Created new unit tests for ngViewSpec.js to validate that controller of redirected route is not executed
PR Checklist (Minor Bugfix)
|
|
@jardilio - Thanks for this PR. Can you take a look at the outstanding tasks above. In particular can you sign the CLA and read up on the commit message guidelines? |
Fixes issue where templateUrl and resolve values are processed and resolved and controllers instantiated for a route which was intented to be redirected. This prevents processing of a route which may have precondition qualifiers to execute and instead follow and process only the redirect path.
|
@jardilio - Have you signed the CLA? |
|
Yes, its signed (Jeff Ardilio), sorry assumed you would have been notified. Also, I did do a rebase to squash the commits and added the appropriate comments. I did not update documentation, wasn't sure that was applicable here. Please let me know if there is anything else you need. |
|
A commit to be picked up is this one: jardilio@63c162b |
ghost
assigned
|
Cleaned up commit ready to be merged: |
| @@ -430,6 +431,9 @@ function $RouteProvider(){ | |||
| $location.url(next.redirectTo(next.pathParams, $location.path(), $location.search())) | |||
| .replace(); | |||
| } | |||
| if (url !== $location.url()) { | |||
There was a problem hiding this comment.
why this if? I think that we should just return no? if you redirect to the same url then you get an infinite loop, but that's a programming error
There was a problem hiding this comment.
@IgorMinar its not necessarily a programming error. In my experience, I may use the redirect function to validate user has pre-qualifying conditions met to hit that area. If they don't, redirect them somewhere else. If they do, keep the course and continue on with the intended route.
It does not currently get caught in an infinite loop since the url didn't change, it doesn't fire any change watchers to re-evaluate.
petebacondarwin
force-pushed
the
master
branch
from
02dc2aa to
fd2d6c0
Compare
jeffbcross
force-pushed
the
master
branch
2 times, most recently
from
cad9560 to
f294244
Compare
jeffbcross
force-pushed
the
master
branch
4 times, most recently
from
e8dc429 to
e83fab9
Compare
petebacondarwin
force-pushed
the
master
branch
from
4dd5a20 to
998c61c
Compare
|
http://plnkr.co/edit/BFNoi9Vk8aRDz0p1SuJG?p=preview reproduction of the bug still affects snapshot --- if the fix isn't too huge, it would probably be good to land |
|
Was this fixed by #9678? |
|
No, I don't think so: http://plnkr.co/edit/zBSjV0HG9XDc1K6oiay5?p=preview |
|
LGTM - I am going to rebase and see if it breaks anything. |
… `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 angular#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.
… `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 angular#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`.
|
@petebacondarwin Seems there is a regression in latest ngRoute 1.6.3 and resolves are again processed twice for routes that have redirectTo value. Most likely the regression is caused by feat($route): implement |
|
@maximnaidenov - can you provide a reproduction? That commit was from almost a year ago. |
|
@petebacondarwin While preparing a sample I realized the problem is in my code and is provoked by the restructuring in the mentioned feature. Sorry for bothering you. |
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