fix: trigger redirect on popstate (#592)

vuejs · Nov 13, 2020 · 18dbdc2 · 18dbdc2
1 parent 30d2d26
commit 18dbdc2
Show file tree

Hide file tree

Showing 4 changed files with 74 additions and 48 deletions.
diff --git a/__tests__/warnings.spec.ts b/__tests__/warnings.spec.ts
@@ -15,7 +15,9 @@ describe('warnings', () => {
         { path: '/redirect', redirect: { params: { foo: 'f' } } },
       ],
     })
-    await router.push('/redirect').catch(() => {})
+    try {
+      await router.push('/redirect')
+    } catch (err) {}
     expect('Invalid redirect found').toHaveBeenWarned()
   })
 

diff --git a/e2e/hash/index.ts b/e2e/hash/index.ts
@@ -23,6 +23,7 @@ const router = createRouter({
   history: createWebHashHistory('/' + __dirname + '/#/'),
   routes: [
     { path: '/', component: Home },
+    { path: '/redirect', name: 'redirect', redirect: '/foo' },
     { path: '/foo', component: Foo },
     { path: '/bar', component: Bar },
     { path: '/unicode/:id', name: 'unicode', component: Unicode },

diff --git a/e2e/specs/hash.js b/e2e/specs/hash.js
@@ -92,4 +92,20 @@ module.exports = {
 
       .end()
   },
+
+  /** @type {import('nightwatch').NightwatchTest} */
+  'manual hash change to trigger redirect': function (browser) {
+    browser
+      .url(baseURL + '/')
+      .waitForElementPresent('#app > *', 1000)
+      .assert.containsText('.view', 'home')
+
+      .execute(function () {
+        window.location.hash = '#/redirect'
+      })
+      .assert.containsText('.view', 'Foo')
+      .assert.urlEquals(baseURL + '/foo')
+
+      .end()
+  },
 }
diff --git a/src/router.ts b/src/router.ts
@@ -545,24 +545,13 @@ export function createRouter(options: RouterOptions): Router {
     return push(assign(locationAsObject(to), { replace: true }))
   }
 
-  function pushWithRedirect(
-    to: RouteLocationRaw | RouteLocation,
-    redirectedFrom?: RouteLocation
-  ): Promise<NavigationFailure | void | undefined> {
-    const targetLocation: RouteLocation = (pendingLocation = resolve(to))
-    const from = currentRoute.value
-    const data: HistoryState | undefined = (to as RouteLocationOptions).state
-    const force: boolean | undefined = (to as RouteLocationOptions).force
-    // to could be a string where `replace` is a function
-    const replace = (to as RouteLocationOptions).replace === true
-
-    const lastMatched =
-      targetLocation.matched[targetLocation.matched.length - 1]
+  function handleRedirectRecord(to: RouteLocation): RouteLocationRaw | void {
+    const lastMatched = to.matched[to.matched.length - 1]
     if (lastMatched && lastMatched.redirect) {
       const { redirect } = lastMatched
       // transform it into an object to pass the original RouteLocaleOptions
       let newTargetLocation = locationAsObject(
-        typeof redirect === 'function' ? redirect(targetLocation) : redirect
+        typeof redirect === 'function' ? redirect(to) : redirect
       )
 
       if (
@@ -576,29 +565,42 @@ export function createRouter(options: RouterOptions): Router {
             null,
             2
           )}\n when navigating to "${
-            targetLocation.fullPath
+            to.fullPath
           }". A redirect must contain a name or path. This will break in production.`
         )
-        return Promise.reject(new Error('Invalid redirect'))
+        throw new Error('Invalid redirect')
       }
+
+      return assign(
+        {
+          query: to.query,
+          hash: to.hash,
+          params: to.params,
+        },
+        newTargetLocation
+      )
+    }
+  }
+
+  function pushWithRedirect(
+    to: RouteLocationRaw | RouteLocation,
+    redirectedFrom?: RouteLocation
+  ): Promise<NavigationFailure | void | undefined> {
+    const targetLocation: RouteLocation = (pendingLocation = resolve(to))
+    const from = currentRoute.value
+    const data: HistoryState | undefined = (to as RouteLocationOptions).state
+    const force: boolean | undefined = (to as RouteLocationOptions).force
+    // to could be a string where `replace` is a function
+    const replace = (to as RouteLocationOptions).replace === true
+
+    const shouldRedirect = handleRedirectRecord(targetLocation)
+
+    if (shouldRedirect)
       return pushWithRedirect(
-        assign(
-          {
-            query: targetLocation.query,
-            hash: targetLocation.hash,
-            params: targetLocation.params,
-          },
-          newTargetLocation,
-          {
-            state: data,
-            force,
-            replace,
-          }
-        ),
+        assign(shouldRedirect, { state: data, force, replace }),
         // keep original redirectedFrom if it exists
         redirectedFrom || targetLocation
       )
-    }
 
     // if it was a redirect we already called `pushWithRedirect` above
     const toLocation = targetLocation as RouteLocationNormalized
@@ -616,7 +618,7 @@ export function createRouter(options: RouterOptions): Router {
         from,
         from,
         // this is a push, the only way for it to be triggered from a
-        // history.listen is with a redirect, which makes it become a pus
+        // history.listen is with a redirect, which makes it become a push
         true,
         // This cannot be the first navigation because the initial location
         // cannot be manually navigated to
@@ -625,20 +627,12 @@ export function createRouter(options: RouterOptions): Router {
     }
 
     return (failure ? Promise.resolve(failure) : navigate(toLocation, from))
-      .catch((error: NavigationFailure | NavigationRedirectError) => {
-        if (
-          isNavigationFailure(
-            error,
-            ErrorTypes.NAVIGATION_ABORTED |
-              ErrorTypes.NAVIGATION_CANCELLED |
-              ErrorTypes.NAVIGATION_GUARD_REDIRECT
-          )
-        ) {
-          return error
-        }
-        // unknown error, rejects
-        return triggerError(error)
-      })
+      .catch((error: NavigationFailure | NavigationRedirectError) =>
+        isNavigationFailure(error)
+          ? error
+          : // reject any unknown error
+            triggerError(error)
+      )
       .then((failure: NavigationFailure | NavigationRedirectError | void) => {
         if (failure) {
           if (
@@ -896,7 +890,19 @@ export function createRouter(options: RouterOptions): Router {
   function setupListeners() {
     removeHistoryListener = routerHistory.listen((to, _from, info) => {
       // cannot be a redirect route because it was in history
-      const toLocation = resolve(to) as RouteLocationNormalized
+      let toLocation = resolve(to) as RouteLocationNormalized
+
+      // due to dynamic routing, and to hash history with manual navigation
+      // (manually changing the url or calling history.hash = '#/somewhere'),
+      // there could be a redirect record in history
+      const shouldRedirect = handleRedirectRecord(toLocation)
+      if (shouldRedirect) {
+        pushWithRedirect(
+          assign(shouldRedirect, { replace: true }),
+          toLocation
+        ).catch(noop)
+        return
+      }
 
       pendingLocation = toLocation
       const from = currentRoute.value
@@ -927,9 +933,10 @@ export function createRouter(options: RouterOptions): Router {
             // the error is already handled by router.push we just want to avoid
             // logging the error
             pushWithRedirect(
+              // TODO: should we force replace: true
               (error as NavigationRedirectError).to,
               toLocation
-              // avoid an uncaught rejection
+              // avoid an uncaught rejection, let push call triggerError
             ).catch(noop)
             // avoid the then branch
             return Promise.reject()