fix($route): update current route upon service instantiation #5681
Conversation
| @@ -389,7 +389,7 @@ describe('$route', function() { | |||
| var onChangeSpy = jasmine.createSpy('onChange'); | |||
|
|
|||
| $rootScope.$on('$routeChangeStart', onChangeSpy); | |||
| expect($route.current).toBeUndefined(); | |||
| expect($route.current).not.toBeUndefined(); | |||
There was a problem hiding this comment.
we should have an explicit assertion and not not.toBeUndefined()
There was a problem hiding this comment.
As in checking a specific parameter or just fixing up the silly double negative?
There was a problem hiding this comment.
checking specific parameter
|
can you please fix the assertions? otherwise this looks good to me. |
ghost
assigned
|
I tried to fix the assert but realized that $route.current is in the first test set to some object which doesn't match any of the route definitions. I also noticed that there is no new test added for this PR. we should add one because we'll regress on this sooner or later. |
|
I'm going to punt on this PR for now and assign it to 1.2.9 |
|
I'll just fix the double negative now. The modified assertions should be enough to imply that $route.current has been set because beforehand it was undefined. In a regression I'd imagine it'd go back to being undefined again. |
This fixes cases where the first ngView is loaded in a template asynchronously (such as through ngInclude), as the service will miss the first event otherwise. Closes #4957
|
This pull request broke some of our code due to a change in the ordering of the onLocationSuccess event and the route resolve logic. In the past, onLocationSuccess was always fired before the route's resolve method was called. As of this change, onLocationSuccess fired after the resolve method when navigating to a new page (though no change was observed on initial page load). |
This reverts commit 2b344db. I think I merged this commit prematurely and in addition to that we found out that it's breaking google apps. Jen Bourey will provide more info at the original PR angular#5681
This reverts commit 2b344db. I think I merged this commit prematurely and in addition to that we found out that it's breaking google apps. Jen Bourey will provide more info at the original PR angular#5681
|
Apologies for the delayed response, I've been super busy lately. I spent an hour or two trying to recreate the issue you are describing in plunker and haven't had good results! Could someone give me some guidance on exactly what's going wrong and maybe an example route setup? That would be really helpful. I'm looking to get this PR fixed up in the next week! |
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
|
Since this original fix breaks other things, we have a new router in the works, and it appears that there is a workaround for this issue at #4957 (comment), I am going to close this issue. |
This fixes cases where the first ngView is loaded in a template asynchronously (such as through ngInclude), as the service will miss the first $locationChangeSuccess event otherwise.
Closes #4957