Skip to content

Commit

Permalink
fix(router): stop emitting to event observable on destroy (#40638)
Browse files Browse the repository at this point in the history
No longer emits to `Router.events` after the router has been destroyed. Also
returns a resolved promise to the navigation methods.

Fixes #40502.

PR Close #40638
  • Loading branch information
crisbeto authored and alxhub committed Feb 4, 2021
1 parent b1d5897 commit 737bc7a
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 0 deletions.
6 changes: 6 additions & 0 deletions packages/router/src/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,7 @@ export class Router {
private navigations: Observable<NavigationTransition>;
private lastSuccessfulNavigation: Navigation|null = null;
private currentNavigation: Navigation|null = null;
private disposed = false;

private locationSubscription?: SubscriptionLike;
/**
Expand Down Expand Up @@ -1043,10 +1044,12 @@ export class Router {

/** Disposes of the router. */
dispose(): void {
this.transitions.complete();
if (this.locationSubscription) {
this.locationSubscription.unsubscribe();
this.locationSubscription = undefined;
}
this.disposed = true;
}

/**
Expand Down Expand Up @@ -1251,6 +1254,9 @@ export class Router {
rawUrl: UrlTree, source: NavigationTrigger, restoredState: RestoredState|null,
extras: NavigationExtras,
priorPromise?: {resolve: any, reject: any, promise: Promise<boolean>}): Promise<boolean> {
if (this.disposed) {
return Promise.resolve(false);
}
// * Imperative navigations (router.navigate) might trigger additional navigations to the same
// URL via a popstate event and the locationChangeListener. We should skip these duplicate
// navs. Duplicates may also be triggered by attempts to sync AngularJS and Angular router
Expand Down
46 changes: 46 additions & 0 deletions packages/router/test/integration.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4504,6 +4504,52 @@ describe('Integration', () => {

expect(navigateSpy.calls.mostRecent().args[1]!.queryParams);
})));


it('should stop emitting events after the router is destroyed',
fakeAsync(inject([Router], (router: Router) => {
const fixture = createRoot(router, RootCmp);
router.resetConfig([{path: 'user/:name', component: UserCmp}]);

let events = 0;
const subscription = router.events.subscribe(() => events++);

router.navigateByUrl('/user/frodo');
advance(fixture);
expect(events).toBeGreaterThan(0);

const previousCount = events;
router.dispose();
router.navigateByUrl('/user/bilbo');
advance(fixture);

expect(events).toBe(previousCount);
subscription.unsubscribe();
})));

it('should resolve navigation promise with false after the router is destroyed',
fakeAsync(inject([Router], (router: Router) => {
const fixture = createRoot(router, RootCmp);
let result = null as boolean | null;
const callback = (r: boolean) => result = r;
router.resetConfig([{path: 'user/:name', component: UserCmp}]);

router.navigateByUrl('/user/frodo').then(callback);
advance(fixture);
expect(result).toBe(true);
result = null as boolean | null;

router.dispose();

router.navigateByUrl('/user/bilbo').then(callback);
advance(fixture);
expect(result).toBe(false);
result = null as boolean | null;

router.navigate(['/user/bilbo']).then(callback);
advance(fixture);
expect(result).toBe(false);
})));
});

describe('routerLinkActive', () => {
Expand Down

0 comments on commit 737bc7a

Please sign in to comment.