fix(router): avoid in-guard redirect in new navigation happened

diff --git a/__tests__/router.spec.js b/__tests__/router.spec.js
index 8c8ae1ebf91806e7d7d50a06db6ff7adf8f63174..224748f6d2ed3ce57b081aaf415da9669b07f3e8 100644 (file)
--- a/__tests__/router.spec.js
+++ b/__tests__/router.spec.js
@@ -3,8 +3,9 @@ require('./helper')
 const expect = require('expect')
 const fakePromise = require('faked-promise')
 const { HTML5History } = require('../src/history/html5')
+const { AbstractHistory } = require('../src/history/abstract')
 const { Router } = require('../src/router')
-const { createDom, components } = require('./utils')
+const { createDom, components, tick } = require('./utils')
 
 function mockHistory() {
   // TODO: actually do a mock
@@ -76,7 +77,7 @@ describe('Router', () => {
   })
 
   describe('navigation', () => {
-    it('waits before navigating in an array of beforeEnter', async () => {
+    it('cancels pending navigations if a newer one is finished on push', async () => {
       const [p1, r1] = fakePromise()
       const [p2, r2] = fakePromise()
       const history = mockHistory()
@@ -87,6 +88,7 @@ describe('Router', () => {
             path: '/a',
             component: components.Home,
             async beforeEnter(to, from, next) {
+              expect(from.fullPath).toBe('/')
               await p1
               next()
             },
@@ -96,6 +98,7 @@ describe('Router', () => {
             component: components.Foo,
             name: 'Foo',
             async beforeEnter(to, from, next) {
+              expect(from.fullPath).toBe('/')
               await p2
               next()
             },
@@ -117,6 +120,87 @@ describe('Router', () => {
       }
       expect(router.currentRoute.fullPath).toBe('/b')
     })
+
+    it('cancels pending navigations if a newer one is finished on user navigation (from history)', async () => {
+      const [p1, r1] = fakePromise()
+      const [p2, r2] = fakePromise()
+      const history = new AbstractHistory()
+      const router = new Router({ history, routes })
+      // navigate first to add entries to the history stack
+      await router.push('/p/initial')
+      await router.push('/p/a')
+      await router.push('/p/b')
+
+      router.beforeEach(async (to, from, next) => {
+        if (to.fullPath === '/p/initial') {
+          // because we delay navigation, we are coming from /p/b
+          expect(from.fullPath).toBe('/p/b')
+          await p1
+        } else {
+          expect(from.fullPath).toBe('/p/b')
+          await p2
+        }
+        next()
+      })
+
+      // trigger to history.back()
+      history.back()
+      history.back()
+
+      expect(router.currentRoute.fullPath).toBe('/p/b')
+      // resolves the last call to history.back() first
+      // so we end up on /p/initial
+      r1()
+      await tick()
+      expect(router.currentRoute.fullPath).toBe('/p/initial')
+      // resolves the pending navigation, this should be cancelled
+      r2()
+      await tick()
+      expect(router.currentRoute.fullPath).toBe('/p/initial')
+    })
+
+    it('cancels pending in-guard navigations if a newer one is finished on user navigation (from history)', async () => {
+      const [p1, r1] = fakePromise()
+      const [p2, r2] = fakePromise()
+      const history = new AbstractHistory()
+      const router = new Router({ history, routes })
+      // navigate first to add entries to the history stack
+      await router.push('/p/initial')
+      await router.push('/p/a')
+      await router.push('/p/b')
+
+      router.beforeEach(async (to, from, next) => {
+        console.log('going to', to.fullPath, 'from', from.fullPath)
+        if (to.fullPath === '/p/initial') {
+          console.log('waiting for p1')
+          await p1
+          console.log('done with p1')
+          next()
+        } else if (from.fullPath === '/p/b') {
+          console.log('waiting for p2')
+          await p2
+          console.log('done with p2')
+          next('/p/other-place')
+        } else {
+          next()
+        }
+      })
+
+      // trigger to history.back()
+      history.back()
+      history.back()
+
+      expect(router.currentRoute.fullPath).toBe('/p/b')
+      // resolves the last call to history.back() first
+      // so we end up on /p/initial
+      r1()
+      await tick()
+      expect(router.currentRoute.fullPath).toBe('/p/initial')
+      // resolves the pending navigation, this should be cancelled
+      r2()
+      await tick()
+      expect(router.currentRoute.fullPath).toBe('/p/initial')
+    })
   })
 
   describe('matcher', () => {

diff --git a/src/router.ts b/src/router.ts
index a9ee23d554fd63397800afb87d1d0a2550e76543..dd7c8b7497b6a846034d45955ab29c407c69c2ac 100644 (file)
--- a/src/router.ts
+++ b/src/router.ts
@@ -30,6 +30,8 @@ export interface RouterOptions {
   routes: RouteRecord[]
 }
 
+type ErrorHandler = (error: any) => any
+
 export class Router {
   protected history: BaseHistory
   private matcher: RouterMatcher
@@ -38,6 +40,7 @@ export class Router {
   currentRoute: Readonly<RouteLocationNormalized> = START_LOCATION_NORMALIZED
   pendingLocation: Readonly<RouteLocationNormalized> = START_LOCATION_NORMALIZED
   private app: any
+  private errorHandlers: ErrorHandler[] = []
 
   constructor(options: RouterOptions) {
     this.history = options.history
@@ -50,10 +53,16 @@ export class Router {
       // console.log({ to, matchedRoute })
 
       const toLocation: RouteLocationNormalized = { ...to, ...matchedRoute }
+      this.pendingLocation = toLocation
 
       try {
         await this.navigate(toLocation, this.currentRoute)
 
+        // a more recent navigation took place
+        if (this.pendingLocation !== toLocation) {
+          throw new NavigationCancelled(toLocation, this.currentRoute)
+        }
+
         // accept current navigation
         this.currentRoute = {
           ...to,
@@ -64,6 +73,16 @@ export class Router {
         // TODO: use the push/replace technique with any navigation to
         // preserve history when moving forward
         if (error instanceof NavigationGuardRedirect) {
+          // TODO: refactor the duplication of new NavigationCancelled by
+          // checking instanceof NavigationError (it's another TODO)
+          // a more recent navigation took place
+          if (this.pendingLocation !== toLocation) {
+            return this.triggerError(
+              new NavigationCancelled(toLocation, this.currentRoute)
+            )
+          }
+
+          // TODO: handle errors
           this.push(error.to)
         } else if (error instanceof NavigationAborted) {
           // TODO: test on different browsers ensure consistent behavior
@@ -355,6 +374,25 @@ export class Router {
     }
   }
 
+  /**
+   * Add an error handler to catch errors during navigation
+   * TODO: return a remover like beforeEach
+   * @param handler error handler
+   */
+  onError(handler: ErrorHandler): void {
+    this.errorHandlers.push(handler)
+  }
+
+  /**
+   * Trigger all registered error handlers
+   * @param error thrown error
+   */
+  private triggerError(error: any): void {
+    for (const handler of this.errorHandlers) {
+      handler(error)
+    }
+  }
+
   private updateReactiveRoute() {
     if (!this.app) return
     // TODO: matched should be non enumerable and the defineProperty here shouldn't be necessary