Skip to content

Commit

Permalink
fix(router): ensure duplicate popstate/hashchange events are handled …
Browse files Browse the repository at this point in the history 
…correctly (#37674)

The current method of handling duplicate navigations caused by 'hashchange' and 'popstate' events for the same url change does not correctly handle cancelled navigations. Because `scheduleNavigation` is called in a `setTimeout` in the location change subscription, the duplicate navigations are not flushed at the same time. This means that if the initial navigation hits a guard that schedules a new navigation, the navigation for the duplicate event will not compare to the correct transition (because we inserted another navigation between the duplicates). See #16710 (comment)

Fixes #16710

PR Close #37674
  • Loading branch information
@atscott
atscott committed Jul 13, 2020
1 parent 69472a1 commit 9185c6e
Show file tree
Hide file tree
Showing 3 changed files with 118 additions and 45 deletions.
4 changes: 2 additions & 2 deletions goldens/size-tracking/integration-payloads.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
"master": {
"uncompressed": {
"runtime-es2015": 2289,
"main-es2015": 245488,
"main-es2015": 246044,
"polyfills-es2015": 36938,
"5-es2015": 751
}
Expand All @@ -49,7 +49,7 @@
"master": {
"uncompressed": {
"runtime-es2015": 2289,
"main-es2015": 221268,
"main-es2015": 221897,
"polyfills-es2015": 36938,
"5-es2015": 779
}
Expand Down
99 changes: 69 additions & 30 deletions packages/router/src/router.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
* found in the LICENSE file at https://angular.io/license
*/

import {Location} from '@angular/common';
import {Location, PopStateEvent} from '@angular/common';
import {Compiler, Injectable, Injector, isDevMode, NgModuleFactoryLoader, NgModuleRef, NgZone, Type, ɵConsole as Console} from '@angular/core';
import {BehaviorSubject, EMPTY, Observable, of, Subject, Subscription} from 'rxjs';
import {BehaviorSubject, EMPTY, Observable, of, Subject, SubscriptionLike} from 'rxjs';
import {catchError, filter, finalize, map, switchMap, tap} from 'rxjs/operators';

import {QueryParamsHandling, Route, Routes, standardizeConfig, validateConfig} from './config';
Expand Down Expand Up @@ -276,6 +276,16 @@ function defaultRouterHook(snapshot: RouterStateSnapshot, runExtras: {
return of(null) as any;
}

/**
* Information related to a location change, necessary for scheduling follow-up Router navigations.
*/
type LocationChangeInfo = {
source: 'popstate'|'hashchange',
urlTree: UrlTree,
state: RestoredState|null,
transitionId: number
};

/**
* @description
*
Expand All @@ -298,8 +308,12 @@ export class Router {
private lastSuccessfulNavigation: Navigation|null = null;
private currentNavigation: Navigation|null = null;

// TODO(issue/24571): remove '!'.
private locationSubscription!: Subscription;
private locationSubscription?: SubscriptionLike;
/**
* Tracks the previously seen location change from the location subscription so we can compare
* the two latest to see if they are duplicates. See setUpLocationChangeListener.
*/
private lastLocationChangeInfo: LocationChangeInfo|null = null;
private navigationId: number = 0;
private configLoader: RouterConfigLoader;
private ngModule: NgModuleRef<any>;
Expand Down Expand Up @@ -851,26 +865,66 @@ export class Router {
}

/**
* Sets up the location change listener.
* Sets up the location change listener. This listener detects navigations triggered from outside
* the Router (the browser back/forward buttons, for example) and schedules a corresponding Router
* navigation so that the correct events, guards, etc. are triggered.
*/
setUpLocationChangeListener(): void {
// Don't need to use Zone.wrap any more, because zone.js
// already patch onPopState, so location change callback will
// run into ngZone
if (!this.locationSubscription) {
this.locationSubscription = <any>this.location.subscribe((change: any) => {
let rawUrlTree = this.parseUrl(change['url']);
const source: NavigationTrigger = change['type'] === 'popstate' ? 'popstate' : 'hashchange';
// Navigations coming from Angular router have a navigationId state property. When this
// exists, restore the state.
const state = change.state && change.state.navigationId ? change.state : null;
setTimeout(() => {
this.scheduleNavigation(rawUrlTree, source, state, {replaceUrl: true});
}, 0);
this.locationSubscription = this.location.subscribe(event => {
const currentChange = this.extractLocationChangeInfoFromEvent(event);
if (this.shouldScheduleNavigation(this.lastLocationChangeInfo, currentChange)) {
// The `setTimeout` was added in #12160 and is likely to support Angular/AngularJS
// hybrid apps.
setTimeout(() => {
const {source, state, urlTree} = currentChange;
this.scheduleNavigation(urlTree, source, state, {replaceUrl: true});
}, 0);
}
this.lastLocationChangeInfo = currentChange;
});
}
}

/** Extracts router-related information from a `PopStateEvent`. */
private extractLocationChangeInfoFromEvent(change: PopStateEvent): LocationChangeInfo {
return {
source: change['type'] === 'popstate' ? 'popstate' : 'hashchange',
urlTree: this.parseUrl(change['url']!),
// Navigations coming from Angular router have a navigationId state
// property. When this exists, restore the state.
state: change.state?.navigationId ? change.state : null,
transitionId: this.getTransition().id
} as const;
}

/**
* Determines whether two events triggered by the Location subscription are due to the same
* navigation. The location subscription can fire two events (popstate and hashchange) for a
* single navigation. The second one should be ignored, that is, we should not schedule another
* navigation in the Router.
*/
private shouldScheduleNavigation(previous: LocationChangeInfo|null, current: LocationChangeInfo):
boolean {
if (!previous) return true;

const sameDestination = current.urlTree.toString() === previous.urlTree.toString();
const eventsOccurredAtSameTime = current.transitionId === previous.transitionId;
if (!eventsOccurredAtSameTime || !sameDestination) {
return true;
}

if ((current.source === 'hashchange' && previous.source === 'popstate') ||
(current.source === 'popstate' && previous.source === 'hashchange')) {
return false;
}

return true;
}

/** The current URL. */
get url(): string {
return this.serializeUrl(this.currentUrlTree);
Expand Down Expand Up @@ -918,7 +972,7 @@ export class Router {
dispose(): void {
if (this.locationSubscription) {
this.locationSubscription.unsubscribe();
this.locationSubscription = null!;
this.locationSubscription = undefined;
}
}

Expand Down Expand Up @@ -1151,21 +1205,6 @@ export class Router {
return Promise.resolve(true); // return value is not used
}

// Because of a bug in IE and Edge, the location class fires two events (popstate and
// hashchange) every single time. The second one should be ignored. Otherwise, the URL will
// flicker. Handles the case when a popstate was emitted first.
if (lastNavigation && source == 'hashchange' && lastNavigation.source === 'popstate' &&
lastNavigation.rawUrl.toString() === rawUrl.toString()) {
return Promise.resolve(true); // return value is not used
}
// Because of a bug in IE and Edge, the location class fires two events (popstate and
// hashchange) every single time. The second one should be ignored. Otherwise, the URL will
// flicker. Handles the case when a hashchange was emitted first.
if (lastNavigation && source == 'popstate' && lastNavigation.source === 'hashchange' &&
lastNavigation.rawUrl.toString() === rawUrl.toString()) {
return Promise.resolve(true); // return value is not used
}

let resolve: any;
let reject: any;
let promise: Promise<boolean>;
Expand Down
60 changes: 47 additions & 13 deletions packages/router/test/integration.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -910,17 +910,29 @@ describe('Integration', () => {
})));

describe('duplicate in-flight navigations', () => {
@Injectable()
class RedirectingGuard {
constructor(private router: Router) {}
canActivate() {
this.router.navigate(['/simple']);
return false;
}
}

beforeEach(() => {
TestBed.configureTestingModule({
providers: [{
provide: 'in1Second',
useValue: (c: any, a: ActivatedRouteSnapshot, b: RouterStateSnapshot) => {
let res: any = null;
const p = new Promise(_ => res = _);
setTimeout(() => res(true), 1000);
return p;
}
}]
providers: [
{
provide: 'in1Second',
useValue: (c: any, a: ActivatedRouteSnapshot, b: RouterStateSnapshot) => {
let res: any = null;
const p = new Promise(_ => res = _);
setTimeout(() => res(true), 1000);
return p;
}
},
RedirectingGuard
]
});
});

Expand Down Expand Up @@ -966,6 +978,28 @@ describe('Integration', () => {

expect(location.path()).toEqual('/simple');
}));

it('should skip duplicate location events', fakeAsync(() => {
const router = TestBed.inject(Router);
const location = TestBed.inject(Location) as unknown as SpyLocation;
const fixture = createRoot(router, RootCmp);

router.resetConfig([
{path: 'blocked', component: SimpleCmp, canActivate: [RedirectingGuard]},
{path: 'simple', component: SimpleCmp}
]);
router.navigateByUrl('/simple');
advance(fixture);

const recordedEvents = [] as Event[];
router.events.forEach(e => onlyNavigationStartAndEnd(e) && recordedEvents.push(e));

location.simulateUrlPop('/blocked');
location.simulateHashChange('/blocked');

advance(fixture);
expectEvents(recordedEvents, [[NavigationStart, '/blocked']]);
}));
});

it('should support secondary routes', fakeAsync(inject([Router], (router: Router) => {
Expand Down Expand Up @@ -3878,8 +3912,8 @@ describe('Integration', () => {

expectEvents(recordedEvents, [
[NavigationStart, '/lazyFalse/loaded'],
// No GuardCheck events as `canLoad` is a special guard that's not actually part of the
// guard lifecycle.
// No GuardCheck events as `canLoad` is a special guard that's not actually part of
// the guard lifecycle.
[NavigationCancel, '/lazyFalse/loaded'],

[NavigationStart, '/blank'], [RoutesRecognized, '/blank'],
Expand Down Expand Up @@ -4842,8 +4876,8 @@ describe('Integration', () => {
constructor(
lazy: LazyParentComponent, // should be able to inject lazy/direct parent
lazyService: LazyLoadedServiceDefinedInModule, // should be able to inject lazy service
eager:
EagerParentComponent // should use the injector of the location to create a parent
eager: EagerParentComponent // should use the injector of the location to create a
// parent
) {}
}

Expand Down

0 comments on commit 9185c6e

Please sign in to comment.