From 2807fb489545e8b741b1b7eb37e90ea476747c24 Mon Sep 17 00:00:00 2001 From: Janka Uryga Date: Tue, 11 Jun 2024 21:36:27 +0200 Subject: [PATCH] fix: app-router prefetch crash when an invalid URL is passed to Link (#66755) Closes [#66650](https://github.com/vercel/next.js/issues/66650) Closes NEXT-3520 - Make Link not throw during prefetch if it received an invalid `href` (see [#66650](https://github.com/vercel/next.js/issues/66650)) - Throw in dev mode if an invalid link was passed to `router.prefetch` -- this matches current prod behavior - (previously, we'd immediately exit out of `router.prefetch`, so the user would see no indication that this'd fail in prod) If an invalid URL was passed to ``, the whole app would crash and show "A client-side exception occurred". A failed prefetch should not bring down the whole app. Note that This preserves the current behavior of explicit `router.prefetch(url)` throwing an error if `url` is invalid. We may want to adjust this in the future, but that could be considered a breaking change, so I'm leaving it out for now. This only affects `Link`, which was intended to catch any errors thrown from `router.prefetch`, but that bit was accidentally broken. --- .../next/src/client/components/app-router.tsx | 20 ++++++++++++++----- packages/next/src/client/link.tsx | 14 +++++++++---- .../app-prefetch/app/invalid-url/delay.js | 15 ++++++++++++++ .../app-prefetch/app/invalid-url/error.js | 4 ++++ .../app/invalid-url/from-link/page.js | 16 +++++++++++++++ .../invalid-url/from-router-prefetch/page.js | 20 +++++++++++++++++++ .../app/invalid-url/invalid-url.js | 8 ++++++++ .../app-dir/app-prefetch/prefetching.test.ts | 18 +++++++++++++++++ 8 files changed, 106 insertions(+), 9 deletions(-) create mode 100644 test/e2e/app-dir/app-prefetch/app/invalid-url/delay.js create mode 100644 test/e2e/app-dir/app-prefetch/app/invalid-url/error.js create mode 100644 test/e2e/app-dir/app-prefetch/app/invalid-url/from-link/page.js create mode 100644 test/e2e/app-dir/app-prefetch/app/invalid-url/from-router-prefetch/page.js create mode 100644 test/e2e/app-dir/app-prefetch/app/invalid-url/invalid-url.js diff --git a/packages/next/src/client/components/app-router.tsx b/packages/next/src/client/components/app-router.tsx index 86ea7dfd478eb..23c3ab55619aa 100644 --- a/packages/next/src/client/components/app-router.tsx +++ b/packages/next/src/client/components/app-router.tsx @@ -356,14 +356,24 @@ function Router({ forward: () => window.history.forward(), prefetch: (href, options) => { // Don't prefetch for bots as they don't navigate. + if (isBot(window.navigator.userAgent)) { + return + } + + let url: URL + try { + url = new URL(addBasePath(href), window.location.href) + } catch (_) { + throw new Error( + `Cannot prefetch '${href}' because it cannot be converted to a URL.` + ) + } + // Don't prefetch during development (improves compilation performance) - if ( - isBot(window.navigator.userAgent) || - process.env.NODE_ENV === 'development' - ) { + if (process.env.NODE_ENV === 'development') { return } - const url = new URL(addBasePath(href), window.location.href) + // External urls can't be prefetched in the same way. if (isExternalURL(url)) { return diff --git a/packages/next/src/client/link.tsx b/packages/next/src/client/link.tsx index 71f69a4780145..400c03282f461 100644 --- a/packages/next/src/client/link.tsx +++ b/packages/next/src/client/link.tsx @@ -161,15 +161,21 @@ function prefetch( prefetched.add(prefetchedKey) } - const prefetchPromise = isAppRouter - ? (router as AppRouterInstance).prefetch(href, appOptions) - : (router as NextRouter).prefetch(href, as, options) + const doPrefetch = async () => { + if (isAppRouter) { + // note that `appRouter.prefetch()` is currently sync, + // so we have to wrap this call in an async function to be able to catch() errors below. + return (router as AppRouterInstance).prefetch(href, appOptions) + } else { + return (router as NextRouter).prefetch(href, as, options) + } + } // Prefetch the JSON page if asked (only in the client) // We need to handle a prefetch error here since we may be // loading with priority which can reject but we don't // want to force navigation since this is only a prefetch - Promise.resolve(prefetchPromise).catch((err) => { + doPrefetch().catch((err) => { if (process.env.NODE_ENV !== 'production') { // rethrow to show invalid URL errors throw err diff --git a/test/e2e/app-dir/app-prefetch/app/invalid-url/delay.js b/test/e2e/app-dir/app-prefetch/app/invalid-url/delay.js new file mode 100644 index 0000000000000..0f6f762a51bbc --- /dev/null +++ b/test/e2e/app-dir/app-prefetch/app/invalid-url/delay.js @@ -0,0 +1,15 @@ +'use client' + +import { useEffect } from 'react' +import { useState } from 'react' + +export function Delay({ duration = 500, children }) { + const [isVisible, setIsVisible] = useState(false) + useEffect(() => { + const timeout = setTimeout(() => setIsVisible(true), duration) + return () => clearTimeout(timeout) + }, [duration]) + + if (!isVisible) return null + return <>{children} +} diff --git a/test/e2e/app-dir/app-prefetch/app/invalid-url/error.js b/test/e2e/app-dir/app-prefetch/app/invalid-url/error.js new file mode 100644 index 0000000000000..365d656d68601 --- /dev/null +++ b/test/e2e/app-dir/app-prefetch/app/invalid-url/error.js @@ -0,0 +1,4 @@ +'use client' +export default function Error() { + return

A prefetch threw an error

+} diff --git a/test/e2e/app-dir/app-prefetch/app/invalid-url/from-link/page.js b/test/e2e/app-dir/app-prefetch/app/invalid-url/from-link/page.js new file mode 100644 index 0000000000000..cdb564620683e --- /dev/null +++ b/test/e2e/app-dir/app-prefetch/app/invalid-url/from-link/page.js @@ -0,0 +1,16 @@ +import Link from 'next/link' +import { INVALID_URL } from '../invalid-url' +import { Delay } from '../delay' + +export const dynamic = 'force-dynamic' + +export default function Page() { + return ( + <> + invalid link + +

Hello, world!

+ + + ) +} diff --git a/test/e2e/app-dir/app-prefetch/app/invalid-url/from-router-prefetch/page.js b/test/e2e/app-dir/app-prefetch/app/invalid-url/from-router-prefetch/page.js new file mode 100644 index 0000000000000..a5a61aa3624da --- /dev/null +++ b/test/e2e/app-dir/app-prefetch/app/invalid-url/from-router-prefetch/page.js @@ -0,0 +1,20 @@ +'use client' +import { useEffect } from 'react' +import { INVALID_URL } from '../invalid-url' +import { Delay } from '../delay' +import { useRouter } from 'next/navigation' + +export const dynamic = 'force-dynamic' + +export default function Page() { + const router = useRouter() + useEffect(() => { + router.prefetch(INVALID_URL) + }, [router]) + + return ( + +

Hello, world!

+ + ) +} diff --git a/test/e2e/app-dir/app-prefetch/app/invalid-url/invalid-url.js b/test/e2e/app-dir/app-prefetch/app/invalid-url/invalid-url.js new file mode 100644 index 0000000000000..a0415625a2753 --- /dev/null +++ b/test/e2e/app-dir/app-prefetch/app/invalid-url/invalid-url.js @@ -0,0 +1,8 @@ +// We need a URL that reliably fails `new URL(url, window.location) +// This only fails if `window.location` starts with `https://`: +// +// const invalidUrl = 'http:' +// +// because `new URL('http:', 'http://localhost:3000')` works fine. +// So better to pick something that's always invalid +export const INVALID_URL = '///' diff --git a/test/e2e/app-dir/app-prefetch/prefetching.test.ts b/test/e2e/app-dir/app-prefetch/prefetching.test.ts index 50b83fee6867f..df83f886da1a1 100644 --- a/test/e2e/app-dir/app-prefetch/prefetching.test.ts +++ b/test/e2e/app-dir/app-prefetch/prefetching.test.ts @@ -394,5 +394,23 @@ createNextDescribe( expect(await browser.elementById('count').text()).toBe('4') }) }) + + describe('invalid URLs', () => { + it('should not throw when an invalid URL is passed to Link', async () => { + const browser = await next.browser('/invalid-url/from-link') + + await check(() => browser.hasElementByCssSelector('h1'), true) + expect(await browser.elementByCss('h1').text()).toEqual('Hello, world!') + }) + + it('should throw when an invalid URL is passed to router.prefetch', async () => { + const browser = await next.browser('/invalid-url/from-router-prefetch') + + await check(() => browser.hasElementByCssSelector('h1'), true) + expect(await browser.elementByCss('h1').text()).toEqual( + 'A prefetch threw an error' + ) + }) + }) } )