From d9c4840a9c53d5e41d95772c57c1101e8ac1a54a Mon Sep 17 00:00:00 2001 From: Martin Sikora Date: Fri, 17 Apr 2020 11:48:19 +0200 Subject: [PATCH] fix(router): cancel navigation when at least one resolver completes with no "next" emission (#24621) This change aligns behavior for resolvers which return EMPTY. Currently EMPTY resolvers have inconsistent behavior: - One resolver that returns EMPTY => won't navigate and just ends on ResolveStart router event. - Two resolvers where both return EMPTY => throws "Error: Uncaught (in promise): EmptyError: no elements in sequence" - Two resolvers where one returns a value and the other one returns EMPTY => Navigates successfully. With this change any EMPTY resolver will cancel navigation. BREAKING CHANGE: Any resolver which return EMPTY will cancel navigation. If you want to allow the navigation to continue, you will need to update the resolvers to emit some value, (i.e. defaultIfEmpty(...), of(...), etc). PR Close #24195 PR Close #24621 --- aio/content/guide/router.md | 4 +- packages/router/src/operators/resolve_data.ts | 44 +++--- packages/router/src/router.ts | 22 ++- packages/router/test/integration.spec.ts | 149 +++++++++++++++++- .../test/operators/resolve_data.spec.ts | 102 ++++++++++++ 5 files changed, 296 insertions(+), 25 deletions(-) create mode 100644 packages/router/test/operators/resolve_data.spec.ts diff --git a/aio/content/guide/router.md b/aio/content/guide/router.md index 7ec04284d1..3760fe0457 100644 --- a/aio/content/guide/router.md +++ b/aio/content/guide/router.md @@ -2956,7 +2956,7 @@ Update the `CrisisDetailComponent` to get the crisis from the `ActivatedRoute.d -Note the following two important points: +Note the following three important points: 1. The router's `Resolve` interface is optional. The `CrisisDetailResolverService` doesn't inherit from a base class. @@ -2964,6 +2964,8 @@ The router looks for that method and calls it if found. 1. The router calls the resolver in any case where the the user could navigate away so you don't have to code for each use case. +1. Returning an empty `Observable` in at least one resolver will cancel navigation. + The relevant Crisis Center code for this milestone follows. diff --git a/packages/router/src/operators/resolve_data.ts b/packages/router/src/operators/resolve_data.ts index 956a21f3b8..6965d9e1cc 100644 --- a/packages/router/src/operators/resolve_data.ts +++ b/packages/router/src/operators/resolve_data.ts @@ -7,8 +7,8 @@ */ import {Injector} from '@angular/core'; -import {from, MonoTypeOperatorFunction, Observable, of} from 'rxjs'; -import {concatMap, last, map, mergeMap, reduce} from 'rxjs/operators'; +import {EMPTY, from, MonoTypeOperatorFunction, Observable, of} from 'rxjs'; +import {concatMap, map, mergeMap, takeLast, tap} from 'rxjs/operators'; import {ResolveData} from '../config'; import {NavigationTransition} from '../router'; @@ -26,13 +26,16 @@ export function resolveData( if (!canActivateChecks.length) { return of(t); } - + let canActivateChecksResolved = 0; return from(canActivateChecks) .pipe( concatMap( check => runResolve( check.route, targetSnapshot!, paramsInheritanceStrategy, moduleInjector)), - reduce((_: any, __: any) => _), map(_ => t)); + tap(() => canActivateChecksResolved++), + takeLast(1), + mergeMap(_ => canActivateChecksResolved === canActivateChecks.length ? of(t) : EMPTY), + ); })); }; } @@ -59,22 +62,23 @@ function resolveNode( if (keys.length === 0) { return of({}); } - if (keys.length === 1) { - const key = keys[0]; - return getResolver(resolve[key], futureARS, futureRSS, moduleInjector) - .pipe(map((value: any) => { - return {[key]: value}; - })); - } const data: {[k: string]: any} = {}; - const runningResolvers$ = from(keys).pipe(mergeMap((key: string) => { - return getResolver(resolve[key], futureARS, futureRSS, moduleInjector) - .pipe(map((value: any) => { - data[key] = value; - return value; - })); - })); - return runningResolvers$.pipe(last(), map(() => data)); + return from(keys).pipe( + mergeMap( + (key: string) => getResolver(resolve[key], futureARS, futureRSS, moduleInjector) + .pipe(tap((value: any) => { + data[key] = value; + }))), + takeLast(1), + mergeMap(() => { + // Ensure all resolvers returned values, otherwise don't emit any "next" and just complete + // the chain which will cancel navigation + if (Object.keys(data).length === keys.length) { + return of(data); + } + return EMPTY; + }), + ); } function getResolver( @@ -83,4 +87,4 @@ function getResolver( const resolver = getToken(injectionToken, futureARS, moduleInjector); return resolver.resolve ? wrapIntoObservable(resolver.resolve(futureARS, futureRSS)) : wrapIntoObservable(resolver(futureARS, futureRSS)); -} \ No newline at end of file +} diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index 0100c0e126..91602c7b06 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -639,9 +639,25 @@ export class Router { this.serializeUrl(t.urlAfterRedirects), t.targetSnapshot!); this.triggerEvent(resolveStart); }), - resolveData( - this.paramsInheritanceStrategy, - this.ngModule.injector), // + switchMap(t => { + let dataResolved = false; + return of(t).pipe( + resolveData( + this.paramsInheritanceStrategy, this.ngModule.injector), + tap({ + next: () => dataResolved = true, + complete: () => { + if (!dataResolved) { + const navCancel = new NavigationCancel( + t.id, this.serializeUrl(t.extractedUrl), + `At least one route resolver didn't emit any value.`); + eventsSubject.next(navCancel); + t.resolve(false); + } + } + }), + ); + }), tap(t => { const resolveEnd = new ResolveEnd( t.id, this.serializeUrl(t.extractedUrl), diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts index 4405614281..1098c0c4d9 100644 --- a/packages/router/test/integration.spec.ts +++ b/packages/router/test/integration.spec.ts @@ -13,7 +13,7 @@ import {ComponentFixture, fakeAsync, inject, TestBed, tick} from '@angular/core/ import {By} from '@angular/platform-browser/src/dom/debug/by'; import {expect} from '@angular/platform-browser/testing/src/matchers'; import {ActivatedRoute, ActivatedRouteSnapshot, ActivationEnd, ActivationStart, CanActivate, CanDeactivate, ChildActivationEnd, ChildActivationStart, DefaultUrlSerializer, DetachedRouteHandle, Event, GuardsCheckEnd, GuardsCheckStart, Navigation, NavigationCancel, NavigationEnd, NavigationError, NavigationStart, ParamMap, Params, PreloadAllModules, PreloadingStrategy, PRIMARY_OUTLET, Resolve, ResolveEnd, ResolveStart, RouteConfigLoadEnd, RouteConfigLoadStart, Router, RouteReuseStrategy, RouterEvent, RouterModule, RouterPreloader, RouterStateSnapshot, RoutesRecognized, RunGuardsAndResolvers, UrlHandlingStrategy, UrlSegmentGroup, UrlSerializer, UrlTree} from '@angular/router'; -import {Observable, Observer, of, Subscription} from 'rxjs'; +import {EMPTY, Observable, Observer, of, Subscription} from 'rxjs'; import {filter, first, map, tap} from 'rxjs/operators'; import {forEach} from '../src/utils/collection'; @@ -1564,6 +1564,7 @@ describe('Integration', () => { {provide: 'resolveSix', useClass: ResolveSix}, {provide: 'resolveError', useValue: (a: any, b: any) => Promise.reject('error')}, {provide: 'resolveNullError', useValue: (a: any, b: any) => Promise.reject(null)}, + {provide: 'resolveEmpty', useValue: (a: any, b: any) => EMPTY}, {provide: 'numberOfUrlSegments', useValue: (a: any, b: any) => a.url.length}, ] }); @@ -1647,6 +1648,152 @@ describe('Integration', () => { expect(e).toEqual(null); }))); + it('should not navigate when all resolvers return empty result', + fakeAsync(inject([Router], (router: Router) => { + const fixture = createRoot(router, RootCmp); + + router.resetConfig([ + {path: 'simple', component: SimpleCmp, resolve: {e1: 'resolveEmpty', e2: 'resolveEmpty'}} + ]); + + const recordedEvents: any[] = []; + router.events.subscribe(e => e instanceof RouterEvent && recordedEvents.push(e)); + + let e: any = null; + router.navigateByUrl('/simple').catch(error => e = error); + advance(fixture); + + expectEvents(recordedEvents, [ + [NavigationStart, '/simple'], + [RoutesRecognized, '/simple'], + [GuardsCheckStart, '/simple'], + [GuardsCheckEnd, '/simple'], + [ResolveStart, '/simple'], + [NavigationCancel, '/simple'], + ]); + + expect(e).toEqual(null); + }))); + + it('should not navigate when at least one resolver returns empty result', + fakeAsync(inject([Router], (router: Router) => { + const fixture = createRoot(router, RootCmp); + + router.resetConfig([ + {path: 'simple', component: SimpleCmp, resolve: {e1: 'resolveTwo', e2: 'resolveEmpty'}} + ]); + + const recordedEvents: any[] = []; + router.events.subscribe(e => e instanceof RouterEvent && recordedEvents.push(e)); + + let e: any = null; + router.navigateByUrl('/simple').catch(error => e = error); + advance(fixture); + + expectEvents(recordedEvents, [ + [NavigationStart, '/simple'], + [RoutesRecognized, '/simple'], + [GuardsCheckStart, '/simple'], + [GuardsCheckEnd, '/simple'], + [ResolveStart, '/simple'], + [NavigationCancel, '/simple'], + ]); + + expect(e).toEqual(null); + }))); + + it('should not navigate when all resolvers for a child route from forChild() returns empty result', + fakeAsync(inject( + [Router, Location, NgModuleFactoryLoader], + (router: Router, location: Location, loader: SpyNgModuleFactoryLoader) => { + const fixture = createRoot(router, RootCmp); + + @Component({selector: 'lazy-cmp', template: 'lazy-loaded-1'}) + class LazyComponent1 { + } + + router.resetConfig([{path: 'lazy', loadChildren: 'expected1'}]); + + @NgModule({ + declarations: [LazyComponent1], + imports: [ + RouterModule.forChild([{ + path: 'loaded', + component: LazyComponent1, + resolve: {e1: 'resolveEmpty', e2: 'resolveEmpty'} + }]), + ], + }) + class LoadedModule { + } + + loader.stubbedModules = {expected1: LoadedModule}; + + const recordedEvents: any[] = []; + router.events.subscribe(e => e instanceof RouterEvent && recordedEvents.push(e)); + + let e: any = null; + router.navigateByUrl('lazy/loaded').catch(error => e = error); + advance(fixture); + + expectEvents(recordedEvents, [ + [NavigationStart, '/lazy/loaded'], + [RoutesRecognized, '/lazy/loaded'], + [GuardsCheckStart, '/lazy/loaded'], + [GuardsCheckEnd, '/lazy/loaded'], + [ResolveStart, '/lazy/loaded'], + [NavigationCancel, '/lazy/loaded'], + ]); + + expect(e).toEqual(null); + }))); + + it('should not navigate when at least one resolver for a child route from forChild() returns empty result', + fakeAsync(inject( + [Router, Location, NgModuleFactoryLoader], + (router: Router, location: Location, loader: SpyNgModuleFactoryLoader) => { + const fixture = createRoot(router, RootCmp); + + @Component({selector: 'lazy-cmp', template: 'lazy-loaded-1'}) + class LazyComponent1 { + } + + router.resetConfig([{path: 'lazy', loadChildren: 'expected1'}]); + + @NgModule({ + declarations: [LazyComponent1], + imports: [ + RouterModule.forChild([{ + path: 'loaded', + component: LazyComponent1, + resolve: {e1: 'resolveTwo', e2: 'resolveEmpty'} + }]), + ], + }) + class LoadedModule { + } + + loader.stubbedModules = {expected1: LoadedModule}; + + const recordedEvents: any[] = []; + router.events.subscribe(e => e instanceof RouterEvent && recordedEvents.push(e)); + + let e: any = null; + router.navigateByUrl('lazy/loaded').catch(error => e = error); + advance(fixture); + + expectEvents(recordedEvents, [ + [NavigationStart, '/lazy/loaded'], + [RoutesRecognized, '/lazy/loaded'], + [GuardsCheckStart, '/lazy/loaded'], + [GuardsCheckEnd, '/lazy/loaded'], + [ResolveStart, '/lazy/loaded'], + [NavigationCancel, '/lazy/loaded'], + ]); + + expect(e).toEqual(null); + }))); + it('should preserve resolved data', fakeAsync(inject([Router], (router: Router) => { const fixture = createRoot(router, RootCmp); diff --git a/packages/router/test/operators/resolve_data.spec.ts b/packages/router/test/operators/resolve_data.spec.ts new file mode 100644 index 0000000000..fc27632c02 --- /dev/null +++ b/packages/router/test/operators/resolve_data.spec.ts @@ -0,0 +1,102 @@ +/** + * @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 {Injector} from '@angular/core'; +import {TestBed} from '@angular/core/testing'; +import {EMPTY, of} from 'rxjs'; +import {TestScheduler} from 'rxjs/testing'; + +import {resolveData} from '../../src/operators/resolve_data'; + +describe('resolveData operator', () => { + let testScheduler: TestScheduler; + let injector: Injector; + + beforeEach(() => { + TestBed.configureTestingModule({ + providers: [ + {provide: 'resolveTwo', useValue: (a: any, b: any) => of(2)}, + {provide: 'resolveFour', useValue: (a: any, b: any) => 4}, + {provide: 'resolveEmpty', useValue: (a: any, b: any) => EMPTY}, + ] + }); + }); + beforeEach(() => { + testScheduler = new TestScheduler(assertDeepEquals); + }); + beforeEach(() => { + injector = TestBed.inject(Injector); + }); + + it('should re-emit updated value from source after all resolvers emit and complete', () => { + testScheduler.run(({hot, cold, expectObservable}) => { + const transition: any = createTransition({e1: 'resolveTwo'}, {e2: 'resolveFour'}); + const source = cold('-(t|)', {t: deepClone(transition)}); + const expected = '-(t|)'; + const outputTransition = deepClone(transition); + outputTransition.guards.canActivateChecks[0].route._resolvedData = {e1: 2}; + outputTransition.guards.canActivateChecks[1].route._resolvedData = {e2: 4}; + + expectObservable(source.pipe(resolveData('emptyOnly', injector))).toBe(expected, { + t: outputTransition + }); + }); + }); + + it('should re-emit value from source when there are no resolvers', () => { + testScheduler.run(({hot, cold, expectObservable}) => { + const transition: any = createTransition({}); + const source = cold('-(t|)', {t: deepClone(transition)}); + const expected = '-(t|)'; + const outputTransition = deepClone(transition); + outputTransition.guards.canActivateChecks[0].route._resolvedData = {}; + + expectObservable(source.pipe(resolveData('emptyOnly', injector))).toBe(expected, { + t: outputTransition + }); + }); + }); + + it('should not emit when there\'s one resolver that doesn\'t emit', () => { + testScheduler.run(({hot, cold, expectObservable}) => { + const transition: any = createTransition({e2: 'resolveEmpty'}); + const source = cold('-(t|)', {t: deepClone(transition)}); + const expected = '-|'; + expectObservable(source.pipe(resolveData('emptyOnly', injector))).toBe(expected); + }); + }); + + it('should not emit if at least one resolver doesn\'t emit', () => { + testScheduler.run(({hot, cold, expectObservable}) => { + const transition: any = createTransition({e1: 'resolveTwo'}, {e2: 'resolveEmpty'}); + const source = cold('-(t|)', {t: deepClone(transition)}); + const expected = '-|'; + expectObservable(source.pipe(resolveData('emptyOnly', injector))).toBe(expected); + }); + }); +}); + +function assertDeepEquals(a: any, b: any) { + return expect(a).toEqual(b); +} + +function createTransition(...resolvers: {[key: string]: string}[]) { + return { + targetSnapshot: {}, + guards: { + canActivateChecks: + resolvers.map(resolver => ({ + route: {_resolve: resolver, pathFromRoot: [{url: '/'}], data: {}}, + })), + }, + }; +} + +function deepClone(obj: any) { + return JSON.parse(JSON.stringify(obj)); +}