From 8a45d049e1d17a9c50f29834bf35d90715978157 Mon Sep 17 00:00:00 2001 From: Christian Svensson Date: Fri, 21 Oct 2016 19:46:46 +0200 Subject: [PATCH] fix(onBefore): Skip remaining hooks after the (#2) first hook that modifies the transition. --- src/transition/transition.ts | 5 +- src/transition/transitionHook.ts | 37 +++++++++----- test/transitionSpec.ts | 86 +++++++++++++++++++++++++++++++- 3 files changed, 113 insertions(+), 15 deletions(-) diff --git a/src/transition/transition.ts b/src/transition/transition.ts index 8d1e3d17..42c1bd3d 100644 --- a/src/transition/transition.ts +++ b/src/transition/transition.ts @@ -473,6 +473,7 @@ export class Transition implements IHookRegistry { */ run(): Promise { let runSynchronousHooks = TransitionHook.runSynchronousHooks; + let runAllHooks = TransitionHook.runAllHooks; let hookBuilder = this.hookBuilder(); let globals = this.router.globals; globals.transitionHistory.enqueue(this); @@ -503,7 +504,7 @@ export class Transition implements IHookRegistry { trace.traceSuccess(this.$to(), this); this.success = true; this._deferred.resolve(this.to()); - runSynchronousHooks(hookBuilder.getOnSuccessHooks(), true); + runAllHooks(hookBuilder.getOnSuccessHooks()); }; const transitionError = (reason: any) => { @@ -511,7 +512,7 @@ export class Transition implements IHookRegistry { this.success = false; this._deferred.reject(reason); this._error = reason; - runSynchronousHooks(hookBuilder.getOnErrorHooks(), true); + runAllHooks(hookBuilder.getOnErrorHooks()); }; trace.traceTransitionStart(this); diff --git a/src/transition/transitionHook.ts b/src/transition/transitionHook.ts index 8a790ea1..326b3850 100644 --- a/src/transition/transitionHook.ts +++ b/src/transition/transitionHook.ts @@ -94,31 +94,44 @@ export class TransitionHook { return `${event} context: ${context}, ${maxLength(200, name)}`; } + /** + * Run all TransitionHooks, ignoring their return value. + */ + static runAllHooks(hooks: TransitionHook[]): void { + hooks.forEach(hook => { + try { + hook.invokeHook(); + } catch (exception) { + let errorHandler = hook.transition.router.stateService.defaultErrorHandler(); + errorHandler(exception); + } + }); + } /** * Given an array of TransitionHooks, runs each one synchronously and sequentially. + * Should any hook return a Rejection synchronously, the remaining hooks will not run. * * Returns a promise chain composed of any promises returned from each hook.invokeStep() call */ - static runSynchronousHooks(hooks: TransitionHook[], swallowExceptions: boolean = false): Promise { + static runSynchronousHooks(hooks: TransitionHook[]): Promise { let results: Promise[] = []; - for (let i = 0; i < hooks.length; i++) { - let hook = hooks[i]; + + for (let hook of hooks) { try { - results.push(hook.invokeHook()); - } catch (exception) { - if (!swallowExceptions) { - return Rejection.errored(exception).toPromise(); + let hookResult = hook.invokeHook(); + + if (Rejection.isTransitionRejectionPromise(hookResult)) { + // Break on first thrown error or false/TargetState + return hookResult; } - let errorHandler = hook.transition.router.stateService.defaultErrorHandler(); - errorHandler(exception); + results.push(hookResult); + } catch (exception) { + return Rejection.errored(exception).toPromise(); } } - let rejections = results.filter(Rejection.isTransitionRejectionPromise); - if (rejections.length) return rejections[0]; - return results .filter(isPromise) .reduce((chain: Promise, promise: Promise) => chain.then(val(promise)), services.$q.when()); diff --git a/test/transitionSpec.ts b/test/transitionSpec.ts index 4b6f8d51..1f747f9d 100644 --- a/test/transitionSpec.ts +++ b/test/transitionSpec.ts @@ -152,6 +152,61 @@ describe('transition', function () { }), 20); })); + describe('.onBefore()', function() { + beforeEach(() => $state.defaultErrorHandler(() => {})); + + it('should stop running remaining hooks if hook modifies transition synchronously', ((done) => { + let counter = 0; + let increment = (amount) => { + return () => { + counter += amount; + return false; + }; + }; + + $transitions.onBefore({}, increment(1), { priority: 2 }); + $transitions.onBefore({}, increment(100), { priority: 1 }); + + Promise.resolve() + .then(goFail('first', 'second')) + .then(() => expect(counter).toBe(1)) + .then(goFail('second', 'third')) + .then(() => expect(counter).toBe(2)) + .then(done); + })); + + it('should stop running remaining hooks when synchronous result throws or returns false|TargetState', ((done) => { + let current = null; + + $transitions.onBefore({}, (t) => { current = t.to().name; }); + $transitions.onBefore({ to: 'first' }, () => { + throw Error('first-error'); + }, { priority: 1 }); + $transitions.onBefore({ to: 'second' }, () => false, { priority: 3 }); + $transitions.onBefore({ to: 'third' }, () => $state.target('A'), { priority: 2 }); + + Promise.resolve() + .then(goFail('A', 'first')) + .then((res) => { + expect(res.type).toBe(RejectType.ERROR); + expect(current).toBe(null); + }) + .then(goFail('A', 'second')) + .then((res) => { + expect(res.type).toBe(RejectType.ABORTED); + expect(current).toBe(null); + }) + .then(goFail('A', 'third')) + .then((res) => { + expect(res.type).toBe(RejectType.SUPERSEDED); + expect(current).toBe(null); + }) + .then(go('A', 'B')) + .then(() => expect(current).toBe('B')) + .then(done); + })); + }); + describe('.onStart()', function() { it('should fire matching events when transition starts', ((done) => { var t = null; @@ -317,7 +372,9 @@ describe('transition', function () { .then(done); })); - it('should be called even if other .onSuccess() callbacks fail (throw errors, etc)', ((done) => { + it('should call all .onSuccess() even when callbacks fail (throw errors, etc)', ((done) => { + $transitions.onSuccess({ from: "*", to: "*" }, () => false); + $transitions.onSuccess({ from: "*", to: "*" }, () => $state.target('A')); $transitions.onSuccess({ from: "*", to: "*" }, function() { throw new Error("oops!"); }); $transitions.onSuccess({ from: "*", to: "*" }, function(trans) { states.push(trans.to().name); }); @@ -364,6 +421,33 @@ describe('transition', function () { .then(() => expect(hooks).toEqual([ 'splatsplat', 'AD' ])) .then(done); })); + + it('should call all error handlers when transition fails.', ((done) => { + let count = 0; + + $state.defaultErrorHandler(() => {}); + $transitions.onStart({}, () => false); + $transitions.onError({}, () => { + count += 1; + return false; + }); + $transitions.onError({}, () => { + count += 10; + $state.target('A'); + }); + $transitions.onError({}, function() { + count += 100; + throw new Error("oops!"); + }); + $transitions.onError({}, () => { + count += 1000; + }); + + Promise.resolve() + .then(goFail("B", "C")) + .then(() => expect(count).toBe(1111)) + .then(done); + })); }); // Test for #2866