Skip to content

Commit

Permalink
feat(errors): create router errors (#3047)
Browse files Browse the repository at this point in the history
* test: add example for navigation errors

* test: add unit tests for navigation errors

* feat(errors): create router errors
  • Loading branch information
lmichelin authored May 19, 2020
1 parent 4c81be8 commit 4c727f9
Show file tree
Hide file tree
Showing 8 changed files with 199 additions and 36 deletions.
1 change: 1 addition & 0 deletions examples/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ <h1>Vue Router Examples</h1>
<li><a href="route-props">Route Props</a></li>
<li><a href="route-alias">Route Alias</a></li>
<li><a href="route-params">Route Params</a></li>
<li><a href="router-errors">Router errors</a></li>
<li><a href="transitions">Transitions</a></li>
<li><a href="data-fetching">Data Fetching</a></li>
<li><a href="navigation-guards">Navigation Guards</a></li>
Expand Down
53 changes: 53 additions & 0 deletions examples/router-errors/app.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import Vue from 'vue'
import VueRouter from 'vue-router'

const component = {
template: `
<div>
{{ $route.fullPath }}
</div>
`
}

Vue.use(VueRouter)

const router = new VueRouter({
routes: [
{ path: '/', component }, { path: '/foo', component }
]
})

router.beforeEach((to, from, next) => {
console.log('from', from.fullPath)
console.log('going to', to.fullPath)
if (to.query.wait) {
setTimeout(() => next(), 100)
} else if (to.query.redirect) {
next(to.query.redirect)
} else if (to.query.abort) {
next(false)
} else {
next()
}
})

new Vue({
el: '#app',
router
})

// 4 NAVIGATION ERROR CASES :

// navigation duplicated
// router.push('/foo')
// router.push('/foo')

// navigation cancelled
// router.push('/foo?wait=y')
// router.push('/')

// navigation redirected
// router.push('/foo?redirect=/')

// navigation aborted
// router.push('/foo?abort=y')
8 changes: 8 additions & 0 deletions examples/router-errors/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<!DOCTYPE html>
<div id="app">
<router-link to="/">/</router-link>
<router-link to="/foo">/foo</router-link>
<router-view></router-view>
</div>
<script src="/__build__/shared.chunk.js"></script>
<script src="/__build__/router-errors.js"></script>
6 changes: 3 additions & 3 deletions src/history/abstract.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

import type Router from '../index'
import { History } from './base'
import { NavigationDuplicated } from './errors'
import { isExtendedError } from '../util/warn'
import { isRouterError } from '../util/warn'
import { NavigationFailureType } from './errors'

export class AbstractHistory extends History {
index: number
Expand Down Expand Up @@ -51,7 +51,7 @@ export class AbstractHistory extends History {
this.updateRoute(route)
},
err => {
if (isExtendedError(NavigationDuplicated, err)) {
if (isRouterError(err, NavigationFailureType.NAVIGATION_DUPLICATED)) {
this.index = targetIndex
}
}
Expand Down
23 changes: 16 additions & 7 deletions src/history/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,20 @@ import { _Vue } from '../install'
import type Router from '../index'
import { inBrowser } from '../util/dom'
import { runQueue } from '../util/async'
import { warn, isError, isExtendedError } from '../util/warn'
import { warn, isError, isRouterError } from '../util/warn'
import { START, isSameRoute } from '../util/route'
import {
flatten,
flatMapComponents,
resolveAsyncComponents
} from '../util/resolve-components'
import { NavigationDuplicated } from './errors'
import {
createNavigationDuplicatedError,
createNavigationCancelledError,
createNavigationRedirectedError,
createNavigationAbortedError,
NavigationFailureType
} from './errors'

export class History {
router: Router
Expand Down Expand Up @@ -108,7 +114,7 @@ export class History {
// When the user navigates through history through back/forward buttons
// we do not want to throw the error. We only throw it if directly calling
// push/replace. That's why it's not included in isError
if (!isExtendedError(NavigationDuplicated, err) && isError(err)) {
if (!isRouterError(err, NavigationFailureType.NAVIGATION_DUPLICATED) && isError(err)) {
if (this.errorCbs.length) {
this.errorCbs.forEach(cb => {
cb(err)
Expand All @@ -126,7 +132,7 @@ export class History {
route.matched.length === current.matched.length
) {
this.ensureURL()
return abort(new NavigationDuplicated(route))
return abort(createNavigationDuplicatedError(current, route))
}

const { updated, deactivated, activated } = resolveQueue(
Expand All @@ -150,12 +156,15 @@ export class History {
this.pending = route
const iterator = (hook: NavigationGuard, next) => {
if (this.pending !== route) {
return abort()
return abort(createNavigationCancelledError(current, route))
}
try {
hook(route, current, (to: any) => {
if (to === false || isError(to)) {
if (to === false) {
// next(false) -> abort navigation, ensure current URL
this.ensureURL(true)
abort(createNavigationAbortedError(current, route))
} else if (isError(to)) {
this.ensureURL(true)
abort(to)
} else if (
Expand All @@ -164,7 +173,7 @@ export class History {
(typeof to.path === 'string' || typeof to.name === 'string'))
) {
// next('/') or next({ path: '/' }) -> redirect
abort()
abort(createNavigationRedirectedError(current, route))

This comment has been minimized.

Copy link
@nialldorgan

nialldorgan Jul 21, 2020

This now appears to create a browser navigation error on every redirect, for example if a user is redirected to a login page this will generate an error this surely is not the intention here

This comment has been minimized.

Copy link
@soletan

soletan Sep 11, 2020

It is breaking VuePress builds due to breaking SSR as well. The way vue-router should be integrated with SSR is obsolete by now for the onReady won't be invoked as soon as some guard is redirecting the user.

This comment has been minimized.

Copy link
@posva

posva Sep 11, 2020

Member

@soletan please don't create false alarms like that without providing reproductions: https://jsfiddle.net/5pwu38m9/

This comment has been minimized.

Copy link
@soletan

soletan Sep 11, 2020

Not every issue fits in a JSFiddle ... but here you are: https://github.com/soletan/vue-router-test

  1. git checkout 0c5d
  2. yarn install --frozen-lockfile
  3. npm run build --> fails
  4. git checkout HEAD
  5. yarn install -frozen-lockfile
  6. npm run build --> succeeds

Difference: in 4 and 5 vuepress is forced to use vue-router 3.1.6 instead of 3.4.3 matching its selector ^3.1.3. Since everything is kept as before the only thing changing is your library. And its breaking a software that assumes when using ^3.1.3 for selecting dependency there are no breaking changes in API ... don't know why its breaking in full detail, but there is no other difference but the version of your backward-compatible library.

if (typeof to === 'object' && to.replace) {
this.replace(to)
} else {
Expand Down
85 changes: 65 additions & 20 deletions src/history/errors.js
Original file line number Diff line number Diff line change
@@ -1,22 +1,67 @@
export class NavigationDuplicated extends Error {
constructor (normalizedLocation) {
super()
this.name = this._name = 'NavigationDuplicated'
// passing the message to super() doesn't seem to work in the transpiled version
this.message = `Navigating to current location ("${
normalizedLocation.fullPath
}") is not allowed`
// add a stack property so services like Sentry can correctly display it
Object.defineProperty(this, 'stack', {
value: new Error().stack,
writable: true,
configurable: true
})
// we could also have used
// Error.captureStackTrace(this, this.constructor)
// but it only exists on node and chrome
}
export const NavigationFailureType = {
redirected: 1,
aborted: 2,
cancelled: 3,
duplicated: 4
}

export function createNavigationRedirectedError (from, to) {
return createRouterError(
from,
to,
NavigationFailureType.redirected,
`Redirected from "${from.fullPath}" to "${stringifyRoute(to)}" via a navigation guard.`
)
}

export function createNavigationDuplicatedError (from, to) {
return createRouterError(
from,
to,
NavigationFailureType.duplicated,
`Avoided redundant navigation to current location: "${from.fullPath}".`
)
}

export function createNavigationCancelledError (from, to) {
return createRouterError(
from,
to,
NavigationFailureType.cancelled,
`Navigation cancelled from "${from.fullPath}" to "${to.fullPath}" with a new navigation.`
)
}

// support IE9
NavigationDuplicated._name = 'NavigationDuplicated'
export function createNavigationAbortedError (from, to) {
return createRouterError(
from,
to,
NavigationFailureType.aborted,
`Navigation aborted from "${from.fullPath}" to "${to.fullPath}" via a navigation guard.`
)
}

function createRouterError (from, to, type, message) {
const error = new Error(message)
error._isRouter = true
error.from = from
error.to = to
error.type = type

const newStack = error.stack.split('\n')
newStack.splice(1, 2) // remove 2 last useless calls
error.stack = newStack.join('\n')
return error
}

const propertiesToLog = ['params', 'query', 'hash']

function stringifyRoute (to) {
if (typeof to === 'string') return to
if ('path' in to) return to.path
const location = {}
for (const key of propertiesToLog) {
if (key in to) location[key] = to[key]
}
return JSON.stringify(location, null, 2)
}
8 changes: 2 additions & 6 deletions src/util/warn.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,6 @@ export function isError (err: any): boolean {
return Object.prototype.toString.call(err).indexOf('Error') > -1
}

export function isExtendedError (constructor: Function, err: any): boolean {
return (
err instanceof constructor ||
// _name is to support IE9 too
(err && (err.name === constructor.name || err._name === constructor._name))
)
export function isRouterError (err: any, errorType: ?string): boolean {
return isError(err) && err._isRouter && (errorType == null || err.type === errorType)
}
51 changes: 51 additions & 0 deletions test/unit/specs/error-handling.spec.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import Vue from 'vue'
import VueRouter from '../../../src/index'
import { NavigationFailureType } from '../../../src/history/errors'

Vue.use(VueRouter)

Expand Down Expand Up @@ -40,6 +41,56 @@ describe('error handling', () => {
})
})

it('NavigationDuplicated error', done => {
const router = new VueRouter()

router.push('/foo')
router.push('/foo').catch(err => {
expect(err.type).toBe(NavigationFailureType.duplicated)
done()
})
})

it('NavigationCancelled error', done => {
const router = new VueRouter()

router.beforeEach((to, from, next) => {
setTimeout(() => next(), 100)
})

router.push('/foo').catch(err => {
expect(err.type).toBe(NavigationFailureType.cancelled)
done()
})
router.push('/')
})

it('NavigationRedirected error', done => {
const router = new VueRouter()

router.beforeEach((to, from, next) => {
if (to.query.redirect) {
next(to.query.redirect)
}
})

router.push('/foo?redirect=/').catch(err => {
expect(err.type).toBe(NavigationFailureType.redirected)
done()
})
})

it('NavigationAborted error', done => {
const router = new VueRouter()

router.beforeEach((to, from, next) => { next(false) })

router.push('/foo').catch(err => {
expect(err.type).toBe(NavigationFailureType.aborted)
done()
})
})

it('async component errors', done => {
spyOn(console, 'warn')
const err = new Error('foo')
Expand Down

0 comments on commit 4c727f9

Please sign in to comment.