feat: refactor navigation to comply with vuejs/rfcs#150

BREAKING CHANGE: This follows the RFC at vuejs/rfcs#150
  Summary: `router.afterEach` and `router.onError` are now the global equivalent of
  `router.push`/`router.replace` as well as navigation through the interface
  (`history.go()`). A navigation only rejects if there was an unexpected error.
  A navigation failure will still resolve the promise returned by `router.push`
  and be exposed as the resolved value.

Author: posva

__tests__/errors.spec.ts

@@ -1,27 +1,17 @@
+import fakePromise from 'faked-promise'
 import { createRouter as newRouter, createMemoryHistory } from '../src'
-import { ErrorTypes } from '../src/errors'
-import { components } from './utils'
-import { RouteRecordRaw } from '../src/types'
+import { NavigationFailure, NavigationFailureType } from '../src/errors'
+import { components, tick } from './utils'
+import { RouteRecordRaw, NavigationGuard, RouteLocationRaw } from '../src/types'
 
 const routes: RouteRecordRaw[] = [
   { path: '/', component: components.Home },
+  { path: '/redirect', redirect: '/' },
   { path: '/foo', component: components.Foo, name: 'Foo' },
-  { path: '/to-foo', redirect: '/foo' },
-  { path: '/to-foo-named', redirect: { name: 'Foo' } },
-  { path: '/to-foo2', redirect: '/to-foo' },
-  { path: '/p/:p', name: 'Param', component: components.Bar },
-  { path: '/to-p/:p', redirect: to => `/p/${to.params.p}` },
-  {
-    path: '/inc-query-hash',
-    redirect: to => ({
-      name: 'Foo',
-      query: { n: to.query.n + '-2' },
-      hash: to.hash + '-2',
-    }),
-  },
 ]
 
 const onError = jest.fn()
