Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor error with enum codes #123

Merged
merged 16 commits into from
Mar 18, 2020
15 changes: 10 additions & 5 deletions __tests__/errors.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { createRouter as newRouter, createMemoryHistory } from '../src'
import { NavigationAborted, NavigationGuardRedirect } from '../src/errors'
import { ErrorTypes } from '../src/errors-new'
import { components, tick } from './utils'
import { RouteRecord } from '../src/types'

Expand Down Expand Up @@ -47,9 +47,11 @@ describe('Errors', () => {
try {
await router.push('/foo')
} catch (err) {
expect(err).toBeInstanceOf(NavigationAborted)
expect(err.type).toBe(ErrorTypes.NAVIGATION_ABORTED)
}
expect(onError).toHaveBeenCalledWith(expect.any(NavigationAborted))
expect(onError).toHaveBeenCalledWith(
expect.objectContaining({ type: ErrorTypes.NAVIGATION_ABORTED })
)
})

it('triggers erros caused by new navigations of a next(redirect) trigered by history', async () => {
Expand All @@ -69,12 +71,15 @@ describe('Errors', () => {
expect(onError).toHaveBeenCalledTimes(2)
expect(onError).toHaveBeenNthCalledWith(
1,
expect.any(NavigationGuardRedirect)
expect.objectContaining({ type: ErrorTypes.NAVIGATION_GUARD_REDIRECT })
)
expect(onError.mock.calls[0]).toMatchObject([
{ to: { params: { p: '1' } }, from: { fullPath: '/p/0' } },
])
expect(onError).toHaveBeenNthCalledWith(2, expect.any(NavigationAborted))
expect(onError).toHaveBeenNthCalledWith(
2,
expect.objectContaining({ type: ErrorTypes.NAVIGATION_ABORTED })
)
expect(onError.mock.calls[1]).toMatchObject([
{ to: { params: { p: '1' } }, from: { params: { p: 'other' } } },
])
Expand Down
16 changes: 11 additions & 5 deletions __tests__/matcher/__snapshots__/resolve.spec.ts.snap
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Router Matcher resolve LocationAsName throws if the named route does not exists 1`] = `
[NoRouteMatchError: No match for
{"name":"Home"}]
Object {
"message": "No match for
{\\"name\\":\\"Home\\"}",
"type": "NO_ROUTE_MATCH",
}
`;

exports[`Router Matcher resolve LocationAsRelative redirects throws if relative location when redirecting 1`] = `
Expand All @@ -14,8 +17,11 @@ Use the function redirect and explicitely provide a name]
`;

exports[`Router Matcher resolve LocationAsRelative throws if the current named route does not exists 1`] = `
[NoRouteMatchError: No match for
{"params":{"a":"foo"}}
Object {
"message": "No match for
{\\"params\\":{\\"a\\":\\"foo\\"}}
while being at
{"name":"home","params":{},"path":"/","meta":{}}]
{\\"name\\":\\"home\\",\\"params\\":{},\\"path\\":\\"/\\",\\"meta\\":{}}",
"type": "NO_ROUTE_MATCH",
}
`;
4 changes: 2 additions & 2 deletions __tests__/router.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import fakePromise from 'faked-promise'
import { createRouter, createMemoryHistory, createWebHistory } from '../src'
import { NavigationCancelled } from '../src/errors'
import { ErrorTypes } from '../src/errors-new'
import { createDom, components, tick } from './utils'
import {
RouteRecord,
Expand Down Expand Up @@ -132,7 +132,7 @@ describe('Router', () => {
try {
await pA
} catch (err) {
expect(err).toBeInstanceOf(NavigationCancelled)
expect(err.type).toBe(ErrorTypes.NAVIGATION_CANCELLED)
}
expect(router.currentRoute.value.fullPath).toBe('/p/b')
}
Expand Down
98 changes: 98 additions & 0 deletions src/errors-new.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
import {
MatcherLocation,
MatcherLocationNormalized,
RouteLocation,
RouteLocationNormalized,
} from './types'

// Using string enums because error codes are exposed to developers
// and number enums could collide with other error codes in runtime
export enum ErrorTypes {
NO_ROUTE_MATCH = 'NO_ROUTE_MATCH',
fnlctrl marked this conversation as resolved.
Show resolved Hide resolved
INVALID_ROUTE_MATCH = 'INVALID_ROUTE_MATCH',
fnlctrl marked this conversation as resolved.
Show resolved Hide resolved
NAVIGATION_GUARD_REDIRECT = 'NAVIGATION_GUARD_REDIRECT',
NAVIGATION_ABORTED = 'NAVIGATION_ABORTED',
NAVIGATION_CANCELLED = 'NAVIGATION_CANCELLED',
}

interface RouterError {
fnlctrl marked this conversation as resolved.
Show resolved Hide resolved
type: ErrorTypes
message: string
from?: RouteLocation
fnlctrl marked this conversation as resolved.
Show resolved Hide resolved
to?: RouteLocation
}

const ErrorTypeMessages = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need some kind of logError method, similar to vue-next. In our case error codes are part of the public api but I think the messages should only be logged in dev mode and just print the error in prod
At this point I wonder if message should just be kept empty 🤔

Copy link
Member Author

@fnlctrl fnlctrl Mar 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a __DEV__ check to remove it in prod build. logError seems unnecessary as we're not catching all errors then print/rethrow them like in core, maybe?

[ErrorTypes.NO_ROUTE_MATCH](
location: MatcherLocation,
currentLocation?: MatcherLocationNormalized
) {
return `No match for\n ${JSON.stringify(location)}${
currentLocation
? '\nwhile being at\n' + JSON.stringify(currentLocation)
: ''
}`
},
[ErrorTypes.INVALID_ROUTE_MATCH](location: any) {
return `Cannot redirect using a relative location:\n${stringifyRoute(
location
)}\nUse the function redirect and explicitly provide a name`
},
[ErrorTypes.NAVIGATION_GUARD_REDIRECT](
from: RouteLocationNormalized,
to: RouteLocation
) {
return `Redirected from "${from.fullPath}" to "${stringifyRoute(
to
)}" via a navigation guard`
},
[ErrorTypes.NAVIGATION_ABORTED](
from: RouteLocationNormalized,
to: RouteLocationNormalized
) {
return `Navigation aborted from "${from.fullPath}" to "${to.fullPath}" via a navigation guard`
},
[ErrorTypes.NAVIGATION_CANCELLED](
from: RouteLocationNormalized,
to: RouteLocationNormalized
) {
return `Navigation cancelled from "${from.fullPath}" to "${to.fullPath}" with a new \`push\` or \`replace\``
},
}

