feat: add deprecation warning for next() callback in navigation guards

Author: posva

packages/router/__tests__/guards/guardToPromiseFn.spec.ts

@@ -4,6 +4,9 @@ import { ErrorTypes } from '../../src/errors'
 import { mockWarn } from '../vitest-mock-warn'
 import { vi, describe, expect, it } from 'vitest'
 
+const NEXT_DEPRECATION_MESSAGE =
+  'The `next()` callback in navigation guards is deprecated. Return the value instead of calling `next(value)`.'
+
 // stub those two
 const to = START_LOCATION_NORMALIZED
 const from = {
@@ -15,28 +18,31 @@ const from = {
 describe('guardToPromiseFn', () => {
   mockWarn()
   it('calls the guard with to, from and, next', async () => {
-    expect.assertions(2)
+    expect.assertions(3)
     const spy = vi.fn((to, from, next) => next())
     await expect(guardToPromiseFn(spy, to, from)()).resolves.toEqual(undefined)
     expect(spy).toHaveBeenCalledWith(to, from, expect.any(Function))
+    expect(NEXT_DEPRECATION_MESSAGE).toHaveBeenWarned()
   })
 
   it('resolves if next is called with no arguments', async () => {
-    expect.assertions(1)
+    expect.assertions(2)
     await expect(
       guardToPromiseFn((to, from, next) => next(), to, from)()
     ).resolves.toEqual(undefined)
+    expect(NEXT_DEPRECATION_MESSAGE).toHaveBeenWarned()
   })
 
   it('resolves if next is called with true', async () => {
-    expect.assertions(1)
+    expect.assertions(2)
     await expect(
       guardToPromiseFn((to, from, next) => next(true), to, from)()
     ).resolves.toEqual(undefined)
+    expect(NEXT_DEPRECATION_MESSAGE).toHaveBeenWarned()
   })
 
   it('rejects if next is called with false', async () => {
-    expect.assertions(1)
+    expect.assertions(2)
     try {
       await guardToPromiseFn((to, from, next) => next(false), to, from)()
     } catch (err) {
@@ -46,10 +52,11 @@ describe('guardToPromiseFn', () => {
         type: ErrorTypes.NAVIGATION_ABORTED,
       })
     }
+    expect(NEXT_DEPRECATION_MESSAGE).toHaveBeenWarned()
   })
 
   it('rejects if next is called with a string location', async () => {
-    expect.assertions(1)
+    expect.assertions(2)
     try {
       await guardToPromiseFn((to, from, next) => next('/new'), to, from)()
     } catch (err) {
@@ -59,10 +66,11 @@ describe('guardToPromiseFn', () => {
         type: ErrorTypes.NAVIGATION_GUARD_REDIRECT,
       })
     }
+    expect(NEXT_DEPRECATION_MESSAGE).toHaveBeenWarned()
   })
 
   it('rejects if next is called with an object location', async () => {
-    expect.assertions(1)
+    expect.assertions(2)
     let redirectTo = { path: '/new' }
     try {
       await guardToPromiseFn((to, from, next) => next(redirectTo), to, from)()
@@ -73,14 +81,16 @@ describe('guardToPromiseFn', () => {
         type: ErrorTypes.NAVIGATION_GUARD_REDIRECT,
       })
     }
+    expect(NEXT_DEPRECATION_MESSAGE).toHaveBeenWarned()
   })
 
   it('rejects if next is called with an error', async () => {
-    expect.assertions(1)
+    expect.assertions(2)
     let error = new Error('nope')
     await expect(
       guardToPromiseFn((to, from, next) => next(error), to, from)()
     ).rejects.toBe(error)
+    expect(NEXT_DEPRECATION_MESSAGE).toHaveBeenWarned()
   })
 
   it('rejects if guard rejects a Promise', async () => {
@@ -265,7 +275,7 @@ describe('guardToPromiseFn', () => {
   })
 
   it('does not warn if guard returns undefined', async () => {
-    expect.assertions(2)
+    expect.assertions(3)
     await expect(
       guardToPromiseFn(
         (to, from, next) => {
@@ -278,5 +288,38 @@ describe('guardToPromiseFn', () => {
     ).resolves.toEqual(undefined)
 
     expect('callback was never called').not.toHaveBeenWarned()
+    expect(NEXT_DEPRECATION_MESSAGE).toHaveBeenWarned()
+  })
+
+  describe('next() callback deprecation', () => {
+    it('warns when guard calls next()', async () => {
+      await guardToPromiseFn((to, from, next) => next(), to, from)()
+      expect(NEXT_DEPRECATION_MESSAGE).toHaveBeenWarned()
+    })
+
+    it('warns only once per guard run when next() is called multiple times', async () => {
+      await expect(
+        guardToPromiseFn(
+          (to, from, next) => {
+            next()
+            next()
+          },
+          to,
+          from
+        )()
+      ).resolves.toEqual(undefined)
+      expect(NEXT_DEPRECATION_MESSAGE).toHaveBeenWarnedTimes(1)
+      expect('called more than once').toHaveBeenWarned()
+    })
+
+    it('does not warn when guard returns value without next parameter', async () => {
+      await guardToPromiseFn((to, from) => true, to, from)()
+      expect(NEXT_DEPRECATION_MESSAGE).not.toHaveBeenWarned()
+    })
+
+    it('does not warn when guard returns undefined without next parameter', async () => {
+      await guardToPromiseFn((to, from) => {}, to, from)()
+      expect(NEXT_DEPRECATION_MESSAGE).not.toHaveBeenWarned()
+    })
   })
 })

packages/router/__tests__/warnings.spec.ts

@@ -10,6 +10,9 @@ import { mockWarn } from './vitest-mock-warn'
 
 let component = defineComponent({})
 
+const NEXT_DEPRECATION_MESSAGE =
+  'The `next()` callback in navigation guards is deprecated. Return the value instead of calling `next(value)`.'
+
 describe('warnings', () => {
   mockWarn()
   it('warns on missing name and path for redirect', async () => {
@@ -119,7 +122,7 @@ describe('warnings', () => {
   })
 
   it('warns if next is called multiple times in one navigation guard', async () => {
-    expect.assertions(3)
+    expect.assertions(4)
     let router = createRouter({
       history: createMemoryHistory(),
       routes: [
@@ -130,14 +133,15 @@ describe('warnings', () => {
 
     router.beforeEach((to, from, next) => {
       next()
-      expect('').not.toHaveBeenWarned()
+      expect('called more than once').not.toHaveBeenWarned()
       next()
       expect('called more than once').toHaveBeenWarnedTimes(1)
       next()
       expect('called more than once').toHaveBeenWarnedTimes(1)
     })
 
     await router.push('/b')
+    expect(NEXT_DEPRECATION_MESSAGE).toHaveBeenWarned()
   })
 
   it('warns if a non valid function is passed as a component', async () => {
@@ -289,6 +293,7 @@ describe('warnings', () => {
     expect(
       'It should be called exactly one time in each navigation guard'
     ).toHaveBeenWarned()
+    expect(NEXT_DEPRECATION_MESSAGE).toHaveBeenWarned()
   })
 
   it('warns when discarding params', () => {

packages/router/src/navigationGuards.ts

@@ -199,7 +199,9 @@ export function guardToPromiseFn(
           record && record.instances[name!],
           to,
           from,
-          __DEV__ ? canOnlyBeCalledOnce(next, to, from) : next
+          __DEV__
+            ? withDeprecationWarning(canOnlyBeCalledOnce(next, to, from))
+            : next
         )
       )
       let guardCall = Promise.resolve(guardReturn)
@@ -231,6 +233,28 @@ export function guardToPromiseFn(
     })
 }
 
+/**
+ * Wraps the next callback to warn when it is used. Dev-only: when __DEV__ is
+ * false (production builds), this branch is dead code and is stripped from the
+ * bundle.
+ *
+ * @internal
+ */
+function withDeprecationWarning(
+  next: NavigationGuardNext
+): NavigationGuardNext {
+  let warned = false
+  return function (this: any) {
+    if (!warned) {
+      warned = true
+      warn(
+        'The `next()` callback in navigation guards is deprecated. Return the value instead of calling `next(value)`.'
+      )
+    }
+    return next.apply(this, arguments as any)
+  }
+}
+
 function canOnlyBeCalledOnce(
   next: NavigationGuardNext,
   to: RouteLocationNormalized,

packages/router/src/typed-routes/navigation-guards.ts

@@ -24,7 +24,10 @@ export interface NavigationGuardWithThis<T> {
     this: T,
     to: RouteLocationNormalized,
     from: RouteLocationNormalizedLoaded,
-    // intentionally not typed to make people use the return
+    /**
+     * @deprecated Return a value from the guard instead of calling `next(value)`.
+     * The callback will be removed in a future version of Vue Router.
+     */
     next: NavigationGuardNext
   ): _Awaitable<NavigationGuardReturn>
 }
@@ -40,7 +43,10 @@ export interface _NavigationGuardResolved {
     this: undefined,
     to: RouteLocationNormalizedLoaded,
     from: RouteLocationNormalizedLoaded,
-    // intentionally not typed to make people use the return
+    /**
+     * @deprecated Return a value from the guard instead of calling `next(value)`.
+     * The callback will be removed in a future version of Vue Router.
+     */
     next: NavigationGuardNext
   ): _Awaitable<NavigationGuardReturn>
 }
@@ -52,7 +58,10 @@ export interface NavigationGuard {
   (
     to: RouteLocationNormalized,
     from: RouteLocationNormalizedLoaded,
-    // intentionally not typed to make people use the return
+    /**
+     * @deprecated Return a value from the guard instead of calling `next(value)`.
+     * The callback will be removed in a future version of Vue Router.
+     */
     next: NavigationGuardNext
   ): _Awaitable<NavigationGuardReturn>
 }
@@ -69,7 +78,10 @@ export interface NavigationHookAfter {
 }
 
 /**
- * `next()` callback passed to navigation guards.
+ * Callback passed to navigation guards to continue or abort the navigation.
+ *
+ * @deprecated Prefer returning a value from the guard instead of calling
+ * `next(value)`. The callback will be removed in a future version of Vue Router.
  */
 export interface NavigationGuardNext {
   (): void