From a7464bb28466d794bc0406fec0e32f8256ced58b Mon Sep 17 00:00:00 2001 From: Chris Thielen Date: Tue, 14 Feb 2017 11:32:41 -0600 Subject: [PATCH] feat(Transition): Normalize all transition errors to a Rejection. BREAKING CHANGE: All Transition errors are now wrapped in a Rejection object. #### Before: Previously, if a transition hook returned a rejected promise: ```js .onStart({}, () => Promise.reject('reject transition')); ``` In `onError` or `transtion.promise.catch()`, the raw rejection was returned: ```js .onError({}, (trans, err) => err === 'reject transition') ``` #### Now: Now, the error is wrapped in a Rejection object. - The detail (thrown error or rejected value) is still available as `.detail`. ```js .onError({}, (trans, err) => err instanceof Rejection && err.detail === 'reject transition') ``` - The Rejection object indicates the `.type` of transition rejection (ABORTED, ERROR, SUPERSEDED and/or redirection). ```js .onError({}, (trans, err) => { err.type === RejectType.ABORTED === 3 }); ``` #### Motivation: Errors *thrown from* a hook and rejection values *returned from* a hook can now be processed in the same way. #### BC Likelihood How likely is this to affect me? Medium: apps which have onError handlers for rejected values #### BC Severity How severe is this change? Low: Find all error handlers (or .catch/.then chains) that do not understand Rejection. Add `err.detail` processing. --- src/transition/transitionEventType.ts | 2 +- src/transition/transitionHook.ts | 124 +++++++++++++++----------- src/transition/transitionService.ts | 14 +-- test/lazyLoadSpec.ts | 8 +- 4 files changed, 86 insertions(+), 62 deletions(-) diff --git a/src/transition/transitionEventType.ts b/src/transition/transitionEventType.ts index 6da7a7ba..1e67d2bb 100644 --- a/src/transition/transitionEventType.ts +++ b/src/transition/transitionEventType.ts @@ -16,6 +16,6 @@ export class TransitionEventType { public reverseSort: boolean = false, public getResultHandler: GetResultHandler = TransitionHook.HANDLE_RESULT, public getErrorHandler: GetErrorHandler = TransitionHook.REJECT_ERROR, - public rejectIfSuperseded: boolean = true, + public synchronous: boolean = false, ) { } } diff --git a/src/transition/transitionHook.ts b/src/transition/transitionHook.ts index b3e070ea..9c087762 100644 --- a/src/transition/transitionHook.ts +++ b/src/transition/transitionHook.ts @@ -1,22 +1,21 @@ /** * @coreapi * @module transition - */ /** for typedoc */ -import {TransitionHookOptions, HookResult} from "./interface"; -import {defaults, noop, identity} from "../common/common"; -import {fnToString, maxLength} from "../common/strings"; -import {isPromise} from "../common/predicates"; -import {val, is, parse} from "../common/hof"; -import {trace} from "../common/trace"; -import {services} from "../common/coreservices"; - -import {Rejection} from "./rejectFactory"; -import {TargetState} from "../state/targetState"; -import {Transition} from "./transition"; -import {State} from "../state/stateObject"; -import {TransitionEventType} from "./transitionEventType"; -import {StateService} from "../state/stateService"; // has or is using -import {RegisteredHook} from "./hookRegistry"; // has or is using + */ +/** for typedoc */ +import { TransitionHookOptions, HookResult } from './interface'; +import { defaults, noop, silentRejection } from '../common/common'; +import { fnToString, maxLength } from '../common/strings'; +import { isPromise } from '../common/predicates'; +import { is, parse } from '../common/hof'; +import { trace } from '../common/trace'; +import { services } from '../common/coreservices'; +import { Rejection } from './rejectFactory'; +import { TargetState } from '../state/targetState'; +import { Transition } from './transition'; +import { State } from '../state/stateObject'; +import { TransitionEventType } from './transitionEventType'; +import { RegisteredHook } from './hookRegistry'; // has or is using let defaultOptions: TransitionHookOptions = { current: noop, @@ -33,35 +32,52 @@ export type ErrorHandler = (error) => Promise; /** @hidden */ export class TransitionHook { + type: TransitionEventType; constructor(private transition: Transition, private stateContext: State, private registeredHook: RegisteredHook, private options: TransitionHookOptions) { this.options = defaults(options, defaultOptions); + this.type = registeredHook.eventType; } - stateService = () => this.transition.router.stateService; + /** + * These GetResultHandler(s) are used by [[invokeHook]] below + * Each HookType chooses a GetResultHandler (See: [[TransitionService._defineCoreEvents]]) + */ + static HANDLE_RESULT: GetResultHandler = (hook: TransitionHook) => (result: HookResult) => + hook.handleHookResult(result); - static HANDLE_RESULT: GetResultHandler = (hook: TransitionHook) => - (result: HookResult) => - hook.handleHookResult(result); + /** + * If the result is a promise rejection, log it. + * Otherwise, ignore the result. + */ + static LOG_REJECTED_RESULT: GetResultHandler = (hook: TransitionHook) => (result: HookResult) => { + isPromise(result) && result.catch(err => + hook.logError(Rejection.normalize(err))); + return undefined; + }; - static IGNORE_RESULT: GetResultHandler = (hook: TransitionHook) => - (result: HookResult) => undefined; + /** + * These GetErrorHandler(s) are used by [[invokeHook]] below + * Each HookType chooses a GetErrorHandler (See: [[TransitionService._defineCoreEvents]]) + */ + static LOG_ERROR: GetErrorHandler = (hook: TransitionHook) => (error) => + hook.logError(error); - static LOG_ERROR: GetErrorHandler = (hook: TransitionHook) => - (error) => - (hook.stateService().defaultErrorHandler()(error), undefined); + static REJECT_ERROR: GetErrorHandler = (hook: TransitionHook) => (error) => + silentRejection(error); - static REJECT_ERROR: GetErrorHandler = (hook: TransitionHook) => - (error) => - Rejection.errored(error).toPromise(); + static THROW_ERROR: GetErrorHandler = (hook: TransitionHook) => (error) => { + throw error; + }; - static THROW_ERROR: GetErrorHandler = (hook: TransitionHook) => - undefined; + private isSuperseded = () => + !this.type.synchronous && this.options.current() !== this.options.transition; - private rejectIfSuperseded = () => - this.registeredHook.eventType.rejectIfSuperseded && this.options.current() !== this.options.transition; + logError(err): any { + this.transition.router.stateService.defaultErrorHandler()(err); + } invokeHook(): Promise { let hook = this.registeredHook; @@ -71,30 +87,36 @@ export class TransitionHook { trace.traceHookInvocation(this, this.transition, options); // A new transition started before this hook (from a previous transition) could be run. - if (this.rejectIfSuperseded()) { + if (this.isSuperseded()) { return Rejection.superseded(options.current()).toPromise(); } - let cb = hook.callback; - let bind = this.options.bind; - let trans = this.transition; - let state = this.stateContext; + const invokeCallback = () => + hook.callback.call(this.options.bind, this.transition, this.stateContext); - let errorHandler = hook.eventType.getErrorHandler(this); - let resultHandler = hook.eventType.getResultHandler(this); - resultHandler = resultHandler || identity; + const normalizeErr = err => + Rejection.normalize(err).toPromise(); - if (!errorHandler) { - return resultHandler(cb.call(bind, trans, state)); - } + const handleError = err => + hook.eventType.getErrorHandler(this)(err); + + const handleResult = result => + hook.eventType.getResultHandler(this)(result); - try { - return resultHandler(cb.call(bind, trans, state)); - } catch (error) { - return errorHandler(error); + if (this.type.synchronous) { + try { + return handleResult(invokeCallback()); + } catch (err) { + return handleError(Rejection.normalize(err)); + } } + + return services.$q.when() + .then(invokeCallback) + .catch(normalizeErr) + .then(handleResult, handleError); } - + /** * This method handles the return value of a Transition Hook. * @@ -107,15 +129,15 @@ export class TransitionHook { handleHookResult(result: HookResult): Promise { // This transition is no longer current. // Another transition started while this hook was still running. - if (this.rejectIfSuperseded()) { + if (this.isSuperseded()) { // Abort this transition return Rejection.superseded(this.options.current()).toPromise(); } // Hook returned a promise if (isPromise(result)) { - // Wait for the promise, then reprocess the settled promise value - return result.then(this.handleHookResult.bind(this)); + // Wait for the promise, then reprocess with the resulting value + return result.then(val => this.handleHookResult(val)); } trace.traceHookResult(result, this.transition, this.options); diff --git a/src/transition/transitionService.ts b/src/transition/transitionService.ts index d55e3a4f..ffab636f 100644 --- a/src/transition/transitionService.ts +++ b/src/transition/transitionService.ts @@ -235,19 +235,21 @@ export class TransitionService implements IHookRegistry, Disposable { const Phase = TransitionHookPhase; const TH = TransitionHook; const paths = this._criteriaPaths; + const NORMAL_SORT = false, REVERSE_SORT = true; + const ASYNCHRONOUS = false, SYNCHRONOUS = true; - this._defineEvent("onCreate", Phase.CREATE, 0, paths.to, false, TH.IGNORE_RESULT, TH.THROW_ERROR, false); + this._defineEvent("onCreate", Phase.CREATE, 0, paths.to, NORMAL_SORT, TH.LOG_REJECTED_RESULT, TH.THROW_ERROR, SYNCHRONOUS); this._defineEvent("onBefore", Phase.BEFORE, 0, paths.to); this._defineEvent("onStart", Phase.RUN, 0, paths.to); - this._defineEvent("onExit", Phase.RUN, 100, paths.exiting, true); + this._defineEvent("onExit", Phase.RUN, 100, paths.exiting, REVERSE_SORT); this._defineEvent("onRetain", Phase.RUN, 200, paths.retained); this._defineEvent("onEnter", Phase.RUN, 300, paths.entering); this._defineEvent("onFinish", Phase.RUN, 400, paths.to); - this._defineEvent("onSuccess", Phase.SUCCESS, 0, paths.to, false, TH.IGNORE_RESULT, TH.LOG_ERROR, false); - this._defineEvent("onError", Phase.ERROR, 0, paths.to, false, TH.IGNORE_RESULT, TH.LOG_ERROR, false); + this._defineEvent("onSuccess", Phase.SUCCESS, 0, paths.to, NORMAL_SORT, TH.LOG_REJECTED_RESULT, TH.LOG_ERROR, SYNCHRONOUS); + this._defineEvent("onError", Phase.ERROR, 0, paths.to, NORMAL_SORT, TH.LOG_REJECTED_RESULT, TH.LOG_ERROR, SYNCHRONOUS); } /** @hidden */ @@ -269,9 +271,9 @@ export class TransitionService implements IHookRegistry, Disposable { reverseSort: boolean = false, getResultHandler: GetResultHandler = TransitionHook.HANDLE_RESULT, getErrorHandler: GetErrorHandler = TransitionHook.REJECT_ERROR, - rejectIfSuperseded: boolean = true) + synchronous: boolean = false) { - let eventType = new TransitionEventType(name, hookPhase, hookOrder, criteriaMatchPath, reverseSort, getResultHandler, getErrorHandler, rejectIfSuperseded); + let eventType = new TransitionEventType(name, hookPhase, hookOrder, criteriaMatchPath, reverseSort, getResultHandler, getErrorHandler, synchronous); this._eventTypes.push(eventType); makeEvent(this, this, eventType); diff --git a/test/lazyLoadSpec.ts b/test/lazyLoadSpec.ts index 087ddfef..d772534a 100644 --- a/test/lazyLoadSpec.ts +++ b/test/lazyLoadSpec.ts @@ -389,7 +389,7 @@ describe('future state', function () { expect($state.get('A')).toBe(futureStateDef); $state.go('A').catch(() => { - expect(errors).toEqual(['nope']); + expect(errors.map(x => x.detail)).toEqual(['nope']); expect($state.get('A')).toBe(futureStateDef); done(); }); @@ -399,18 +399,18 @@ describe('future state', function () { expect($state.get('A')).toBe(futureStateDef); $state.go('A').catch(() => { - expect(errors).toEqual(['nope']); + expect(errors.map(x => x.detail)).toEqual(['nope']); expect($state.get('A')).toBe(futureStateDef); expect(count).toBe(1); $state.go('A').catch(() => { - expect(errors).toEqual(['nope', 'nope']); + expect(errors.map(x => x.detail)).toEqual(['nope', 'nope']); expect($state.get('A')).toBe(futureStateDef); expect(count).toBe(2); // this time it should lazy load $state.go('A').then(() => { - expect(errors).toEqual(['nope', 'nope']); + expect(errors.map(x => x.detail)).toEqual(['nope', 'nope']); expect($state.get('A')).toBeTruthy(); expect($state.get('A')).not.toBe(futureStateDef); expect(count).toBe(3);