From 0b483f9308a52cbc6df6d1e0f354e199412a6059 Mon Sep 17 00:00:00 2001 From: skirtle <65301168+skirtles-code@users.noreply.github.com> Date: Sun, 3 Mar 2024 02:18:16 +0000 Subject: [PATCH] feat(dx): warn when passing undefined/null locations --- packages/router/__tests__/router.spec.ts | 16 +++ packages/router/__tests__/useLink.spec.ts | 148 ++++++++++++++++++++++ packages/router/src/RouterLink.ts | 43 ++++++- packages/router/src/devtools.ts | 14 +- packages/router/src/router.ts | 17 ++- packages/router/src/utils/index.ts | 3 + 6 files changed, 235 insertions(+), 6 deletions(-) create mode 100644 packages/router/__tests__/useLink.spec.ts diff --git a/packages/router/__tests__/router.spec.ts b/packages/router/__tests__/router.spec.ts index 951f2f0db..2ba960835 100644 --- a/packages/router/__tests__/router.spec.ts +++ b/packages/router/__tests__/router.spec.ts @@ -330,6 +330,22 @@ describe('Router', () => { expect(route1.params).toEqual({ p: 'a' }) }) + it('handles an undefined location', async () => { + const { router } = await newRouter() + + const route1 = router.resolve(undefined as any) + expect('router.resolve() was passed an invalid location').toHaveBeenWarned() + expect(route1.path).toBe('/') + }) + + it('handles a null location', async () => { + const { router } = await newRouter() + + const route1 = router.resolve(null as any) + expect('router.resolve() was passed an invalid location').toHaveBeenWarned() + expect(route1.path).toBe('/') + }) + it('removes null/undefined optional params when current location has it', async () => { const { router } = await newRouter() diff --git a/packages/router/__tests__/useLink.spec.ts b/packages/router/__tests__/useLink.spec.ts new file mode 100644 index 000000000..63b977fc0 --- /dev/null +++ b/packages/router/__tests__/useLink.spec.ts @@ -0,0 +1,148 @@ +/** + * @jest-environment jsdom + */ +import { nextTick, ref } from 'vue' +import { mount } from '@vue/test-utils' +import { mockWarn } from 'jest-mock-warn' +import { + createMemoryHistory, + createRouter, + routeLocationKey, + RouteLocationRaw, + routerKey, + useLink, + UseLinkOptions, +} from '../src' + +async function callUseLink(args: UseLinkOptions) { + const router = createRouter({ + history: createMemoryHistory(), + routes: [ + { + path: '/', + component: {}, + name: 'root', + }, + { + path: '/a', + component: {}, + name: 'a', + }, + { + path: '/b', + component: {}, + name: 'b', + }, + ], + }) + + await router.push('/') + + let link: ReturnType + + mount( + { + setup() { + link = useLink(args) + + return () => '' + }, + }, + { + global: { + provide: { + [routerKey as any]: router, + [routeLocationKey as any]: router.currentRoute.value, + }, + }, + } + ) + + return link! +} + +describe('useLink', () => { + describe('basic usage', () => { + it('supports a string for "to"', async () => { + const { href, route } = await callUseLink({ + to: '/a', + }) + + expect(href.value).toBe('/a') + expect(route.value).toMatchObject({ name: 'a' }) + }) + + it('supports an object for "to"', async () => { + const { href, route } = await callUseLink({ + to: { path: '/a' }, + }) + + expect(href.value).toBe('/a') + expect(route.value).toMatchObject({ name: 'a' }) + }) + + it('supports a ref for "to"', async () => { + const to = ref('/a') + + const { href, route } = await callUseLink({ + to, + }) + + expect(href.value).toBe('/a') + expect(route.value).toMatchObject({ name: 'a' }) + + to.value = { path: '/b' } + + await nextTick() + + expect(href.value).toBe('/b') + expect(route.value).toMatchObject({ name: 'b' }) + }) + }) + + describe('warnings', () => { + mockWarn() + + it('should warn when "to" is undefined', async () => { + await callUseLink({ + to: undefined as any, + }) + + expect('Invalid value for prop "to" in useLink()').toHaveBeenWarned() + expect( + 'router.resolve() was passed an invalid location' + ).toHaveBeenWarned() + }) + + it('should warn when "to" is an undefined ref', async () => { + await callUseLink({ + to: ref(undefined as any), + }) + + expect('Invalid value for prop "to" in useLink()').toHaveBeenWarned() + expect( + 'router.resolve() was passed an invalid location' + ).toHaveBeenWarned() + }) + + it('should warn when "to" changes to a null ref', async () => { + const to = ref('/a') + + const { href, route } = await callUseLink({ + to, + }) + + expect(href.value).toBe('/a') + expect(route.value).toMatchObject({ name: 'a' }) + + to.value = null as any + + await nextTick() + + expect('Invalid value for prop "to" in useLink()').toHaveBeenWarned() + expect( + 'router.resolve() was passed an invalid location' + ).toHaveBeenWarned() + }) + }) +}) diff --git a/packages/router/src/RouterLink.ts b/packages/router/src/RouterLink.ts index 1d18845d6..de916836f 100644 --- a/packages/router/src/RouterLink.ts +++ b/packages/router/src/RouterLink.ts @@ -36,7 +36,8 @@ import { isSameRouteLocationParams, isSameRouteRecord } from './location' import { routerKey, routeLocationKey } from './injectionSymbols' import { RouteRecord } from './matcher/types' import { NavigationFailure } from './errors' -import { isArray, isBrowser, noop } from './utils' +import { isArray, isBrowser, isObject, noop } from './utils' +import { warn } from './warning' export interface RouterLinkOptions { /** @@ -83,6 +84,7 @@ export interface UseLinkDevtoolsContext { route: RouteLocationNormalized & { href: string } isActive: boolean isExactActive: boolean + error: string } export type UseLinkOptions = VueUseOptions @@ -93,7 +95,40 @@ export function useLink(props: UseLinkOptions) { const router = inject(routerKey)! const currentRoute = inject(routeLocationKey)! - const route = computed(() => router.resolve(unref(props.to))) + const isValidTo = (to: unknown) => typeof to === 'string' || isObject(to) + let hasPrevious = false + let previousTo: unknown = null + + const route = computed(() => { + const to = unref(props.to) + + if (__DEV__ && (!hasPrevious || to !== previousTo)) { + if (!isValidTo(to)) { + if (hasPrevious) { + warn( + `Invalid value for prop "to" in useLink()\n- to:`, + to, + `\n- previous to:`, + previousTo, + `\n- props:`, + props + ) + } else { + warn( + `Invalid value for prop "to" in useLink()\n- to:`, + to, + `\n- props:`, + props + ) + } + } + + previousTo = to + hasPrevious = true + } + + return router.resolve(to) + }) const activeRecordIndex = computed(() => { const { matched } = route.value @@ -157,6 +192,7 @@ export function useLink(props: UseLinkOptions) { route: route.value, isActive: isActive.value, isExactActive: isExactActive.value, + error: '', } // @ts-expect-error: this is internal @@ -168,6 +204,9 @@ export function useLink(props: UseLinkOptions) { linkContextDevtools.route = route.value linkContextDevtools.isActive = isActive.value linkContextDevtools.isExactActive = isExactActive.value + linkContextDevtools.error = isValidTo(unref(props.to)) + ? '' + : 'Invalid "to" value' }, { flush: 'post' } ) diff --git a/packages/router/src/devtools.ts b/packages/router/src/devtools.ts index 81760225b..b543fa80b 100644 --- a/packages/router/src/devtools.ts +++ b/packages/router/src/devtools.ts @@ -119,10 +119,16 @@ export function addDevtools(app: App, router: Router, matcher: RouterMatcher) { ;( componentInstance.__vrl_devtools as UseLinkDevtoolsContext[] ).forEach(devtoolsData => { + let label = devtoolsData.route.path let backgroundColor = ORANGE_400 let tooltip: string = '' + let textColor = 0 - if (devtoolsData.isExactActive) { + if (devtoolsData.error) { + label = devtoolsData.error + backgroundColor = RED_100 + textColor = RED_700 + } else if (devtoolsData.isExactActive) { backgroundColor = LIME_500 tooltip = 'This is exactly active' } else if (devtoolsData.isActive) { @@ -131,8 +137,8 @@ export function addDevtools(app: App, router: Router, matcher: RouterMatcher) { } node.tags.push({ - label: devtoolsData.route.path, - textColor: 0, + label, + textColor, tooltip, backgroundColor, }) @@ -419,6 +425,8 @@ const CYAN_400 = 0x22d3ee const ORANGE_400 = 0xfb923c // const GRAY_100 = 0xf4f4f5 const DARK = 0x666666 +const RED_100 = 0xfee2e2 +const RED_700 = 0xb91c1c function formatRouteRecordForInspector( route: RouteRecordMatcher diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index b1ec39dfd..493439161 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -32,7 +32,14 @@ import { NavigationRedirectError, isNavigationFailure, } from './errors' -import { applyToParams, isBrowser, assign, noop, isArray } from './utils' +import { + applyToParams, + isBrowser, + assign, + noop, + isArray, + isObject, +} from './utils' import { useCallbacks } from './utils/callbacks' import { encodeParam, decode, encodeHash } from './encoding' import { @@ -460,6 +467,14 @@ export function createRouter(options: RouterOptions): Router { }) } + if (__DEV__ && !isObject(rawLocation)) { + warn( + `router.resolve() was passed an invalid location. This will fail in production.\n- Location:`, + rawLocation + ) + rawLocation = {} + } + let matcherLocation: MatcherLocationRaw // path could be relative in object as well diff --git a/packages/router/src/utils/index.ts b/packages/router/src/utils/index.ts index af90cb5ce..70fc0aa81 100644 --- a/packages/router/src/utils/index.ts +++ b/packages/router/src/utils/index.ts @@ -37,3 +37,6 @@ export const noop = () => {} */ export const isArray: (arg: ArrayLike | any) => arg is ReadonlyArray = Array.isArray + +export const isObject = (val: unknown): val is Record => + val !== null && typeof val === 'object'