Skip to content

Commit

Permalink
fix(router): Do not clear currentNavigation if already set to next one (
Browse files Browse the repository at this point in the history
#43852)

Experimentation with the Router URL management exposed a situation where
the `currentNavigation` was being cleared in the `finalize` after the
`currentNavigation` was already set to the next one.

This change ensures that the `currentNavigation` is only cleared if the
id of the finalized transition matches the one on the
`currentNavigation` object.

PR Close #43852
  • Loading branch information
atscott authored and AndrewKushnir committed Oct 18, 2021
1 parent a3ee474 commit 32f368a
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 4 deletions.
9 changes: 5 additions & 4 deletions packages/router/src/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -923,10 +923,11 @@ export class Router {
t.id} is not equal to the current navigation id ${this.navigationId}`;
this.cancelNavigationTransition(t, cancelationReason);
}
// currentNavigation should always be reset to null here. If navigation was
// successful, lastSuccessfulTransition will have already been set. Therefore
// we can safely set currentNavigation to null here.
this.currentNavigation = null;
// Only clear current navigation if it is still set to the one that
// finalized.
if (this.currentNavigation?.id === t.id) {
this.currentNavigation = null;
}
}),
catchError((e) => {
// TODO(atscott): The NavigationTransition `t` used here does not accurately
Expand Down
25 changes: 25 additions & 0 deletions packages/router/test/integration.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1310,6 +1310,31 @@ describe('Integration', () => {
expect(fixture.nativeElement.innerHTML).toContain('simple');
expect(location.urlChanges).toEqual(['hash: /blocked']);
}));

it('should accurately track currentNavigation', fakeAsync(() => {
const router = TestBed.inject(Router);
router.resetConfig([
{path: 'one', component: SimpleCmp, canActivate: ['in1Second']},
{path: 'two', component: BlankCmp, canActivate: ['in1Second']},
]);

router.events.subscribe((e) => {
if (e instanceof NavigationStart) {
if (e.url === '/one') {
router.navigateByUrl('two');
}
router.events.subscribe((e) => {
if (e instanceof GuardsCheckEnd) {
expect(router.getCurrentNavigation()?.extractedUrl.toString()).toEqual('/two');
expect(router.getCurrentNavigation()?.extras).toBeDefined();
}
});
}
});

router.navigateByUrl('one');
tick(1000);
}));
});

it('should support secondary routes', fakeAsync(inject([Router], (router: Router) => {
Expand Down

0 comments on commit 32f368a

Please sign in to comment.