+const afterEach = jest.fn()
 function createRouter() {
   const history = createMemoryHistory()
   const router = newRouter({
@@ -30,27 +20,298 @@ function createRouter() {
   })
 
   router.onError(onError)
+  router.afterEach(afterEach)
   return { router, history }
 }
 
-describe('Errors', () => {
+describe('Errors & Navigation failures', () => {
   beforeEach(() => {
     onError.mockReset()
+    afterEach.mockReset()
   })
 
-  it('triggers onError when navigation is aborted', async () => {
+  it('next(false) triggers afterEach', async () => {
+    await testNavigation(
+      false,
+      expect.objectContaining({
+        type: NavigationFailureType.aborted,
+      })
+    )
+  })
+
+  it('next("/location") triggers afterEach', async () => {
+    await testNavigation(
+      ((to, from, next) => {
+        if (to.path === '/location') next()
+        else next('/location')
+      }) as NavigationGuard,
+      undefined
+    )
+  })
+
+  it('redirect triggers afterEach', async () => {
+    await testNavigation(undefined, undefined, '/redirect')
+  })
+
+  it('next() triggers afterEach', async () => {
+    await testNavigation(undefined, undefined)
+  })
+
+  it('next(true) triggers afterEach', async () => {
+    await testNavigation(true, undefined)
+  })
+
+  it('triggers afterEach if a new navigation happens', async () => {
     const { router } = createRouter()
+    const [promise, resolve] = fakePromise()
     router.beforeEach((to, from, next) => {
-      next(false)
+      // let it hang otherwise
+      if (to.path === '/') next()
+      else promise.then(() => next())
     })
 
-    try {
-      await router.push('/foo')
-    } catch (err) {
-      expect(err.type).toBe(ErrorTypes.NAVIGATION_ABORTED)
-    }
-    expect(onError).toHaveBeenCalledWith(
-      expect.objectContaining({ type: ErrorTypes.NAVIGATION_ABORTED })
+    let from = router.currentRoute.value
+
+    // should hang
+    let navigationPromise = router.push('/foo')
+
+    await expect(router.push('/')).resolves.toEqual(undefined)
+    expect(afterEach).toHaveBeenCalledTimes(1)
+    expect(onError).toHaveBeenCalledTimes(0)
+
+    resolve()
+    await navigationPromise
+    expect(afterEach).toHaveBeenCalledTimes(2)
+    expect(onError).toHaveBeenCalledTimes(0)
+
+    expect(afterEach).toHaveBeenLastCalledWith(
+      expect.objectContaining({ path: '/foo' }),
+      from,
+      expect.objectContaining({ type: NavigationFailureType.cancelled })
     )
   })
+
+  it('next(new Error()) triggers onError', async () => {
+    let error = new Error()
+    await testError(error, error)
+  })
+
+  it('triggers onError with thrown errors', async () => {
+    let error = new Error()
+    await testError(() => {
+      throw error
+    }, error)
+  })
+
+  it('triggers onError with rejected promises', async () => {
+    let error = new Error()
+    await testError(async () => {
+      throw error
+    }, error)
+  })
+
+  describe('history navigation', () => {
+    it('triggers afterEach with history.back', async () => {
+      const { router, history } = createRouter()
+
+      await router.push('/')
+      await router.push('/foo')
+
+      afterEach.mockReset()
+      onError.mockReset()
+
+      const [promise, resolve] = fakePromise()
+      router.beforeEach((to, from, next) => {
+        // let it hang otherwise
+        if (to.path === '/') next()
+        else promise.then(() => next())
+      })
+
+      let from = router.currentRoute.value
+
+      // should hang
+      let navigationPromise = router.push('/bar')
+
+      // goes from /foo to /
+      history.go(-1)
+
+      await tick()
+
+      expect(afterEach).toHaveBeenCalledTimes(1)
+      expect(onError).toHaveBeenCalledTimes(0)
+      expect(afterEach).toHaveBeenLastCalledWith(
+        expect.objectContaining({ path: '/' }),
+        from,
+        undefined
+      )
+
+      resolve()
+      await expect(navigationPromise).resolves.toEqual(
+        expect.objectContaining({ type: NavigationFailureType.cancelled })
+      )
+
+      expect(afterEach).toHaveBeenCalledTimes(2)
+      expect(onError).toHaveBeenCalledTimes(0)
+
+      expect(afterEach).toHaveBeenLastCalledWith(
+        expect.objectContaining({ path: '/bar' }),
+        from,
+        expect.objectContaining({ type: NavigationFailureType.cancelled })
+      )
+    })
+
+    it('next(false) triggers afterEach with history.back', async () => {
+      await testHistoryNavigation(
+        false,
+        expect.objectContaining({ type: NavigationFailureType.aborted })
+      )
+    })
+
+    it('next("/location") triggers afterEach with history.back', async () => {
+      await testHistoryNavigation(
+        ((to, from, next) => {
+          if (to.path === '/location') next()
+          else next('/location')
+        }) as NavigationGuard,
+        undefined
+      )
+    })
+
+    it('next() triggers afterEach with history.back', async () => {
+      await testHistoryNavigation(undefined, undefined)
+    })
+
+    it('next(true) triggers afterEach with history.back', async () => {
+      await testHistoryNavigation(true, undefined)
+    })
+
+    it('next(new Error()) triggers onError with history.back', async () => {
+      let error = new Error()
+      await testHistoryError(error, error)
+    })
+
+    it('triggers onError with thrown errors with history.back', async () => {
+      let error = new Error()
+      await testHistoryError(() => {
+        throw error
+      }, error)
+    })
+
+    it('triggers onError with rejected promises with history.back', async () => {
+      let error = new Error()
+      await testHistoryError(async () => {
+        throw error
+      }, error)
+    })
+  })
 })
+
+async function testError(
+  nextArgument: any | NavigationGuard,
+  expectedError: Error | void = undefined,
+  to: RouteLocationRaw = '/foo'
+) {
+  const { router } = createRouter()
+  router.beforeEach(
+    typeof nextArgument === 'function'
+      ? nextArgument
+      : (to, from, next) => {
+          next(nextArgument)
+        }
+  )
+
+  await expect(router.push(to)).rejects.toEqual(expectedError)
+
+  expect(afterEach).toHaveBeenCalledTimes(0)
+  expect(onError).toHaveBeenCalledTimes(1)
+
+  expect(onError).toHaveBeenCalledWith(expectedError)
+}
+
+async function testNavigation(
+  nextArgument: any | NavigationGuard,
+  expectedFailure: NavigationFailure | void = undefined,
+  to: RouteLocationRaw = '/foo'
+) {
+  const { router } = createRouter()
+  router.beforeEach(
+    typeof nextArgument === 'function'
+      ? nextArgument
+      : (to, from, next) => {
+          next(nextArgument)
+        }
+  )
+
+  await expect(router.push(to)).resolves.toEqual(expectedFailure)
+
+  expect(afterEach).toHaveBeenCalledTimes(1)
+  expect(onError).toHaveBeenCalledTimes(0)
+
+  expect(afterEach).toHaveBeenCalledWith(
+    expect.any(Object),
+    expect.any(Object),
+    expectedFailure
+  )
+}
+
+async function testHistoryNavigation(
+  nextArgument: any | NavigationGuard,
+  expectedFailure: NavigationFailure | void = undefined,
+  to: RouteLocationRaw = '/foo'
+) {
+  const { router, history } = createRouter()
+  await router.push(to)
+
+  router.beforeEach(
+    typeof nextArgument === 'function'
+      ? nextArgument
+      : (to, from, next) => {
+          next(nextArgument)
+        }
+  )
+
+  afterEach.mockReset()
+  onError.mockReset()
+
+  history.go(-1)
+
+  await tick()
+
+  expect(afterEach).toHaveBeenCalledTimes(1)
+  expect(onError).toHaveBeenCalledTimes(0)
+
+  expect(afterEach).toHaveBeenCalledWith(
+    expect.any(Object),
+    expect.any(Object),
+    expectedFailure
+  )
+}
+
+async function testHistoryError(
+  nextArgument: any | NavigationGuard,
+  expectedError: Error | void = undefined,
+  to: RouteLocationRaw = '/foo'
+) {
+  const { router, history } = createRouter()
+  await router.push(to)
+
+  router.beforeEach(
+    typeof nextArgument === 'function'
+      ? nextArgument
+      : (to, from, next) => {
+          next(nextArgument)
+        }
+  )
+
+  afterEach.mockReset()
+  onError.mockReset()
+
+  history.go(-1)
+
+  await tick()
+
+  expect(afterEach).toHaveBeenCalledTimes(0)
+  expect(onError).toHaveBeenCalledTimes(1)
+
+  expect(onError).toHaveBeenCalledWith(expectedError)
+}

__tests__/guards/global-after.spec.ts

@@ -31,7 +31,8 @@ describe('router.afterEach', () => {
     expect(spy).toHaveBeenCalledTimes(1)
     expect(spy).toHaveBeenCalledWith(
       expect.objectContaining({ fullPath: '/foo' }),
-      expect.objectContaining({ fullPath: '/' })
+      expect.objectContaining({ fullPath: '/' }),
+      undefined
     )
   })
 
@@ -44,7 +45,7 @@ describe('router.afterEach', () => {
     expect(spy).not.toHaveBeenCalled()
   })
 
-  it('calls afterEach guards on push', async () => {
+  it('calls afterEach guards on multiple push', async () => {
     const spy = jest.fn()
     const router = createRouter({ routes })
     await router.push('/nested')
@@ -53,24 +54,15 @@ describe('router.afterEach', () => {
     expect(spy).toHaveBeenCalledTimes(1)
     expect(spy).toHaveBeenLastCalledWith(
       expect.objectContaining({ name: 'nested-home' }),
-      expect.objectContaining({ name: 'nested-default' })
+      expect.objectContaining({ name: 'nested-default' }),
+      undefined
     )
     await router.push('/nested')
     expect(spy).toHaveBeenLastCalledWith(
       expect.objectContaining({ name: 'nested-default' }),
-      expect.objectContaining({ name: 'nested-home' })
+      expect.objectContaining({ name: 'nested-home' }),
+      undefined
     )
     expect(spy).toHaveBeenCalledTimes(2)
   })
-
-  it('does not call afterEach if navigation is cancelled', async () => {
-    const spy = jest.fn()
-    const router = createRouter({ routes })
-    router.afterEach(spy)
-    router.beforeEach((to, from, next) => {
-      next(false) // cancel the navigation
-    })
-    await router.push('/foo').catch(err => {}) // ignore the error
-    expect(spy).not.toHaveBeenCalled()
-  })
 })