fix(Rejection): Dont log an ignored trans as console.error

refactor(Rejection): rename TransitionRejection to Rejection. Remove RejectFactory in favor of static methods on Rejection.

We were creating a promise in RejectFactory and then not using the promise. Chrome (and ng2-polyfills) was logging this to the console as an uncaught error.

Closes #2676

Author: christopherthielen

src/common/strings.ts

@@ -1,7 +1,7 @@
 /** @module common_strings */ /** */
 
 import {isString, isArray, isDefined, isNull, isPromise, isInjectable, isObject} from "./predicates";
-import {TransitionRejection} from "../transition/rejectFactory";
+import {Rejection} from "../transition/rejectFactory";
 import {IInjectable, identity} from "./common";
 import {pattern, is, not, val, invoke} from "./hof";
 import {Transition} from "../transition/transition";
@@ -47,7 +47,6 @@ function _fromJson(json) {
 
 
 function promiseToString(p) {
-  if (is(TransitionRejection)(p.reason)) return p.reason.toString();
   return `Promise(${JSON.stringify(p)})`;
 }
 
@@ -62,15 +61,18 @@ export function fnToString(fn: IInjectable) {
   return _fn && _fn.toString() || "undefined";
 }
 
+const isTransitionRejectionPromise = Rejection.isTransitionRejectionPromise;
 
 let stringifyPattern = pattern([
-  [not(isDefined),            val("undefined")],
-  [isNull,                    val("null")],
-  [isPromise,                 promiseToString],
-  [is(Transition),            invoke("toString")],
-  [is(Resolvable),            invoke("toString")],
-  [isInjectable,              functionToString],
-  [val(true),                 identity]
+  [not(isDefined),                  val("undefined")],
+  [isNull,                          val("null")],
+  [isPromise,                       promiseToString],
+  [isTransitionRejectionPromise,    x => x._transitionRejection.toString()],
+  [is(Rejection),                   invoke("toString")],
+  [is(Transition),                  invoke("toString")],
+  [is(Resolvable),                  invoke("toString")],
+  [isInjectable,                    functionToString],
+  [val(true),                       identity]
 ]);
 
 export function stringify(o) {

src/state/hooks/transitionManager.ts

@@ -4,7 +4,7 @@ import {Param} from "../../params/param";
 
 import {TreeChanges} from "../../transition/interface";
 import {Transition} from "../../transition/transition";
-import {TransitionRejection, RejectType} from "../../transition/rejectFactory";
+import {Rejection, RejectType} from "../../transition/rejectFactory";
 
 import {StateDeclaration} from "../interface";
 import {StateService} from "../stateService";
@@ -76,7 +76,7 @@ export class TransitionManager {
   transRejected(error): (StateDeclaration|Promise<any>) {
     let {transition, $state, $q} = this;
     // Handle redirect and abort
-    if (error instanceof TransitionRejection) {
+    if (error instanceof Rejection) {
       if (error.type === RejectType.IGNORED) {
         // Update $stateParmas/$state.params/$location.url if transition ignored, but dynamic params have changed.
         let dynamic = $state.$current.parameters().filter(prop('dynamic'));

src/state/stateService.ts

@@ -15,7 +15,7 @@ import {UrlRouter} from "../url/urlRouter";
 
 import {TransitionOptions} from "../transition/interface";
 import {TransitionService, defaultTransOpts} from "../transition/transitionService";
-import {RejectFactory} from "../transition/rejectFactory";
+import {Rejection} from "../transition/rejectFactory";
 import {Transition} from "../transition/transition";
 
 import {StateOrName, StateDeclaration} from "./interface";
@@ -40,8 +40,6 @@ export class StateService {
   get current()     { return this.globals.current; }
   get $current()    { return this.globals.$current; }
 
-  private rejectFactory = new RejectFactory();
-
   constructor(private $view: ViewService,
               private $urlRouter: UrlRouter,
               private $transitions: TransitionService,
@@ -66,7 +64,6 @@ export class StateService {
     let latest = latestThing();
     let $from$ = PathFactory.makeTargetState(fromPath);
     let callbackQueue = new Queue<Function>([].concat(this.stateProvider.invalidCallbacks));
-    let rejectFactory = this.rejectFactory;
     let {$q, $injector} = services;
 
     const invokeCallback = (callback: Function) => $q.when($injector.invoke(callback, null, { $to$, $from$ }));
@@ -79,15 +76,15 @@ export class StateService {
       // Recreate the TargetState, in case the state is now defined.
       target = this.target(target.identifier(), target.params(), target.options());
 
-      if (!target.valid()) return rejectFactory.invalid(target.error());
-      if (latestThing() !== latest) return rejectFactory.superseded();
+      if (!target.valid()) return Rejection.invalid(target.error()).toPromise();
+      if (latestThing() !== latest) return Rejection.superseded().toPromise();
 
       return this.transitionTo(target.identifier(), target.params(), target.options());
     };
 
     function invokeNextCallback() {
       let nextCallback = callbackQueue.dequeue();
-      if (nextCallback === undefined) return rejectFactory.invalid($to$.error());
+      if (nextCallback === undefined) return Rejection.invalid($to$.error()).toPromise();
       return invokeCallback(nextCallback).then(checkForRedirect).then(result => result || invokeNextCallback());
     }
 

src/transition/rejectFactory.ts

@@ -8,59 +8,64 @@ export enum RejectType {
   SUPERSEDED = 2, ABORTED = 3, INVALID = 4, IGNORED = 5
 }
 
-export class TransitionRejection {
+export class Rejection {
   type: number;
   message: string;
   detail: string;
   redirected: boolean;
 
-  constructor(type, message, detail) {
-    extend(this, {
-      type: type,
-      message: message,
-      detail: detail
-    });
+  constructor(type, message?, detail?) {
+    this.type = type;
+    this.message = message;
+    this.detail = detail;
   }
 
   toString() {
     const detailString = d => d && d.toString !== Object.prototype.toString ? d.toString() : stringify(d);
     let type = this.type, message = this.message, detail = detailString(this.detail);
     return `TransitionRejection(type: ${type}, message: ${message}, detail: ${detail})`;
   }
-}
 
+  toPromise() {
+    return extend(services.$q.reject(this), { _transitionRejection: this });
+  }
+
+  /** Returns true if the obj is a rejected promise created from the `asPromise` factory */
+  static isTransitionRejectionPromise(obj) {
+    return obj && (typeof obj.then === 'function') && obj._transitionRejection instanceof Rejection;
+  }
 
-export class RejectFactory {
-  constructor() {}
-  superseded(detail?: any, options?: any) {
+  /** Returns a TransitionRejection due to transition superseded */
+  static superseded(detail?: any, options?: any) {
     let message = "The transition has been superseded by a different transition (see detail).";
-    let reason = new TransitionRejection(RejectType.SUPERSEDED, message, detail);
+    let rejection = new Rejection(RejectType.SUPERSEDED, message, detail);
     if (options && options.redirected) {
-      reason.redirected = true;
+      rejection.redirected = true;
     }
-    return extend(services.$q.reject(reason), {reason: reason});
+    return rejection;
   }
 
-  redirected(detail?: any) {
-    return this.superseded(detail, {redirected: true});
+  /** Returns a TransitionRejection due to redirected transition */
+  static redirected(detail?: any) {
+    return Rejection.superseded(detail, {redirected: true});
   }
 
-  invalid(detail?: any) {
+  /** Returns a TransitionRejection due to invalid transition */
+  static invalid(detail?: any) {
     let message = "This transition is invalid (see detail)";
-    let reason = new TransitionRejection(RejectType.INVALID, message, detail);
-    return extend(services.$q.reject(reason), {reason: reason});
+    return new Rejection(RejectType.INVALID, message, detail);
   }
 
-  ignored(detail?: any) {
+  /** Returns a TransitionRejection due to ignored transition */
+  static ignored(detail?: any) {
     let message = "The transition was ignored.";
-    let reason = new TransitionRejection(RejectType.IGNORED, message, detail);
-    return extend(services.$q.reject(reason), {reason: reason});
+    return new Rejection(RejectType.IGNORED, message, detail);
   }
 
-  aborted(detail?: any) {
+  /** Returns a TransitionRejection due to aborted transition */
+  static aborted(detail?: any) {
     // TODO think about how to encapsulate an Error() object
     let message = "The transition has been aborted.";
-    let reason = new TransitionRejection(RejectType.ABORTED, message, detail);
-    return extend(services.$q.reject(reason), {reason: reason});
+    return new Rejection(RejectType.ABORTED, message, detail);
   }
 }

src/transition/transition.ts

@@ -2,26 +2,27 @@
 import {trace} from "../common/trace";
 import {services} from "../common/coreservices";
 import {
-    map, find, extend, filter, mergeR, unnest, tail,
-    omit, toJson, abstractKey, arrayTuples, allTrueR, unnestR, identity, anyTrueR
+    map, find, extend, filter, mergeR,  tail,
+    omit, toJson, abstractKey, arrayTuples, unnestR, identity, anyTrueR
 } from "../common/common";
 import { isObject } from "../common/predicates";
 import { not, prop, propEq, val } from "../common/hof";
 
 import {StateDeclaration, StateOrName} from "../state/interface";
 import {TransitionOptions, TransitionHookOptions, TreeChanges, IHookRegistry, IHookRegistration, IHookGetter} from "./interface";
 
-import {TransitionHook, HookRegistry, matchState, HookBuilder, RejectFactory} from "./module";
+import {TransitionHook, HookRegistry, matchState, HookBuilder} from "./module";
 import {Node} from "../path/node";
 import {PathFactory} from "../path/pathFactory";
 import {State, TargetState} from "../state/module";
 import {Param} from "../params/module";
 import {Resolvable} from "../resolve/module";
 import {TransitionService} from "./transitionService";
 import {ViewConfig} from "../view/interface";
+import {Rejection} from "./rejectFactory";
 
 
-let transitionCount = 0, REJECT = new RejectFactory();
+let transitionCount = 0;
 const stateSelf: (_state: State) => StateDeclaration = prop("self");
 
 /**
@@ -375,8 +376,9 @@ export class Transition implements IHookRegistry {
 
     let syncResult = runSynchronousHooks(hookBuilder.getOnBeforeHooks());
 
-    if (TransitionHook.isRejection(syncResult)) {
-      let rejectReason = (<any> syncResult).reason;
+    if (Rejection.isTransitionRejectionPromise(syncResult)) {
+      syncResult.catch(() => 0); // issue #2676
+      let rejectReason = (<any> syncResult)._transitionRejection;
       this._deferred.reject(rejectReason);
       return this.promise;
     }
@@ -389,8 +391,7 @@ export class Transition implements IHookRegistry {
 
     if (this.ignored()) {
       trace.traceTransitionIgnored(this);
-      let ignored = REJECT.ignored();
-      this._deferred.reject(ignored.reason);
+      this._deferred.reject(Rejection.ignored());
       return this.promise;
     }
 
@@ -410,7 +411,7 @@ export class Transition implements IHookRegistry {
 
     trace.traceTransitionStart(this);
 
-    let chain = hookBuilder.asyncHooks().reduce((_chain, step) => _chain.then(step.invokeStep), syncResult);
+    let chain = hookBuilder.asyncHooks().reduce((_chain, step) => _chain.then(step.invokeHook.bind(step)), syncResult);
     chain.then(resolve, reject);
 
     return this.promise;

src/transition/transitionHook.ts

@@ -7,12 +7,10 @@ import {not, pattern, val, eq, is, parse } from "../common/hof";
 import {trace} from "../common/trace";
 import {services} from "../common/coreservices";
 
-import {TransitionRejection, RejectFactory} from "./rejectFactory";
+import {Rejection} from "./rejectFactory";
 import {TargetState} from "../state/module";
 import {ResolveContext} from "../resolve/module";
 
-let REJECT = new RejectFactory();
-
 let defaultOptions: TransitionHookOptions = {
   async: true,
   rejectIfSuperseded: true,
@@ -32,43 +30,45 @@ export class TransitionHook {
 
   private isSuperseded = () => this.options.current() !== this.options.transition;
 
-  /**
-   * Handles transition abort and transition redirect. Also adds any returned resolvables
-   * to the pathContext for the current pathElement.  If the transition is rejected, then a rejected
-   * promise is returned here, otherwise undefined is returned.
-   */
-  mapHookResult: Function = pattern([
-    // Transition is no longer current
-    [this.isSuperseded, () => REJECT.superseded(this.options.current())],
-    // If the hook returns false, abort the current Transition
-    [eq(false),         () => REJECT.aborted("Hook aborted transition")],
-    // If the hook returns a Transition, halt the current Transition and redirect to that Transition.
-    [is(TargetState),   (target) => REJECT.redirected(target)],
-    // A promise was returned, wait for the promise and then chain another hookHandler
-    [isPromise,         (promise) => promise.then(this.handleHookResult.bind(this))]
-  ]);
-
-  invokeStep = (moreLocals) => { // bind to this
+  invokeHook(moreLocals) {
     let { options, fn, resolveContext } = this;
     let locals = extend({}, this.locals, moreLocals);
     trace.traceHookInvocation(this, options);
     if (options.rejectIfSuperseded && this.isSuperseded()) {
-      return REJECT.superseded(options.current());
+      return Rejection.superseded(options.current()).toPromise();
     }
 
     // TODO: Need better integration of returned promises in synchronous code.
     if (!options.async) {
       let hookResult = resolveContext.invokeNow(fn, locals, options);
       return this.handleHookResult(hookResult);
     }
-    return resolveContext.invokeLater(fn, locals, options).then(this.handleHookResult.bind(this));
+    return resolveContext.invokeLater(fn, locals, options).then(val => this.handleHookResult(val));
   };
 
-  handleHookResult(hookResult) {
+  /**
+   * This method handles the return value of a Transition Hook.
+   *
+   * A hook can return false, a redirect (TargetState), or a promise (which may resolve to false or a redirect)
+   */
+  handleHookResult(hookResult): Promise<any> {
     if (!isDefined(hookResult)) return undefined;
-    trace.traceHookResult(hookResult, undefined, this.options);
 
-    let transitionResult = this.mapHookResult(hookResult);
+    /**
+     * Handles transition superseded, transition aborted and transition redirect.
+     */
+    const mapHookResult = pattern([
+      // Transition is no longer current
+      [this.isSuperseded, () => Rejection.superseded(this.options.current()).toPromise()],
+      // If the hook returns false, abort the current Transition
+      [eq(false),         () => Rejection.aborted("Hook aborted transition").toPromise()],
+      // If the hook returns a Transition, halt the current Transition and redirect to that Transition.
+      [is(TargetState),   (target) => Rejection.redirected(target).toPromise()],
+      // A promise was returned, wait for the promise and then chain another hookHandler
+      [isPromise,         (promise) => promise.then(this.handleHookResult.bind(this))]
+    ]);
+
+    let transitionResult = mapHookResult(hookResult);
     if (transitionResult) trace.traceHookResult(hookResult, transitionResult, this.options);
 
     return transitionResult;
@@ -92,24 +92,21 @@ export class TransitionHook {
     let results = [];
     for (let i = 0; i < hooks.length; i++) {
       try {
-        results.push(hooks[i].invokeStep(locals));
+        results.push(hooks[i].invokeHook(locals));
       } catch (exception) {
-        if (!swallowExceptions) return REJECT.aborted(exception);
-        console.log("Swallowed exception during synchronous hook handler: " + exception); // TODO: What to do here?
+        if (!swallowExceptions) {
+          return Rejection.aborted(exception).toPromise();
+        }
+
+        console.error("Swallowed exception during synchronous hook handler: " + exception); // TODO: What to do here?
       }
     }
 
-    let rejections = results.filter(TransitionHook.isRejection);
+    let rejections = results.filter(Rejection.isTransitionRejectionPromise);
     if (rejections.length) return rejections[0];
 
     return results
-        .filter(not(TransitionHook.isRejection))
         .filter(<Predicate<any>> isPromise)
         .reduce((chain, promise) => chain.then(val(promise)), services.$q.when());
   }
-
-
-  static isRejection(hookResult) {
-    return hookResult && hookResult.reason instanceof TransitionRejection && hookResult;
-  }
 }