From f368381d1282f56631569b0d67dd623544db8128 Mon Sep 17 00:00:00 2001 From: Dzmitry Shylovich Date: Fri, 24 Mar 2017 21:32:17 +0300 Subject: [PATCH] fix(router): should run CanActivate after CanDeactivate guards Closes #14059 Closes #15467 --- packages/router/src/router.ts | 66 +++++++++++++----------- packages/router/test/integration.spec.ts | 50 +++++++++++++++++- 2 files changed, 84 insertions(+), 32 deletions(-) diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index b1fad97f09..59b548b099 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -773,7 +773,8 @@ class CanDeactivate { export class PreActivation { - private checks: Array = []; + private canActivateChecks: CanActivate[] = []; + private canDeactivateChecks: CanDeactivate[] = []; constructor( private future: RouterStateSnapshot, private curr: RouterStateSnapshot, private moduleInjector: Injector) {} @@ -785,33 +786,20 @@ export class PreActivation { } checkGuards(): Observable { - if (this.checks.length === 0) return of (true); - const checks$ = from(this.checks); - const runningChecks$ = mergeMap.call(checks$, (s: any) => { - if (s instanceof CanActivate) { - return andObservables( - from([this.runCanActivateChild(s.path), this.runCanActivate(s.route)])); - } else if (s instanceof CanDeactivate) { - // workaround https://github.com/Microsoft/TypeScript/issues/7271 - const s2 = s as CanDeactivate; - return this.runCanDeactivate(s2.component, s2.route); - } else { - throw new Error('Cannot be reached'); - } - }); - return every.call(runningChecks$, (result: any) => result === true); + if (this.canDeactivateChecks.length === 0 && this.canActivateChecks.length === 0) { + return of (true); + } + const canDeactivate$ = this.runCanDeactivateChecks(); + return mergeMap.call( + canDeactivate$, + (canDeactivate: boolean) => canDeactivate ? this.runCanActivateChecks() : of (false)); } resolveData(): Observable { - if (this.checks.length === 0) return of (null); - const checks$ = from(this.checks); - const runningChecks$ = concatMap.call(checks$, (s: any) => { - if (s instanceof CanActivate) { - return this.runResolve(s.route); - } else { - return of (null); - } - }); + if (this.canActivateChecks.length === 0) return of (null); + const checks$ = from(this.canActivateChecks); + const runningChecks$ = + concatMap.call(checks$, (check: CanActivate) => this.runResolve(check.route)); return reduce.call(runningChecks$, (_: any, __: any) => _); } @@ -829,7 +817,7 @@ export class PreActivation { (v: any, k: string) => this.deactiveRouteAndItsChildren(v, outletMap._outlets[k])); } - traverseRoutes( + private traverseRoutes( futureNode: TreeNode, currNode: TreeNode, parentOutletMap: RouterOutletMap, futurePath: ActivatedRouteSnapshot[]): void { const future = futureNode.value; @@ -840,7 +828,8 @@ export class PreActivation { if (curr && future._routeConfig === curr._routeConfig) { if (this.shouldRunGuardsAndResolvers( curr, future, future._routeConfig.runGuardsAndResolvers)) { - this.checks.push(new CanDeactivate(outlet.component, curr), new CanActivate(futurePath)); + this.canActivateChecks.push(new CanActivate(futurePath)); + this.canDeactivateChecks.push(new CanDeactivate(outlet.component, curr)); } else { // we need to set the data future.data = curr.data; @@ -861,7 +850,7 @@ export class PreActivation { this.deactiveRouteAndItsChildren(currNode, outlet); } - this.checks.push(new CanActivate(futurePath)); + this.canActivateChecks.push(new CanActivate(futurePath)); // If we have a component, we need to go through an outlet. if (future.component) { this.traverseChildRoutes(futureNode, null, outlet ? outlet.outletMap : null, futurePath); @@ -906,14 +895,29 @@ export class PreActivation { }); if (!r.component) { - this.checks.push(new CanDeactivate(null, r)); + this.canDeactivateChecks.push(new CanDeactivate(null, r)); } else if (outlet && outlet.isActivated) { - this.checks.push(new CanDeactivate(outlet.component, r)); + this.canDeactivateChecks.push(new CanDeactivate(outlet.component, r)); } else { - this.checks.push(new CanDeactivate(null, r)); + this.canDeactivateChecks.push(new CanDeactivate(null, r)); } } + private runCanDeactivateChecks(): Observable { + const checks$ = from(this.canDeactivateChecks); + const runningChecks$ = mergeMap.call( + checks$, (check: CanDeactivate) => this.runCanDeactivate(check.component, check.route)); + return every.call(runningChecks$, (result: boolean) => result === true); + } + + private runCanActivateChecks(): Observable { + const checks$ = from(this.canActivateChecks); + const runningChecks$ = mergeMap.call( + checks$, (check: CanActivate) => andObservables(from( + [this.runCanActivateChild(check.path), this.runCanActivate(check.route)]))); + return every.call(runningChecks$, (result: boolean) => result === true); + } + private runCanActivate(future: ActivatedRouteSnapshot): Observable { const canActivate = future._routeConfig ? future._routeConfig.canActivate : null; if (!canActivate || canActivate.length === 0) return of (true); diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts index 1de8f250a6..c62a56ef55 100644 --- a/packages/router/test/integration.spec.ts +++ b/packages/router/test/integration.spec.ts @@ -1703,7 +1703,24 @@ describe('Integration', () => { log.push('called'); return false; } - } + }, + { + provide: 'alwaysFalseWithDelayAndLogging', + useValue: () => { + log.push('called'); + let resolve: (result: boolean) => void; + const promise = new Promise(res => resolve = res); + setTimeout(() => resolve(false), 0); + return promise; + } + }, + { + provide: 'canActivate_alwaysTrueAndLogging', + useValue: () => { + log.push('canActivate called'); + return true; + } + }, ] }); }); @@ -1855,6 +1872,37 @@ describe('Integration', () => { expect(location.path()).toEqual('/main/component1'); }))); + it('should not run CanActivate when CanDeactivate returns false', + fakeAsync(inject([Router, Location], (router: Router, location: Location) => { + const fixture = createRoot(router, RootCmp); + + router.resetConfig([{ + path: 'main', + component: TeamCmp, + children: [ + { + path: 'component1', + component: SimpleCmp, + canDeactivate: ['alwaysFalseWithDelayAndLogging'] + }, + { + path: 'component2', + component: SimpleCmp, + canActivate: ['canActivate_alwaysTrueAndLogging'] + }, + ] + }]); + + router.navigateByUrl('/main/component1'); + advance(fixture); + expect(location.path()).toEqual('/main/component1'); + + router.navigateByUrl('/main/component2'); + advance(fixture); + expect(location.path()).toEqual('/main/component1'); + expect(log).toEqual(['called']); + }))); + it('should call guards every time when navigating to the same url over and over again', fakeAsync(inject([Router, Location], (router: Router, location: Location) => { const fixture = createRoot(router, RootCmp);