refactor data hook resolution

yyx990803 · yyx990803 · commit b2ed17afabfd · 2016-01-25T01:27:53.000-05:00
diff --git a/src/pipeline.js b/src/pipeline.js
@@ -224,7 +224,10 @@ export function activate (view, transition, depth, cb, reuse) {
   }
 
   if (activateHook) {
-    transition.callHooks(activateHook, component, afterActivate, { cleanup })
+    transition.callHooks(activateHook, component, afterActivate, {
+      cleanup,
+      postActivate: true
+    })
   } else {
     afterActivate()
   }
@@ -257,46 +260,45 @@ export function reuse (view, transition) {
 
 function loadData (component, transition, hook, cb, cleanup) {
   component.$loadingRouteData = true
-  transition.callHooks(hook, component, (data, onError) => {
-    // merge data from multiple data hooks
-    if (Array.isArray(data) && data._needMerge) {
-      data = data.reduce((res, obj) => {
-        if (isPlainObject(obj)) {
-          Object.keys(obj).forEach(key => {
-            res[key] = obj[key]
-          })
-        }
-        return res
-      }, Object.create(null))
-    }
-    // handle promise sugar syntax
-    const promises = []
-    if (isPlainObject(data)) {
-      Object.keys(data).forEach(key => {
-        const val = data[key]
-        if (isPromise(val)) {
-          promises.push(val.then(resolvedVal => {
-            component.$set(key, resolvedVal)
-          }))
-        } else {
-          component.$set(key, val)
-        }
-      })
-    }
-    if (!promises.length) {
-      component.$loadingRouteData = false
-      component.$emit('route-data-loaded', component)
-      cb && cb()
-    } else {
-      promises[0].constructor.all(promises).then(() => {
+  transition.callHooks(hook, component, cb, {
+    cleanup,
+    postActivate: true,
+    processData: (data) => {
+      // merge data from multiple data hooks
+      if (Array.isArray(data) && data._needMerge) {
+        data = data.reduce((res, obj) => {
+          if (isPlainObject(obj)) {
+            Object.keys(obj).forEach(key => {
+              res[key] = obj[key]
+            })
+          }
+          return res
+        }, Object.create(null))
+      }
+      // handle promise sugar syntax
+      const promises = []
+      if (isPlainObject(data)) {
+        Object.keys(data).forEach(key => {
+          const val = data[key]
+          if (isPromise(val)) {
+            promises.push(val.then(resolvedVal => {
+              component.$set(key, resolvedVal)
+            }))
+          } else {
+            component.$set(key, val)
+          }
+        })
+      }
+      const onDataLoaded = () => {
         component.$loadingRouteData = false
         component.$emit('route-data-loaded', component)
-        cb && cb()
-      }, onError)
+      }
+      if (!promises.length) {
+        onDataLoaded()
+      } else {
+        return promises[0].constructor.all(promises).then(onDataLoaded)
+      }
     }
-  }, {
-    cleanup: cleanup,
-    expectData: true
   })
 }
 
diff --git a/src/transition.js b/src/transition.js
@@ -190,13 +190,15 @@ export default class RouteTransition {
    * @param {Function} [cb]
    * @param {Object} [options]
    *                 - {Boolean} expectBoolean
-   *                 - {Boolean} expectData
+   *                 - {Boolean} postActive
+   *                 - {Function} processData
    *                 - {Function} cleanup
    */
 
   callHook (hook, context, cb, {
     expectBoolean = false,
-    expectData = false,
+    postActivate = false,
+    processData,
     cleanup
   } = {}) {
 
@@ -210,19 +212,30 @@ export default class RouteTransition {
     }
 
     // handle errors
-    const onError = (err) => {
-      // cleanup indicates an after-activation hook,
-      // so instead of aborting we just let the transition
-      // finish.
-      cleanup ? next() : abort()
+    const onError = err => {
+      if (postActivate) {
+        if (!nextCalled) next()
+      } else {
+        abort()
+      }
       if (err && !transition.router._suppress) {
         warn('Uncaught error during transition: ')
         throw err instanceof Error ? err : new Error(err)
       }
     }
 
+    // since promise swallows errors, we have to
+    // throw it in the next tick...
+    const onPromiseError = err => {
+      try {
+        onError(err)
+      } catch (e) {
+        setTimeout(() => { throw e }, 0)
+      }
+    }
+
     // advance the transition to the next step
-    const next = (data) => {
+    const next = () => {
       if (nextCalled) {
         warn('transition.next() should be called only once.')
         return
@@ -232,7 +245,33 @@ export default class RouteTransition {
         cleanup && cleanup()
         return
       }
-      cb && cb(data, onError)
+      cb && cb()
+    }
+
+    const nextWithBoolean = res => {
+      if (typeof res === 'boolean') {
+        res ? next() : abort()
+      } else if (isPromise(res)) {
+        res.then((ok) => {
+          ok ? next() : abort()
+        }, onPromiseError)
+      } else if (!hook.length) {
+        next()
+      }
+    }
+
+    const nextWithData = data => {
+      let res
+      try {
+        res = processData(data)
+      } catch (err) {
+        return onError(err)
+      }
+      if (isPromise(res)) {
+        res.then(next, onPromiseError)
+      } else {
+        next()
+      }
     }
 
     // expose a clone of the transition object, so that each
@@ -242,7 +281,7 @@ export default class RouteTransition {
       to: transition.to,
       from: transition.from,
       abort: abort,
-      next: next,
+      next: processData ? nextWithData : next,
       redirect: function () {
         transition.redirect.apply(transition, arguments)
       }
@@ -256,22 +295,21 @@ export default class RouteTransition {
       return onError(err)
     }
 
-    // handle boolean/promise return values
-    const resIsPromise = isPromise(res)
     if (expectBoolean) {
-      if (typeof res === 'boolean') {
-        res ? next() : abort()
-      } else if (resIsPromise) {
-        res.then((ok) => {
-          ok ? next() : abort()
-        }, onError)
-      } else if (!hook.length) {
-        next(res)
+      // boolean hooks
+      nextWithBoolean(res)
+    } else if (isPromise(res)) {
+      // promise
+      if (processData) {
+        res.then(nextWithData, onPromiseError)
+      } else {
+        res.then(next, onPromiseError)
       }
-    } else if (resIsPromise) {
-      res.then(next, onError)
-    } else if ((expectData && isPlainOjbect(res)) || !hook.length) {
-      next(res)
+    } else if (processData && isPlainOjbect(res)) {
+      // data promise sugar
+      nextWithData(res)
+    } else if (!hook.length) {
+      next()
     }
   }
 
@@ -288,17 +326,15 @@ export default class RouteTransition {
     if (Array.isArray(hooks)) {
       const res = []
       res._needMerge = true
-      let onError
       this.runQueue(hooks, (hook, _, next) => {
         if (!this.aborted) {
-          this.callHook(hook, context, (r, onError) => {
+          this.callHook(hook, context, r => {
             if (r) res.push(r)
-            onError = onError
             next()
           }, options)
         }
       }, () => {
-        cb(res, onError)
+        cb(res)
       })
     } else {
       this.callHook(hooks, context, cb, options)
diff --git a/test/unit/specs/pipeline/data.js b/test/unit/specs/pipeline/data.js
@@ -125,7 +125,7 @@ describe('data', function () {
     })
   })
 
-  it('promise error', function (done) {
+  it('promise reject', function (done) {
     test({
       data: {
         data: function () {
@@ -141,8 +141,8 @@ describe('data', function () {
       assertCalls(calls, ['data.data'])
       expect(router.app.$el.textContent).toBe('loading...')
       setTimeout(function () {
-        // should complete the transition despite error
-        expect(router.app.$el.textContent).toBe('')
+        // should not abort, but nor should finish loading data
+        expect(router.app.$el.textContent).toBe('loading...')
         done()
       }, wait * 2)
     })
@@ -190,8 +190,8 @@ describe('data', function () {
       assertCalls(calls, ['data.data'])
       expect(router.app.$el.textContent).toBe('loading...')
       setTimeout(function () {
-        // should complete the transition despite error
-        expect(router.app.$el.textContent).toBe('')
+        // should not abort, but nor should finish loading data
+        expect(router.app.$el.textContent).toBe('loading...')
         done()
       }, wait * 2)
     })
