From 12ccf57340ce93f6fc8d9edcacb522418b5a822d Mon Sep 17 00:00:00 2001 From: Jason Aden Date: Tue, 11 Sep 2018 22:18:50 -0700 Subject: [PATCH] refactor(router): update test based on router quick-cancelling ongoing navigations (#25740) PR Close #25740 --- packages/router/src/operators/mergeMapIf.ts | 23 -- packages/router/src/operators/resolve_data.ts | 4 +- packages/router/src/operators/throwIf.ts | 26 -- packages/router/src/router.ts | 320 +++++++++--------- packages/router/src/utils/assert.ts | 19 -- packages/router/test/integration.spec.ts | 97 +++--- 6 files changed, 205 insertions(+), 284 deletions(-) delete mode 100644 packages/router/src/operators/mergeMapIf.ts delete mode 100644 packages/router/src/operators/throwIf.ts delete mode 100644 packages/router/src/utils/assert.ts diff --git a/packages/router/src/operators/mergeMapIf.ts b/packages/router/src/operators/mergeMapIf.ts deleted file mode 100644 index f7cd95da29..0000000000 --- a/packages/router/src/operators/mergeMapIf.ts +++ /dev/null @@ -1,23 +0,0 @@ -/** - * @license - * Copyright Google Inc. All Rights Reserved. - * - * Use of this source code is governed by an MIT-style license that can be - * found in the LICENSE file at https://angular.io/license - */ - -import {EMPTY, MonoTypeOperatorFunction, Observable, of } from 'rxjs'; -import {mergeMap} from 'rxjs/operators'; - -export function mergeMapIf( - predicate: (value: T) => boolean, tap: (value: T) => any): MonoTypeOperatorFunction { - return (source: Observable) => { - return source.pipe(mergeMap(s => { - if (predicate(s)) { - tap(s); - return EMPTY; - } - return of (s); - })); - }; -} diff --git a/packages/router/src/operators/resolve_data.ts b/packages/router/src/operators/resolve_data.ts index e0d3501861..368297dffa 100644 --- a/packages/router/src/operators/resolve_data.ts +++ b/packages/router/src/operators/resolve_data.ts @@ -11,8 +11,8 @@ import {map, mergeMap} from 'rxjs/operators'; import {NavigationTransition} from '../router'; -export function resolveData( - paramsInheritanceStrategy: 'emptyOnly' | 'always'): MonoTypeOperatorFunction { +export function resolveData(paramsInheritanceStrategy: 'emptyOnly' | 'always'): + MonoTypeOperatorFunction { return function(source: Observable) { return source.pipe(mergeMap(t => { if (!t.preActivation) { diff --git a/packages/router/src/operators/throwIf.ts b/packages/router/src/operators/throwIf.ts deleted file mode 100644 index b77c341a50..0000000000 --- a/packages/router/src/operators/throwIf.ts +++ /dev/null @@ -1,26 +0,0 @@ -/** - * @license - * Copyright Google Inc. All Rights Reserved. - * - * Use of this source code is governed by an MIT-style license that can be - * found in the LICENSE file at https://angular.io/license - */ - -import {MonoTypeOperatorFunction, Observable} from 'rxjs'; -import {tap} from 'rxjs/operators'; - -export function throwIf( - predicate: (value: T) => boolean, - errorFactory: (() => any) = defaultErrorFactory): MonoTypeOperatorFunction { - return (source: Observable) => { - return source.pipe(tap(s => { - if (predicate(s)) { - throw errorFactory(); - } - })); - }; -} - -function defaultErrorFactory() { - return new Error(); -} \ No newline at end of file diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index fab1fc97ad..836aa337b7 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -19,7 +19,6 @@ import {afterPreactivation} from './operators/after_preactivation'; import {applyRedirects} from './operators/apply_redirects'; import {beforePreactivation} from './operators/before_preactivation'; import {checkGuards} from './operators/check_guards'; -import {mergeMapIf} from './operators/mergeMapIf'; import {recognize} from './operators/recognize'; import {resolveData} from './operators/resolve_data'; import {PreActivation} from './pre_activation'; @@ -30,7 +29,6 @@ import {ActivatedRoute, ActivatedRouteSnapshot, RouterState, RouterStateSnapshot import {Params, isNavigationCancelingError} from './shared'; import {DefaultUrlHandlingStrategy, UrlHandlingStrategy} from './url_handling_strategy'; import {UrlSerializer, UrlTree, containsTree, createEmptyUrlTree} from './url_tree'; -import {assertDefined} from './utils/assert'; import {forEach} from './utils/collection'; import {TreeNode, nodeChildrenAsMap} from './utils/tree'; @@ -370,179 +368,184 @@ export class Router { this.processNavigations(); } + // clang-format off private setupNavigations(transitions: Observable): Observable { return transitions.pipe( - filter(t => t.id !== 0), mergeMap(t => Promise.resolve(t)), - // Extract URL - map(t => ({ - ...t, // - extractedUrl: this.urlHandlingStrategy.extract(t.rawUrl), // - } as NavigationTransition)), + filter(t => t.id !== 0), mergeMap(t => Promise.resolve(t)), - // Using switchMap so we cancel executing navigations when a new one comes in - switchMap(t => { - let completed = false; - let errored = false; - return of (t).pipe( + // Extract URL + map(t => ({ + ...t, + extractedUrl: this.urlHandlingStrategy.extract(t.rawUrl)} as NavigationTransition)), + + // Using switchMap so we cancel executing navigations when a new one comes in + switchMap(t => { + let completed = false; + let errored = false; + return of (t).pipe(mergeMap(t => { + const urlTransition = !this.navigated || + t.extractedUrl.toString() !== this.currentUrlTree.toString(); + if ((this.onSameUrlNavigation === 'reload' ? true : urlTransition) && + this.urlHandlingStrategy.shouldProcessUrl(t.rawUrl)) { + return of (t).pipe( + // Update URL if in `eager` update mode + tap(t => this.urlUpdateStrategy === 'eager' && !t.extras.skipLocationChange && + this.setBrowserUrl(t.rawUrl, !!t.extras.replaceUrl, t.id)), + // Fire NavigationStart event mergeMap(t => { - const urlTransition = - !this.navigated || t.extractedUrl.toString() !== this.currentUrlTree.toString(); - if ((this.onSameUrlNavigation === 'reload' ? true : urlTransition) && - this.urlHandlingStrategy.shouldProcessUrl(t.rawUrl)) { - return of (t).pipe( - // Update URL if in `eager` update mode - tap(t => this.urlUpdateStrategy === 'eager' && !t.extras.skipLocationChange && - this.setBrowserUrl(t.rawUrl, !!t.extras.replaceUrl, t.id)), - // Fire NavigationStart event - mergeMap(t => { - const transition = this.transitions.getValue(); - (this.events as Subject) - .next(new NavigationStart( - t.id, this.serializeUrl(t.extractedUrl), t.source, t.state)); - if (transition !== this.transitions.getValue()) { - EMPTY; - } - return [t]; - }), - - // This delay is required to match old behavior that forced navigation to - // always be async - mergeMap(t => Promise.resolve(t)), - - // ApplyRedirects - applyRedirects( - this.ngModule.injector, this.configLoader, this.urlSerializer, - this.config), - // Recognize - recognize( - this.rootComponentType, this.config, (url) => this.serializeUrl(url), - this.paramsInheritanceStrategy), - // Throw if there's no snapshot - tap(t => assertDefined(t.targetSnapshot, 'snapshot must be defined')), - // Fire RoutesRecognized - tap(t => (this.events as Subject) - .next(new RoutesRecognized( - t.id, this.serializeUrl(t.extractedUrl), - this.serializeUrl(t.urlAfterRedirects), - t.targetSnapshot !))), ); - } else if ( - urlTransition && this.rawUrlTree && - this.urlHandlingStrategy.shouldProcessUrl(this.rawUrlTree)) { - (this.events as Subject) - .next(new NavigationStart( - t.id, this.serializeUrl(t.extractedUrl), t.source, t.state)); - - return of ({ - ...t, - urlAfterRedirects: t.extractedUrl, - extras: {...t.extras, skipLocationChange: false, replaceUrl: false}, - targetSnapshot: - createEmptyState(t.extractedUrl, this.rootComponentType).snapshot - }); - } else { - this.rawUrlTree = t.rawUrl; - t.resolve(null); + const transition = this.transitions.getValue(); + (this.events as Subject).next(new NavigationStart( + t.id, this.serializeUrl(t.extractedUrl), t.source, t.state)); + if (transition !== this.transitions.getValue()) { return EMPTY; } + return [t]; }), - // Before Preactivation - beforePreactivation(this.hooks.beforePreactivation), - // --- GUARDS --- - tap(t => this.triggerEvent(new GuardsCheckStart( - t.id, this.serializeUrl(t.extractedUrl), - this.serializeUrl(t.urlAfterRedirects), t.targetSnapshot !))), - map(t => { - const preActivation = new PreActivation( - t.targetSnapshot !, t.currentSnapshot, this.ngModule.injector, - (evt: Event) => this.triggerEvent(evt)); - preActivation.initialize(this.rootContexts); - return {...t, preActivation}; - }), - checkGuards(), tap(t => this.triggerEvent(new GuardsCheckEnd( - t.id, this.serializeUrl(t.extractedUrl), - this.serializeUrl(t.urlAfterRedirects), t.targetSnapshot !, - !!t.guardsResult))), + // This delay is required to match old behavior that forced navigation to + // always be async + mergeMap(t => Promise.resolve(t)), - mergeMapIf( - t => !t.guardsResult, - t => { - this.resetUrlToCurrentUrlTree(); - (this.events as Subject) - .next(new NavigationCancel(t.id, this.serializeUrl(t.extractedUrl), '')); - t.resolve(false); - }), + // ApplyRedirects + applyRedirects( + this.ngModule.injector, this.configLoader, this.urlSerializer,this.config), + // Recognize + recognize( + this.rootComponentType, this.config, (url) => this.serializeUrl(url), + this.paramsInheritanceStrategy), - // --- RESOLVE --- - mergeMap(t => { - if (t.preActivation !.isActivating()) { - return of (t).pipe( - tap(t => this.triggerEvent(new ResolveStart( - t.id, this.serializeUrl(t.extractedUrl), - this.serializeUrl(t.urlAfterRedirects), t.targetSnapshot !))), - resolveData(this.paramsInheritanceStrategy), - tap(t => this.triggerEvent(new ResolveEnd( - t.id, this.serializeUrl(t.extractedUrl), - this.serializeUrl(t.urlAfterRedirects), t.targetSnapshot !))), ); - } - return of (t); - }), + // Fire RoutesRecognized + tap(t => (this.events as Subject).next(new RoutesRecognized( + t.id, this.serializeUrl(t.extractedUrl), this.serializeUrl(t.urlAfterRedirects), + t.targetSnapshot !))) + ); + } else if ( + urlTransition && this.rawUrlTree && + this.urlHandlingStrategy.shouldProcessUrl(this.rawUrlTree)) { + (this.events as Subject).next(new NavigationStart( + t.id, this.serializeUrl(t.extractedUrl), t.source, t.state)); - // --- AFTER PREACTIVATION --- - afterPreactivation(this.hooks.afterPreactivation), map(t => { - const targetRouterState = createRouterState( - this.routeReuseStrategy, t.targetSnapshot !, t.currentRouterState); - return ({...t, targetRouterState}); - }), + return of ({ + ...t, + urlAfterRedirects: t.extractedUrl, + extras: {...t.extras, skipLocationChange: false, replaceUrl: false}, + targetSnapshot: createEmptyState(t.extractedUrl, this.rootComponentType).snapshot + }); + } else { + this.rawUrlTree = t.rawUrl; + t.resolve(null); + return EMPTY; + } + }), - // Side effects of resetting Router instance - tap(t => { - this.currentUrlTree = t.urlAfterRedirects; - this.rawUrlTree = this.urlHandlingStrategy.merge(this.currentUrlTree, t.rawUrl); + // Before Preactivation + beforePreactivation(this.hooks.beforePreactivation), - (this as{routerState: RouterState}).routerState = t.targetRouterState !; + // --- GUARDS --- + tap(t => this.triggerEvent(new GuardsCheckStart( + t.id, this.serializeUrl(t.extractedUrl), + this.serializeUrl(t.urlAfterRedirects), t.targetSnapshot !))), + map(t => { + const preActivation = new PreActivation( + t.targetSnapshot !, t.currentSnapshot, this.ngModule.injector, + (evt: Event) => this.triggerEvent(evt)); + preActivation.initialize(this.rootContexts); + return {...t, preActivation}; + }), - if (this.urlUpdateStrategy === 'deferred' && !t.extras.skipLocationChange) { - this.setBrowserUrl(this.rawUrlTree, !!t.extras.replaceUrl, t.id); - } - }), + checkGuards(), - activate( - this.rootContexts, this.routeReuseStrategy, - (evt: Event) => this.triggerEvent(evt)), + tap(t => this.triggerEvent(new GuardsCheckEnd( + t.id, this.serializeUrl(t.extractedUrl), + this.serializeUrl(t.urlAfterRedirects), t.targetSnapshot !, + !!t.guardsResult))), - tap({next: () => completed = true, complete: () => completed = true}), - finalize(() => { - if (!completed && !errored) { - (this.events as Subject) - .next(new NavigationCancel( - t.id, this.serializeUrl(t.extractedUrl), - `Navigation ID ${t.id} is not equal to the current navigation id ${this.navigationId}`)); - t.resolve(false); - } - }), - catchError((e) => { - errored = true; - if (isNavigationCancelingError(e)) { - this.navigated = true; - this.resetStateAndUrl(t.currentRouterState, t.currentUrlTree, t.rawUrl); - (this.events as Subject) - .next( - new NavigationCancel(t.id, this.serializeUrl(t.extractedUrl), e.message)); - } else { - this.resetStateAndUrl(t.currentRouterState, t.currentUrlTree, t.rawUrl); - (this.events as Subject) - .next(new NavigationError(t.id, this.serializeUrl(t.extractedUrl), e)); - try { - t.resolve(this.errorHandler(e)); - } catch (ee) { - t.reject(ee); - } - } - return EMPTY; - }), ); - })) as any as Observable; + mergeMap(t => { + if (!t.guardsResult) { + this.resetUrlToCurrentUrlTree(); + (this.events as Subject) + .next(new NavigationCancel(t.id, this.serializeUrl(t.extractedUrl), '')); + t.resolve(false); + return EMPTY; + } + return of(t); + }), + + // --- RESOLVE --- + mergeMap(t => { + if (t.preActivation !.isActivating()) { + return of (t).pipe( + tap(t => this.triggerEvent(new ResolveStart( + t.id, this.serializeUrl(t.extractedUrl), + this.serializeUrl(t.urlAfterRedirects), t.targetSnapshot !))), + resolveData(this.paramsInheritanceStrategy), + tap(t => this.triggerEvent(new ResolveEnd( + t.id, this.serializeUrl(t.extractedUrl), + this.serializeUrl(t.urlAfterRedirects), t.targetSnapshot !))), ); + } + return of (t); + }), + + // --- AFTER PREACTIVATION --- + afterPreactivation(this.hooks.afterPreactivation), map(t => { + const targetRouterState = createRouterState( + this.routeReuseStrategy, t.targetSnapshot !, t.currentRouterState); + return ({...t, targetRouterState}); + }), + + // Side effects of resetting Router instance + afterPreactivation(this.hooks.afterPreactivation), map(t => { + const targetRouterState = createRouterState( + this.routeReuseStrategy, t.targetSnapshot !, t.currentRouterState); + return ({...t, targetRouterState}); + }), + + // Side effects of resetting Router instance + tap(t => { + this.currentUrlTree = t.urlAfterRedirects; + this.rawUrlTree = this.urlHandlingStrategy.merge(this.currentUrlTree, t.rawUrl); + + (this as{routerState: RouterState}).routerState = t.targetRouterState !; + + if (this.urlUpdateStrategy === 'deferred' && !t.extras.skipLocationChange) { + this.setBrowserUrl(this.rawUrlTree, !!t.extras.replaceUrl, t.id); + } + }), + + activate( + this.rootContexts, this.routeReuseStrategy, + (evt: Event) => this.triggerEvent(evt)), + + tap({next: () => completed = true, complete: () => completed = true}), + finalize(() => { + if (!completed && !errored) { + (this.events as Subject).next(new NavigationCancel( + t.id, this.serializeUrl(t.extractedUrl), + `Navigation ID ${t.id} is not equal to the current navigation id ${this.navigationId}`)); + t.resolve(false); + } + }), + catchError((e) => { + errored = true; + if (isNavigationCancelingError(e)) { + this.navigated = true; + this.resetStateAndUrl(t.currentRouterState, t.currentUrlTree, t.rawUrl); + (this.events as Subject).next( + new NavigationCancel(t.id, this.serializeUrl(t.extractedUrl), e.message)); + } else { + this.resetStateAndUrl(t.currentRouterState, t.currentUrlTree, t.rawUrl); + (this.events as Subject).next(new NavigationError( + t.id, this.serializeUrl(t.extractedUrl), e)); + try { + t.resolve(this.errorHandler(e)); + } catch (ee) { + t.reject(ee); + } + } + return EMPTY; + }), ); + })) as any as Observable; function activate( rootContexts: ChildrenOutletContexts, routeReuseStrategy: RouteReuseStrategy, @@ -557,6 +560,7 @@ export class Router { }; } } + // clang-format on /** * @internal diff --git a/packages/router/src/utils/assert.ts b/packages/router/src/utils/assert.ts deleted file mode 100644 index 38ad8b0a6d..0000000000 --- a/packages/router/src/utils/assert.ts +++ /dev/null @@ -1,19 +0,0 @@ -/** - * @license - * Copyright Google Inc. All Rights Reserved. - * - * Use of this source code is governed by an MIT-style license that can be - * found in the LICENSE file at https://angular.io/license - */ - -export function assertDefined(actual: T, msg: string) { - if (actual == null) { - throwError(msg); - } -} - -function throwError(msg: string): never { - // tslint:disable-next-line - debugger; // Left intentionally for better debugger experience. - throw new Error(`ASSERTION ERROR: ${msg}`); -} diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts index c2029b2030..d4bb090b0d 100644 --- a/packages/router/test/integration.spec.ts +++ b/packages/router/test/integration.spec.ts @@ -291,49 +291,35 @@ describe('Integration', () => { }); - // TODO(jasonaden): This test now fails because it relies on waiting on a guard to finish - // executing even after a new navigation has been scheduled. The previous implementation - // would do so, but ignore the result of any guards that are executing when a new navigation - // is scheduled. + it('should not wait for prior navigations to start a new navigation', + fakeAsync(inject([Router, Location], (router: Router) => { + const fixture = createRoot(router, RootCmp); - // With new implementation, the current navigation will be unrolled and cleaned up so the - // new navigation can start immediately. This test therefore fails as it relies on that - // previous incorrect behavior. - xit('should execute navigations serialy', - fakeAsync(inject([Router, Location], (router: Router) => { - const fixture = createRoot(router, RootCmp); + router.resetConfig([ + {path: 'a', component: SimpleCmp, canActivate: ['trueRightAway', 'trueIn2Seconds']}, + {path: 'b', component: SimpleCmp, canActivate: ['trueRightAway', 'trueIn2Seconds']} + ]); - 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('/a'); - tick(100); - fixture.detectChanges(); + router.navigateByUrl('/b'); + tick(100); // 200 + fixture.detectChanges(); - router.navigateByUrl('/b'); - tick(100); // 200 - fixture.detectChanges(); + expect(log).toEqual( + ['trueRightAway', 'trueIn2Seconds-start', 'trueRightAway', 'trueIn2Seconds-start']); - expect(log).toEqual(['trueRightAway', 'trueIn2Seconds-start']); + tick(2000); // 2200 + fixture.detectChanges(); - tick(2000); // 2200 - fixture.detectChanges(); + expect(log).toEqual([ + 'trueRightAway', 'trueIn2Seconds-start', 'trueRightAway', 'trueIn2Seconds-start', + 'trueIn2Seconds-end', 'trueIn2Seconds-end' + ]); - 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 inside ChangeDetectionStrategy.OnPush components', fakeAsync(() => { @@ -2971,32 +2957,31 @@ describe('Integration', () => { ]); }))); - it('should allow redirection in NavigationStart', fakeAsync(inject([Router], (router: Router) => { - const fixture = createRoot(router, RootCmp); + it('should allow redirection in NavigationStart', + fakeAsync(inject([Router], (router: Router) => { + const fixture = createRoot(router, RootCmp); - router.resetConfig([ - {path: 'blank', component: UserCmp}, - {path: 'user/:name', component: BlankCmp}, - ]); + router.resetConfig([ + {path: 'blank', component: UserCmp}, + {path: 'user/:name', component: BlankCmp}, + ]); - const navigateSpy = spyOn(router, 'navigate').and.callThrough(); - const recordedEvents: any[] = []; + const navigateSpy = spyOn(router, 'navigate').and.callThrough(); + const recordedEvents: any[] = []; - const navStart$ = router.events.pipe( - tap(e => recordedEvents.push(e)), - filter(e => e instanceof NavigationStart), - first() - ); + const navStart$ = router.events.pipe( + tap(e => recordedEvents.push(e)), filter(e => e instanceof NavigationStart), first()); - navStart$.subscribe((e: NavigationStart | NavigationError) => { - router.navigate(['/blank'], {queryParams: {state: 'redirected'}, queryParamsHandling: 'merge'}); - advance(fixture); - }); + navStart$.subscribe((e: NavigationStart | NavigationError) => { + router.navigate( + ['/blank'], {queryParams: {state: 'redirected'}, queryParamsHandling: 'merge'}); + advance(fixture); + }); - router.navigate(['/user/:fedor']); - advance(fixture); + router.navigate(['/user/:fedor']); + advance(fixture); - expect(navigateSpy.calls.mostRecent().args[1].queryParams); + expect(navigateSpy.calls.mostRecent().args[1].queryParams); }))); });