From 8070323437a8755f323ae2f0d861d490ae8e2f15 Mon Sep 17 00:00:00 2001 From: dummdidumm <5968653+dummdidumm@users.noreply.github.com> Date: Sat, 25 Aug 2018 15:02:16 +0200 Subject: [PATCH 1/2] fix(router-store): Handle navigation error, cancel/error action contain real previous state Fixes #1293, #1292 --- modules/router-store/spec/integration.spec.ts | 160 ++++++++++++++++-- .../router-store/src/router_store_module.ts | 105 +++++++----- 2 files changed, 209 insertions(+), 56 deletions(-) diff --git a/modules/router-store/spec/integration.spec.ts b/modules/router-store/spec/integration.spec.ts index 11254d38c0..19aa9100f2 100644 --- a/modules/router-store/spec/integration.spec.ts +++ b/modules/router-store/spec/integration.spec.ts @@ -1,6 +1,12 @@ -import { Component, Provider, Injectable } from '@angular/core'; +import { Component, Provider, Injectable, ErrorHandler } from '@angular/core'; import { TestBed } from '@angular/core/testing'; -import { NavigationEnd, Router, RouterStateSnapshot } from '@angular/router'; +import { + NavigationEnd, + Router, + RouterStateSnapshot, + NavigationCancel, + NavigationError, +} from '@angular/router'; import { RouterTestingModule } from '@angular/router/testing'; import { Store, StoreModule, ScannedActionsSubject } from '@ngrx/store'; import { filter, first, mapTo, take } from 'rxjs/operators'; @@ -19,6 +25,7 @@ import { ROUTER_REQUEST, ROUTER_NAVIGATED, NavigationActionTiming, + RouterReducerState, } from '../src/router_store_module'; describe('integration spec', () => { @@ -119,7 +126,7 @@ describe('integration spec', () => { }); }); - it('should support rolling back if navigation gets canceled', (done: any) => { + it('should support rolling back if navigation gets canceled (navigation initialized through roter)', (done: any) => { const reducer = (state: string = '', action: RouterAction): any => { if (action.type === ROUTER_NAVIGATION) { return { @@ -176,9 +183,9 @@ describe('integration spec', () => { { type: 'store', state: { - url: '/next', + url: '/', lastAction: ROUTER_CANCEL, - storeState: { url: '/next', lastAction: ROUTER_NAVIGATION }, + storeState: { url: '/', lastAction: ROUTER_NAVIGATION }, }, }, { type: 'action', action: ROUTER_CANCEL }, @@ -189,7 +196,67 @@ describe('integration spec', () => { }); }); - it('should support rolling back if navigation errors', (done: any) => { + it('should support rolling back if navigation gets canceled (navigation initialized through store)', (done: any) => { + const CHANGE_ROUTE = 'CHANGE_ROUTE'; + const reducer = ( + state: RouterReducerState, + action: any + ): RouterReducerState => { + if (action.type === CHANGE_ROUTE) { + return { + state: { url: '/next', root: {} }, + navigationId: 123, + }; + } else { + const nextState = routerReducer(state, action); + if (nextState && nextState.state) { + nextState.state.root = {}; + } + return nextState; + } + }; + + createTestModule({ + reducers: { reducer }, + canActivate: () => false, + config: { stateKey: 'reducer' }, + }); + + const router: Router = TestBed.get(Router); + const store: Store = TestBed.get(Store); + const log = logOfRouterAndActionsAndStore(); + + router + .navigateByUrl('/') + .then(() => { + log.splice(0); + store.dispatch({ type: CHANGE_ROUTE }); + return waitForNavigation(router, NavigationCancel); + }) + .then(() => { + expect(log).toEqual([ + { type: 'router', event: 'NavigationStart', url: '/next' }, + { + type: 'store', + state: { state: { url: '/next', root: {} }, navigationId: 123 }, + }, + { type: 'action', action: CHANGE_ROUTE }, + { type: 'router', event: 'RoutesRecognized', url: '/next' }, + { type: 'router', event: 'GuardsCheckStart', url: '/next' }, + { type: 'router', event: 'GuardsCheckEnd', url: '/next' }, + { + type: 'store', + state: { state: { url: '/', root: {} }, navigationId: 2 }, + }, + { type: 'action', action: ROUTER_CANCEL }, + { type: 'router', event: 'NavigationCancel', url: '/next' }, + ]); + + done(); + }); + }); + + it('should support rolling back if navigation errors (navigation initialized through router)', (done: any) => { const reducer = (state: string = '', action: RouterAction): any => { if (action.type === ROUTER_NAVIGATION) { return { @@ -246,9 +313,9 @@ describe('integration spec', () => { { type: 'store', state: { - url: '/next', + url: '/', lastAction: ROUTER_ERROR, - storeState: { url: '/next', lastAction: ROUTER_NAVIGATION }, + storeState: { url: '/', lastAction: ROUTER_NAVIGATION }, }, }, { type: 'action', action: ROUTER_ERROR }, @@ -259,6 +326,74 @@ describe('integration spec', () => { }); }); + it('should support rolling back if navigation errors (navigation initialized through store)', (done: any) => { + const CHANGE_ROUTE = 'CHANGE_ROUTE'; + const reducer = ( + state: RouterReducerState, + action: any + ): RouterReducerState => { + if (action.type === CHANGE_ROUTE) { + return { + state: { url: '/next', root: {} }, + navigationId: 123, + }; + } else { + const nextState = routerReducer(state, action); + if (nextState && nextState.state) { + nextState.state.root = {}; + } + return nextState; + } + }; + + class SilentErrorHandler implements ErrorHandler { + handleError() { + // don't log router navigation error to not pollute console output of test + } + } + + createTestModule({ + reducers: { reducer }, + canActivate: () => { + throw new Error('BOOM!'); + }, + providers: [{provide: ErrorHandler, useClass: SilentErrorHandler}], + config: { stateKey: 'reducer' }, + }); + + const router: Router = TestBed.get(Router); + const store: Store = TestBed.get(Store); + const log = logOfRouterAndActionsAndStore(); + + router + .navigateByUrl('/') + .then(() => { + log.splice(0); + store.dispatch({ type: CHANGE_ROUTE }); + return waitForNavigation(router, NavigationError); + }) + .then(() => { + expect(log).toEqual([ + { type: 'router', event: 'NavigationStart', url: '/next' }, + { + type: 'store', + state: { state: { url: '/next', root: {} }, navigationId: 123 }, + }, + { type: 'action', action: CHANGE_ROUTE }, + { type: 'router', event: 'RoutesRecognized', url: '/next' }, + { type: 'router', event: 'GuardsCheckStart', url: '/next' }, + { + type: 'store', + state: { state: { url: '/', root: {} }, navigationId: 2 }, + }, + { type: 'action', action: ROUTER_ERROR }, + { type: 'router', event: 'NavigationError', url: '/next' }, + ]); + + done(); + }); + }); + it('should call navigateByUrl when resetting state of the routerReducer', (done: any) => { const reducer = (state: any, action: RouterAction) => { const r = routerReducer(state, action); @@ -385,7 +520,7 @@ describe('integration spec', () => { { type: 'store', state: null }, // ROUTER_REQEST event in the store { type: 'action', action: ROUTER_REQUEST }, { type: 'router', event: 'NavigationStart', url: '/load' }, - { type: 'store', state: null }, + { type: 'store', state: { url: '', navigationId: 1 } }, { type: 'action', action: ROUTER_CANCEL }, { type: 'router', event: 'NavigationCancel', url: '/load' }, ]); @@ -744,12 +879,9 @@ function createTestModule( TestBed.createComponent(AppCmp); } -function waitForNavigation(router: Router) { +function waitForNavigation(router: Router, event: any = NavigationEnd) { return router.events - .pipe( - filter((e): e is NavigationEnd => e instanceof NavigationEnd), - first() - ) + .pipe(filter(e => e instanceof event), first()) .toPromise(); } diff --git a/modules/router-store/src/router_store_module.ts b/modules/router-store/src/router_store_module.ts index 078e163bfc..c7cabceb83 100644 --- a/modules/router-store/src/router_store_module.ts +++ b/modules/router-store/src/router_store_module.ts @@ -3,6 +3,7 @@ import { InjectionToken, ModuleWithProviders, NgModule, + ErrorHandler, } from '@angular/core'; import { NavigationCancel, @@ -20,6 +21,7 @@ import { SerializedRouterStateSnapshot, BaseRouterStoreState, } from './serializer'; +import { withLatestFrom } from 'rxjs/operators'; /** * An action dispatched when a router navigation request is fired. @@ -291,7 +293,7 @@ export class StoreRouterConnectingModule { }; } - private routerState: SerializedRouterStateSnapshot; + private routerState: SerializedRouterStateSnapshot | null; private storeState: any; private trigger = RouterTrigger.NONE; @@ -301,7 +303,8 @@ export class StoreRouterConnectingModule { private store: Store, private router: Router, private serializer: RouterStateSerializer, - @Inject(ROUTER_CONFIG) private config: StoreRouterConfig + @Inject(ROUTER_CONFIG) private config: StoreRouterConfig, + private errorHandler: ErrorHandler ) { this.stateKey = this.config.stateKey as string; @@ -310,29 +313,31 @@ export class StoreRouterConnectingModule { } private setUpStoreStateListener(): void { - this.store.subscribe(state => { - this.storeState = state; - }); - this.store.pipe(select(this.stateKey)).subscribe(() => { - this.navigateIfNeeded(); - }); + this.store + .pipe(select(this.stateKey), withLatestFrom(this.store)) + .subscribe(([routerStoreState, storeState]) => { + this.navigateIfNeeded(routerStoreState, storeState); + }); } - private navigateIfNeeded(): void { - if ( - !this.storeState[this.stateKey] || - !this.storeState[this.stateKey].state - ) { + private navigateIfNeeded( + routerStoreState: RouterReducerState, + storeState: any + ): void { + if (!routerStoreState || !routerStoreState.state) { return; } if (this.trigger === RouterTrigger.ROUTER) { return; } - const url = this.storeState[this.stateKey].state.url; + const url = routerStoreState.state.url; if (this.router.url !== url) { + this.storeState = storeState; this.trigger = RouterTrigger.STORE; - this.router.navigateByUrl(url); + this.router.navigateByUrl(url).catch(e => { + this.errorHandler.handleError(e); + }); } } @@ -342,32 +347,39 @@ export class StoreRouterConnectingModule { NavigationActionTiming.PostActivation; let routesRecognized: RoutesRecognized; - this.router.events.subscribe(event => { - if (event instanceof NavigationStart) { - if (this.trigger !== RouterTrigger.STORE) { - this.dispatchRouterRequest(event); - } - } else if (event instanceof RoutesRecognized) { - routesRecognized = event; - this.routerState = this.serializer.serialize(event.state); + this.router.events + .pipe(withLatestFrom(this.store)) + .subscribe(([event, storeState]) => { + if (event instanceof NavigationStart) { + this.routerState = this.serializer.serialize( + this.router.routerState.snapshot + ); + if (this.trigger !== RouterTrigger.STORE) { + this.storeState = storeState; + this.dispatchRouterRequest(event); + } + } else if (event instanceof RoutesRecognized) { + routesRecognized = event; - if (!dispatchNavLate && this.trigger !== RouterTrigger.STORE) { - this.dispatchRouterNavigation(event); - } - } else if (event instanceof NavigationCancel) { - this.dispatchRouterCancel(event); - } else if (event instanceof NavigationError) { - this.dispatchRouterError(event); - } else if (event instanceof NavigationEnd) { - if (this.trigger !== RouterTrigger.STORE) { - if (dispatchNavLate) { - this.dispatchRouterNavigation(routesRecognized); + if (!dispatchNavLate && this.trigger !== RouterTrigger.STORE) { + this.dispatchRouterNavigation(event); + } + } else if (event instanceof NavigationCancel) { + this.dispatchRouterCancel(event); + this.reset(); + } else if (event instanceof NavigationError) { + this.dispatchRouterError(event); + this.reset(); + } else if (event instanceof NavigationEnd) { + if (this.trigger !== RouterTrigger.STORE) { + if (dispatchNavLate) { + this.dispatchRouterNavigation(routesRecognized); + } + this.dispatchRouterNavigated(event); } - this.dispatchRouterNavigated(event); + this.reset(); } - this.trigger = RouterTrigger.NONE; - } - }); + }); } private dispatchRouterRequest(event: NavigationStart): void { @@ -377,20 +389,23 @@ export class StoreRouterConnectingModule { private dispatchRouterNavigation( lastRoutesRecognized: RoutesRecognized ): void { + const nextRouterState = this.serializer.serialize( + lastRoutesRecognized.state + ); this.dispatchRouterAction(ROUTER_NAVIGATION, { - routerState: this.routerState, + routerState: nextRouterState, event: new RoutesRecognized( lastRoutesRecognized.id, lastRoutesRecognized.url, lastRoutesRecognized.urlAfterRedirects, - this.routerState + nextRouterState ), }); } private dispatchRouterCancel(event: NavigationCancel): void { this.dispatchRouterAction(ROUTER_CANCEL, { - routerState: this.routerState, + routerState: this.routerState!, storeState: this.storeState, event, }); @@ -398,7 +413,7 @@ export class StoreRouterConnectingModule { private dispatchRouterError(event: NavigationError): void { this.dispatchRouterAction(ROUTER_ERROR, { - routerState: this.routerState, + routerState: this.routerState!, storeState: this.storeState, event: new NavigationError(event.id, event.url, `${event}`), }); @@ -416,4 +431,10 @@ export class StoreRouterConnectingModule { this.trigger = RouterTrigger.NONE; } } + + private reset() { + this.trigger = RouterTrigger.NONE; + this.storeState = null; + this.routerState = null; + } } From 7894950f85b4b7ebd42529d74d8b4fb3103344d2 Mon Sep 17 00:00:00 2001 From: dummdidumm <5968653+dummdidumm@users.noreply.github.com> Date: Mon, 27 Aug 2018 09:08:34 +0200 Subject: [PATCH 2/2] fix(router-store): Code cleanup, testing error handler --- modules/router-store/spec/integration.spec.ts | 15 ++++++++------- modules/router-store/src/router_store_module.ts | 8 ++++---- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/modules/router-store/spec/integration.spec.ts b/modules/router-store/spec/integration.spec.ts index 19aa9100f2..476fbf2580 100644 --- a/modules/router-store/spec/integration.spec.ts +++ b/modules/router-store/spec/integration.spec.ts @@ -126,7 +126,7 @@ describe('integration spec', () => { }); }); - it('should support rolling back if navigation gets canceled (navigation initialized through roter)', (done: any) => { + it('should support rolling back if navigation gets canceled (navigation initialized through router)', (done: any) => { const reducer = (state: string = '', action: RouterAction): any => { if (action.type === ROUTER_NAVIGATION) { return { @@ -326,7 +326,7 @@ describe('integration spec', () => { }); }); - it('should support rolling back if navigation errors (navigation initialized through store)', (done: any) => { + it('should support rolling back if navigation errors and hand error to error handler (navigation initialized through store)', (done: any) => { const CHANGE_ROUTE = 'CHANGE_ROUTE'; const reducer = ( state: RouterReducerState, @@ -346,18 +346,19 @@ describe('integration spec', () => { } }; + const routerError = new Error('BOOM!'); class SilentErrorHandler implements ErrorHandler { - handleError() { - // don't log router navigation error to not pollute console output of test + handleError(error: any) { + expect(error).toBe(routerError); } } createTestModule({ reducers: { reducer }, canActivate: () => { - throw new Error('BOOM!'); + throw routerError; }, - providers: [{provide: ErrorHandler, useClass: SilentErrorHandler}], + providers: [{ provide: ErrorHandler, useClass: SilentErrorHandler }], config: { stateKey: 'reducer' }, }); @@ -369,7 +370,7 @@ describe('integration spec', () => { .navigateByUrl('/') .then(() => { log.splice(0); - store.dispatch({ type: CHANGE_ROUTE }); + store.dispatch({ type: CHANGE_ROUTE }); return waitForNavigation(router, NavigationError); }) .then(() => { diff --git a/modules/router-store/src/router_store_module.ts b/modules/router-store/src/router_store_module.ts index c7cabceb83..17cc2b1ae3 100644 --- a/modules/router-store/src/router_store_module.ts +++ b/modules/router-store/src/router_store_module.ts @@ -303,8 +303,8 @@ export class StoreRouterConnectingModule { private store: Store, private router: Router, private serializer: RouterStateSerializer, - @Inject(ROUTER_CONFIG) private config: StoreRouterConfig, - private errorHandler: ErrorHandler + private errorHandler: ErrorHandler, + @Inject(ROUTER_CONFIG) private config: StoreRouterConfig ) { this.stateKey = this.config.stateKey as string; @@ -335,8 +335,8 @@ export class StoreRouterConnectingModule { if (this.router.url !== url) { this.storeState = storeState; this.trigger = RouterTrigger.STORE; - this.router.navigateByUrl(url).catch(e => { - this.errorHandler.handleError(e); + this.router.navigateByUrl(url).catch(error => { + this.errorHandler.handleError(error); }); } }