From 091c390032130c7a281cf051fc8c134225df7be9 Mon Sep 17 00:00:00 2001 From: vsavkin Date: Fri, 28 Oct 2016 14:56:08 -0700 Subject: [PATCH] fix(router): run navigations serialy Closes #11754 --- modules/@angular/router/src/router.ts | 122 +++++++++++++----- .../@angular/router/src/router_preloader.ts | 3 +- .../@angular/router/test/integration.spec.ts | 86 +++++++++++- 3 files changed, 169 insertions(+), 42 deletions(-) diff --git a/modules/@angular/router/src/router.ts b/modules/@angular/router/src/router.ts index 6a6bfe659e..1f1c01bcdb 100644 --- a/modules/@angular/router/src/router.ts +++ b/modules/@angular/router/src/router.ts @@ -8,10 +8,12 @@ import {Location} from '@angular/common'; import {Compiler, ComponentFactoryResolver, Injector, NgModuleFactoryLoader, ReflectiveInjector, Type} from '@angular/core'; +import {BehaviorSubject} from 'rxjs/BehaviorSubject'; import {Observable} from 'rxjs/Observable'; import {Subject} from 'rxjs/Subject'; import {Subscription} from 'rxjs/Subscription'; import {from} from 'rxjs/observable/from'; +import {fromPromise} from 'rxjs/observable/fromPromise'; import {of } from 'rxjs/observable/of'; import {concatMap} from 'rxjs/operator/concatMap'; import {every} from 'rxjs/operator/every'; @@ -275,6 +277,16 @@ function defaultErrorHandler(error: any): any { throw error; } +type NavigationParams = { + id: number, + rawUrl: UrlTree, + prevRawUrl: UrlTree, + extras: NavigationExtras, + resolve: any, + reject: any, + promise: Promise +}; + /** * @whatItDoes Provides the navigation and url manipulation capabilities. * @@ -287,11 +299,13 @@ function defaultErrorHandler(error: any): any { export class Router { private currentUrlTree: UrlTree; private rawUrlTree: UrlTree; - private lastNavigation: UrlTree; + + private navigations: BehaviorSubject = + new BehaviorSubject(null); + private routerEvents: Subject = new Subject(); private currentRouterState: RouterState; private locationSubscription: Subscription; - private routerEvents: Subject; private navigationId: number = 0; private configLoader: RouterConfigLoader; @@ -321,11 +335,12 @@ export class Router { private outletMap: RouterOutletMap, private location: Location, private injector: Injector, loader: NgModuleFactoryLoader, compiler: Compiler, public config: Routes) { this.resetConfig(config); - this.routerEvents = new Subject(); this.currentUrlTree = createEmptyUrlTree(); this.rawUrlTree = this.currentUrlTree; this.configLoader = new RouterConfigLoader(loader, compiler); this.currentRouterState = createEmptyState(this.currentUrlTree, this.rootComponentType); + + this.processNavigations(); } /** @@ -355,18 +370,8 @@ export class Router { // which does not work properly with zone.js in IE and Safari this.locationSubscription = this.location.subscribe(Zone.current.wrap((change: any) => { const rawUrlTree = this.urlSerializer.parse(change['url']); - const tree = this.urlHandlingStrategy.extract(rawUrlTree); - - setTimeout(() => { - // we fire multiple events for a single URL change - // we should navigate only once - if (!this.lastNavigation || this.lastNavigation.toString() !== tree.toString()) { - this.scheduleNavigation( - rawUrlTree, tree, {skipLocationChange: change['pop'], replaceUrl: true}); - } else { - this.rawUrlTree = rawUrlTree; - } + this.scheduleNavigation(rawUrlTree, {skipLocationChange: change['pop'], replaceUrl: true}); }, 0); })); } @@ -488,10 +493,11 @@ export class Router { navigateByUrl(url: string|UrlTree, extras: NavigationExtras = {skipLocationChange: false}): Promise { if (url instanceof UrlTree) { - return this.scheduleNavigation(this.rawUrlTree, url, extras); + return this.scheduleNavigation(this.urlHandlingStrategy.merge(url, this.rawUrlTree), extras); } else { const urlTree = this.urlSerializer.parse(url); - return this.scheduleNavigation(this.rawUrlTree, urlTree, extras); + return this.scheduleNavigation( + this.urlHandlingStrategy.merge(urlTree, this.rawUrlTree), extras); } } @@ -518,7 +524,7 @@ export class Router { */ navigate(commands: any[], extras: NavigationExtras = {skipLocationChange: false}): Promise { - return this.scheduleNavigation(this.rawUrlTree, this.createUrlTree(commands, extras), extras); + return this.navigateByUrl(this.createUrlTree(commands, extras), extras); } /** @@ -543,33 +549,76 @@ export class Router { } } - private scheduleNavigation(rawUrl: UrlTree, url: UrlTree, extras: NavigationExtras): - Promise { - if (this.urlHandlingStrategy.shouldProcessUrl(url)) { - const id = ++this.navigationId; - this.routerEvents.next(new NavigationStart(id, this.serializeUrl(url))); + private processNavigations(): void { + concatMap + .call( + this.navigations, + (nav: NavigationParams) => { + if (nav) { + this.executeScheduledNavigation(nav); + // a failed navigation should not stop the router from processing + // further navigations => the catch + return nav.promise.catch(() => {}); + } else { + return of (null); + } + }) + .subscribe(() => {}); + } - return Promise.resolve().then( - (_) => this.runNavigate( - rawUrl, url, extras.skipLocationChange, extras.replaceUrl, id, null)); + private scheduleNavigation(rawUrl: UrlTree, extras: NavigationExtras): Promise { + const prevRawUrl = this.navigations.value ? this.navigations.value.rawUrl : null; + if (prevRawUrl && prevRawUrl.toString() === rawUrl.toString()) { + return this.navigations.value.promise; + } + + let resolve: any = null; + let reject: any = null; + + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + + const id = ++this.navigationId; + this.navigations.next({id, rawUrl, prevRawUrl, extras, resolve, reject, promise}); + + return promise; + } + + private executeScheduledNavigation({id, rawUrl, prevRawUrl, extras, resolve, + reject}: NavigationParams): void { + const url = this.urlHandlingStrategy.extract(rawUrl); + const prevUrl = prevRawUrl ? this.urlHandlingStrategy.extract(prevRawUrl) : null; + const urlTransition = !prevUrl || url.toString() !== prevUrl.toString(); + + if (urlTransition && this.urlHandlingStrategy.shouldProcessUrl(rawUrl)) { + this.routerEvents.next(new NavigationStart(id, this.serializeUrl(url))); + Promise.resolve() + .then( + (_) => this.runNavigate( + url, rawUrl, extras.skipLocationChange, extras.replaceUrl, id, null)) + .then(resolve, reject); // we cannot process the current URL, but we could process the previous one => // we need to do some cleanup - } else if (this.urlHandlingStrategy.shouldProcessUrl(this.rawUrlTree)) { - const id = ++this.navigationId; + } else if ( + urlTransition && prevRawUrl && this.urlHandlingStrategy.shouldProcessUrl(prevRawUrl)) { this.routerEvents.next(new NavigationStart(id, this.serializeUrl(url))); + Promise.resolve() + .then( + (_) => this.runNavigate( + url, rawUrl, false, false, id, createEmptyState(url, this.rootComponentType))) + .then(resolve, reject); - return Promise.resolve().then( - (_) => this.runNavigate( - rawUrl, url, false, false, id, createEmptyState(url, this.rootComponentType))); } else { this.rawUrlTree = rawUrl; - return Promise.resolve(null); + resolve(null); } } private runNavigate( - rawUrl: UrlTree, url: UrlTree, shouldPreventPushState: boolean, shouldReplaceUrl: boolean, + url: UrlTree, rawUrl: UrlTree, shouldPreventPushState: boolean, shouldReplaceUrl: boolean, id: number, precreatedState: RouterState): Promise { if (id !== this.navigationId) { this.location.go(this.urlSerializer.serialize(this.currentUrlTree)); @@ -624,9 +673,15 @@ export class Router { preActivation.traverse(this.outletMap); }); - const preactivation2$ = mergeMap.call(preactivation$, () => preActivation.checkGuards()); + const preactivation2$ = mergeMap.call(preactivation$, () => { + if (this.navigationId !== id) return of (false); + + return preActivation.checkGuards(); + }); const resolveData$ = mergeMap.call(preactivation2$, (shouldActivate: boolean) => { + if (this.navigationId !== id) return of (false); + if (shouldActivate) { return map.call(preActivation.resolveData(), () => shouldActivate); } else { @@ -641,7 +696,6 @@ export class Router { return; } - this.lastNavigation = appliedUrl; this.currentUrlTree = appliedUrl; this.rawUrlTree = this.urlHandlingStrategy.merge(this.currentUrlTree, rawUrl); diff --git a/modules/@angular/router/src/router_preloader.ts b/modules/@angular/router/src/router_preloader.ts index 0ef72be73b..41854ba305 100644 --- a/modules/@angular/router/src/router_preloader.ts +++ b/modules/@angular/router/src/router_preloader.ts @@ -83,7 +83,8 @@ export class RouterPreloader { setUpPreloading(): void { const navigations = filter.call(this.router.events, (e: any) => e instanceof NavigationEnd); - this.subscription = concatMap.call(navigations, () => this.preload()).subscribe((v: any) => {}); + this.subscription = + concatMap.call(navigations, () => this.preload()).subscribe((v: any) => {}); } preload(): Observable { return this.processRoutes(this.injector, this.router.config); } diff --git a/modules/@angular/router/test/integration.spec.ts b/modules/@angular/router/test/integration.spec.ts index f8bd35b264..1e38866d31 100644 --- a/modules/@angular/router/test/integration.spec.ts +++ b/modules/@angular/router/test/integration.spec.ts @@ -20,7 +20,6 @@ import {forEach} from '../src/utils/collection'; import {RouterTestingModule, SpyNgModuleFactoryLoader} from '../testing'; - describe('Integration', () => { beforeEach(() => { TestBed.configureTestingModule({ @@ -42,6 +41,75 @@ describe('Integration', () => { expect(location.path()).toEqual('/simple'); }))); + describe('should execute navigations serialy', () => { + let log: any[] = []; + + beforeEach(() => { + log = []; + + TestBed.configureTestingModule({ + providers: [ + { + provide: 'trueRightAway', + useValue: () => { + log.push('trueRightAway'); + return true; + } + }, + { + provide: 'trueIn2Seconds', + useValue: () => { + log.push('trueIn2Seconds-start'); + let res: any = null; + const p = new Promise(r => res = r); + setTimeout(() => { + log.push('trueIn2Seconds-end'); + res(true); + }, 2000); + return p; + } + } + ] + }); + }); + + it('should execute navigations serialy', + fakeAsync(inject([Router, Location], (router: Router, location: Location) => { + const fixture = createRoot(router, RootCmp); + + router.resetConfig([ + {path: 'a', component: SimpleCmp, canActivate: ['trueRightAway', 'trueIn2Seconds']}, + {path: 'b', component: SimpleCmp, canActivate: ['trueRightAway', 'trueIn2Seconds']} + ]); + + router.navigateByUrl('/a'); + tick(100); + fixture.detectChanges(); + + router.navigateByUrl('/b'); + tick(100); // 200 + fixture.detectChanges(); + + expect(log).toEqual(['trueRightAway', 'trueIn2Seconds-start']); + + tick(2000); // 2200 + fixture.detectChanges(); + + expect(log).toEqual([ + 'trueRightAway', 'trueIn2Seconds-start', 'trueIn2Seconds-end', 'trueRightAway', + 'trueIn2Seconds-start' + ]); + + tick(2000); // 4200 + fixture.detectChanges(); + + expect(log).toEqual([ + 'trueRightAway', 'trueIn2Seconds-start', 'trueIn2Seconds-end', 'trueRightAway', + 'trueIn2Seconds-start', 'trueIn2Seconds-end' + ]); + }))); + }); + it('should work when an outlet is in an ngIf', fakeAsync(inject([Router, Location], (router: Router, location: Location) => { const fixture = createRoot(router, RootCmp); @@ -415,9 +483,9 @@ describe('Integration', () => { [NavigationStart, '/user/init'], [RoutesRecognized, '/user/init'], [NavigationEnd, '/user/init'], - [NavigationStart, '/user/victor'], [NavigationStart, '/user/fedor'], + [NavigationStart, '/user/victor'], [NavigationCancel, '/user/victor'], - [NavigationCancel, '/user/victor'], [RoutesRecognized, '/user/fedor'], + [NavigationStart, '/user/fedor'], [RoutesRecognized, '/user/fedor'], [NavigationEnd, '/user/fedor'] ]); }))); @@ -1458,8 +1526,8 @@ describe('Integration', () => { expect(location.path()).toEqual('/blank'); expectEvents(recordedEvents, [ - [NavigationStart, '/lazyFalse/loaded'], [NavigationStart, '/blank'], - [RoutesRecognized, '/blank'], [NavigationCancel, '/lazyFalse/loaded'], + [NavigationStart, '/lazyFalse/loaded'], [NavigationCancel, '/lazyFalse/loaded'], + [NavigationStart, '/blank'], [RoutesRecognized, '/blank'], [NavigationEnd, '/blank'] ]); }))); @@ -1961,6 +2029,7 @@ describe('Integration', () => { const config: any = router.config; const firstConfig = config[1]._loadedConfig; + expect(firstConfig).toBeDefined(); expect(firstConfig.routes[0].path).toEqual('LoadedModule1'); @@ -1979,8 +2048,11 @@ describe('Integration', () => { extract(url: UrlTree): UrlTree { const oldRoot = url.root; - const root = new UrlSegmentGroup( - oldRoot.segments, {[PRIMARY_OUTLET]: oldRoot.children[PRIMARY_OUTLET]}); + const children: any = {}; + if (oldRoot.children[PRIMARY_OUTLET]) { + children[PRIMARY_OUTLET] = oldRoot.children[PRIMARY_OUTLET]; + } + const root = new UrlSegmentGroup(oldRoot.segments, children); return new UrlTree(root, url.queryParams, url.fragment); }