export function createRouterError<Type extends ErrorTypes>(
type: Type,
...messageArgs: Parameters<typeof ErrorTypeMessages[Type]>
fnlctrl marked this conversation as resolved.
Show resolved Hide resolved
): RouterError {
const message = (ErrorTypeMessages[type] as any)(...messageArgs)
const error: RouterError = {
fnlctrl marked this conversation as resolved.
Show resolved Hide resolved
type: type,
message,
}
if (
type === ErrorTypes.NAVIGATION_ABORTED ||
type === ErrorTypes.NAVIGATION_CANCELLED ||
type === ErrorTypes.NAVIGATION_GUARD_REDIRECT
) {
error.from = messageArgs[0]
error.to = messageArgs[1]
}
return error
}

const propertiesToLog: (keyof RouteLocationNormalized)[] = [
fnlctrl marked this conversation as resolved.
Show resolved Hide resolved
'params',
'query',
'hash',
]

function stringifyRoute(to: RouteLocation): string {
if (typeof to === 'string') return to
if ('path' in to) return to.path
const location: Partial<RouteLocationNormalized> = {}
for (const key of propertiesToLog) {
// @ts-ignore
if (key in to) location[key] = to[key]
}
return JSON.stringify(location, null, 2)
}
11 changes: 8 additions & 3 deletions src/matcher/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import {
MatcherLocationNormalized,
ListenerRemover,
} from '../types'
import { NoRouteMatchError } from '../errors'
import { createRouterError, ErrorTypes } from '../errors-new'
import { createRouteRecordMatcher, RouteRecordMatcher } from './path-matcher'
import { RouteRecordNormalized } from './types'
import {
Expand Down Expand Up @@ -151,7 +151,7 @@ export function createRouterMatcher(
if ('name' in location && location.name) {
matcher = matcherMap.get(location.name)

if (!matcher) throw new NoRouteMatchError(location)
if (!matcher) throw createRouterError(ErrorTypes.NO_ROUTE_MATCH, location)

name = matcher.record.name
// TODO: merge params with current location. Should this be done by name. I think there should be some kind of relationship between the records like children of a parent should keep parent props but not the rest
Expand All @@ -177,7 +177,12 @@ export function createRouterMatcher(
matcher = currentLocation.name
? matcherMap.get(currentLocation.name)
: matchers.find(m => m.re.test(currentLocation.path))
if (!matcher) throw new NoRouteMatchError(location, currentLocation)
if (!matcher)
throw createRouterError(
ErrorTypes.NO_ROUTE_MATCH,
location,
currentLocation
)
name = matcher.record.name
params = location.params || currentLocation.params
path = matcher.stringify(params)
Expand Down
34 changes: 22 additions & 12 deletions src/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,7 @@ import {
scrollToPosition,
} from './utils/scroll'
import { createRouterMatcher } from './matcher'
import {
NavigationCancelled,
NavigationGuardRedirect,
NavigationAborted,
} from './errors'
import { createRouterError, ErrorTypes } from './errors-new'
import {
extractComponentsGuards,
guardToPromiseFn,
Expand Down Expand Up @@ -211,17 +207,21 @@ export function createRouter({
try {
await navigate(toLocation, from)
} catch (error) {
if (NavigationGuardRedirect.is(error)) {
if (error.type === ErrorTypes.NAVIGATION_GUARD_REDIRECT) {
// push was called while waiting in guards
if (pendingLocation !== toLocation) {
triggerError(new NavigationCancelled(toLocation, from))
triggerError(
createRouterError(ErrorTypes.NAVIGATION_CANCELLED, from, toLocation)
)
}
// preserve the original redirectedFrom if any
return pushWithRedirect(error.to, redirectedFrom || toLocation)
} else {
// TODO: write tests
if (pendingLocation !== toLocation) {
triggerError(new NavigationCancelled(toLocation, from))
triggerError(
createRouterError(ErrorTypes.NAVIGATION_CANCELLED, from, toLocation)
)
}
}
triggerError(error)
Expand Down Expand Up @@ -330,7 +330,10 @@ export function createRouter({
) {
// a more recent navigation took place
if (pendingLocation !== toLocation) {
return triggerError(new NavigationCancelled(toLocation, from), isPush)
return triggerError(
createRouterError(ErrorTypes.NAVIGATION_CANCELLED, toLocation, from),
isPush
)
}

// remove registered guards from removed matched records
Expand Down Expand Up @@ -376,19 +379,26 @@ export function createRouter({
await navigate(toLocation, from)
finalizeNavigation(toLocation, from, false)
} catch (error) {
if (NavigationGuardRedirect.is(error)) {
if (error.type === ErrorTypes.NAVIGATION_GUARD_REDIRECT) {
// TODO: refactor the duplication of new NavigationCancelled by
// checking instanceof NavigationError (it's another TODO)
// a more recent navigation took place
if (pendingLocation !== toLocation) {
return triggerError(new NavigationCancelled(toLocation, from), false)
return triggerError(
createRouterError(
ErrorTypes.NAVIGATION_CANCELLED,
toLocation,
from
),
false
)
}
triggerError(error, false)

// the error is already handled by router.push
// we just want to avoid logging the error
pushWithRedirect(error.to, toLocation).catch(() => {})
} else if (NavigationAborted.is(error)) {
} else if (error.type === ErrorTypes.NAVIGATION_ABORTED) {
console.log('Cancelled, going to', -info.distance)
// TODO: test on different browsers ensure consistent behavior
history.go(-info.distance, false)
Expand Down
9 changes: 6 additions & 3 deletions src/utils/guardToPromiseFn.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
} from '../types'

import { isRouteLocation } from '../types'
import { NavigationGuardRedirect, NavigationAborted } from '../errors'
import { createRouterError, ErrorTypes } from '../errors-new'

export function guardToPromiseFn(
guard: NavigationGuard,
Expand All @@ -19,9 +19,12 @@ export function guardToPromiseFn(
valid?: boolean | RouteLocation
) => {
// TODO: handle callback
if (valid === false) reject(new NavigationAborted(to, from))
if (valid === false)
reject(createRouterError(ErrorTypes.NAVIGATION_ABORTED, from, to))
else if (isRouteLocation(valid)) {
reject(new NavigationGuardRedirect(to, valid))
reject(
createRouterError(ErrorTypes.NAVIGATION_GUARD_REDIRECT, to, valid)
)
} else resolve()
}

Expand Down