diff --git a/goldens/size-tracking/integration-payloads.json b/goldens/size-tracking/integration-payloads.json index cf8bdc7286..93c9f66230 100644 --- a/goldens/size-tracking/integration-payloads.json +++ b/goldens/size-tracking/integration-payloads.json @@ -39,7 +39,7 @@ "master": { "uncompressed": { "runtime-es2015": 2289, - "main-es2015": 245488, + "main-es2015": 246044, "polyfills-es2015": 36938, "5-es2015": 751 } @@ -49,7 +49,7 @@ "master": { "uncompressed": { "runtime-es2015": 2289, - "main-es2015": 221268, + "main-es2015": 221897, "polyfills-es2015": 36938, "5-es2015": 779 } diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index f8508655c2..5d7619e529 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -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'; @@ -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 * @@ -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; @@ -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 = 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); @@ -918,7 +972,7 @@ export class Router { dispose(): void { if (this.locationSubscription) { this.locationSubscription.unsubscribe(); - this.locationSubscription = null!; + this.locationSubscription = undefined; } } @@ -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; diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts index 369936ddfd..ce343becea 100644 --- a/packages/router/test/integration.spec.ts +++ b/packages/router/test/integration.spec.ts @@ -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 + ] }); }); @@ -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) => { @@ -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'], @@ -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 ) {} }