From 089378bb973a6d91ddfe8eea76a670cd805fb5cf Mon Sep 17 00:00:00 2001 From: skirtle <65301168+skirtles-code@users.noreply.github.com> Date: Tue, 5 Mar 2024 07:44:23 +0000 Subject: [PATCH] feat(dx): warn when passing undefined/null locations (#2158) * feat(dx): warn when passing undefined/null locations * Reword test descriptions Co-authored-by: Eduardo San Martin Morote * Pass the router as a plugin during testing * Use null when there are no errors * Use isRouteLocation --------- Co-authored-by: Eduardo San Martin Morote --- packages/router/__tests__/router.spec.ts | 16 +++ packages/router/__tests__/useLink.spec.ts | 143 ++++++++++++++++++++++ packages/router/src/RouterLink.ts | 41 ++++++- packages/router/src/devtools.ts | 14 ++- packages/router/src/router.ts | 9 ++ 5 files changed, 219 insertions(+), 4 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 e762971b5..691b9d9f5 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('warns on undefined location during dev', 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('warns on null location during dev', 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..6f148bd26 --- /dev/null +++ b/packages/router/__tests__/useLink.spec.ts @@ -0,0 +1,143 @@ +/** + * @jest-environment jsdom + */ +import { nextTick, ref } from 'vue' +import { mount } from '@vue/test-utils' +import { mockWarn } from 'jest-mock-warn' +import { + createMemoryHistory, + createRouter, + RouteLocationRaw, + 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: { + plugins: [router], + }, + } + ) + + 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..d2e66fa28 100644 --- a/packages/router/src/RouterLink.ts +++ b/packages/router/src/RouterLink.ts @@ -27,6 +27,7 @@ import { ComponentOptionsMixin, } from 'vue' import { + isRouteLocation, RouteLocationRaw, VueUseOptions, RouteLocation, @@ -37,6 +38,7 @@ import { routerKey, routeLocationKey } from './injectionSymbols' import { RouteRecord } from './matcher/types' import { NavigationFailure } from './errors' import { isArray, isBrowser, noop } from './utils' +import { warn } from './warning' export interface RouterLinkOptions { /** @@ -83,6 +85,7 @@ export interface UseLinkDevtoolsContext { route: RouteLocationNormalized & { href: string } isActive: boolean isExactActive: boolean + error: string | null } export type UseLinkOptions = VueUseOptions @@ -93,7 +96,39 @@ export function useLink(props: UseLinkOptions) { const router = inject(routerKey)! const currentRoute = inject(routeLocationKey)! - const route = computed(() => router.resolve(unref(props.to))) + let hasPrevious = false + let previousTo: unknown = null + + const route = computed(() => { + const to = unref(props.to) + + if (__DEV__ && (!hasPrevious || to !== previousTo)) { + if (!isRouteLocation(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: null, } // @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 = isRouteLocation(unref(props.to)) + ? null + : '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 8fec39d82..20ab4d1e2 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -8,6 +8,7 @@ import { RouteLocationNormalizedLoaded, RouteLocation, RouteRecordName, + isRouteLocation, isRouteName, NavigationGuardWithThis, RouteLocationOptions, @@ -468,6 +469,14 @@ export function createRouter(options: RouterOptions): Router { }) } + if (__DEV__ && !isRouteLocation(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