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 <[email protected]> * Pass the router as a plugin during testing * Use null when there are no errors * Use isRouteLocation --------- Co-authored-by: Eduardo San Martin Morote <[email protected]>
vuejs · Mar 5, 2024 · 089378b · 089378b
1 parent 0435b74
commit 089378b
Show file tree

Hide file tree

Showing 5 changed files with 219 additions and 4 deletions.
diff --git 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
@@ -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<typeof useLink>
+
+  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<RouteLocationRaw>('/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
@@ -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<RouterLinkOptions>
@@ -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<number>(() => {
     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
@@ -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
@@ -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