From 1c600da7af0a6517992de277622ec7109f2c46b8 Mon Sep 17 00:00:00 2001 From: Janka Uryga Date: Tue, 11 Jun 2024 17:12:41 +0200 Subject: [PATCH 1/4] fix: app-router crash when an invalid URL is passed to prefetch --- .../next/src/client/components/app-router.tsx | 12 ++++++++++- .../app-prefetch/app/invalid-url/delay.js | 15 ++++++++++++++ .../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 | 16 +++++++++++++++ 6 files changed, 86 insertions(+), 1 deletion(-) 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/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 0a8493ac219f2..7e913c2724cad 100644 --- a/packages/next/src/client/components/app-router.tsx +++ b/packages/next/src/client/components/app-router.tsx @@ -369,7 +369,17 @@ function Router({ ) { return } - const url = new URL(addBasePath(href), window.location.href) + + let url: URL + try { + url = new URL(addBasePath(href), window.location.href) + } catch (err) { + console.error( + `Cannot prefetch '${href}' because it cannot be converted to a URL.` + ) + return + } + // External urls can't be prefetched in the same way. if (isExternalURL(url)) { return 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/from-link/page.js b/test/e2e/app-dir/app-prefetch/app/invalid-url/from-link/page.js new file mode 100644 index 0000000000000..6279493ecb384 --- /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 async 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..5996f4d2a3f18 --- /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 async 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 002836bc969f4..4846f2e0b511c 100644 --- a/test/e2e/app-dir/app-prefetch/prefetching.test.ts +++ b/test/e2e/app-dir/app-prefetch/prefetching.test.ts @@ -329,4 +329,20 @@ describe('app dir - prefetching', () => { }) }) }) + + describe('should not crash for invalid URLs', () => { + it.each([ + { title: 'passed to Link', path: '/invalid-url/from-link' }, + { + title: 'passed to router.prefetch', + path: '/invalid-url/from-router-prefetch', + }, + ])('$title', async ({ path }) => { + const browser = await next.browser(path) + + await check(() => browser.hasElementByCssSelector('h1'), true) + + expect(await browser.elementByCss('h1').text()).toEqual('Hello, world!') + }) + }) }) From 52b4d34bf75829e9d697835678cd3825e3142a76 Mon Sep 17 00:00:00 2001 From: Janka Uryga Date: Tue, 11 Jun 2024 17:39:45 +0200 Subject: [PATCH 2/4] log error for invalid url prefetches in development --- packages/next/src/client/components/app-router.tsx | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/next/src/client/components/app-router.tsx b/packages/next/src/client/components/app-router.tsx index 7e913c2724cad..7fc2e65f5618b 100644 --- a/packages/next/src/client/components/app-router.tsx +++ b/packages/next/src/client/components/app-router.tsx @@ -362,11 +362,7 @@ function Router({ forward: () => window.history.forward(), prefetch: (href, options) => { // Don't prefetch for bots as they don't navigate. - // Don't prefetch during development (improves compilation performance) - if ( - isBot(window.navigator.userAgent) || - process.env.NODE_ENV === 'development' - ) { + if (isBot(window.navigator.userAgent)) { return } @@ -380,6 +376,11 @@ function Router({ return } + // Don't prefetch during development (improves compilation performance) + if (process.env.NODE_ENV === 'development') { + return + } + // External urls can't be prefetched in the same way. if (isExternalURL(url)) { return From 8e75fd3002d5031854af6659cdee45f3d9e7923f Mon Sep 17 00:00:00 2001 From: Janka Uryga Date: Tue, 11 Jun 2024 17:48:52 +0200 Subject: [PATCH 3/4] test: remove unnecessary async from page components --- test/e2e/app-dir/app-prefetch/app/invalid-url/from-link/page.js | 2 +- .../app-prefetch/app/invalid-url/from-router-prefetch/page.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 index 6279493ecb384..cdb564620683e 100644 --- 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 @@ -4,7 +4,7 @@ import { Delay } from '../delay' export const dynamic = 'force-dynamic' -export default async function Page() { +export default function Page() { return ( <> invalid link 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 index 5996f4d2a3f18..a5a61aa3624da 100644 --- 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 @@ -6,7 +6,7 @@ import { useRouter } from 'next/navigation' export const dynamic = 'force-dynamic' -export default async function Page() { +export default function Page() { const router = useRouter() useEffect(() => { router.prefetch(INVALID_URL) From 933f6c9c514ca5a8370f000cae63aea620a60748 Mon Sep 17 00:00:00 2001 From: Janka Uryga Date: Tue, 11 Jun 2024 19:23:48 +0200 Subject: [PATCH 4/4] make router.prefetch throw when given invalid URL, but don't throw in Link --- .../next/src/client/components/app-router.tsx | 5 ++--- packages/next/src/client/link.tsx | 14 ++++++++---- .../app-prefetch/app/invalid-url/error.js | 4 ++++ .../app-dir/app-prefetch/prefetching.test.ts | 22 ++++++++++--------- 4 files changed, 28 insertions(+), 17 deletions(-) create mode 100644 test/e2e/app-dir/app-prefetch/app/invalid-url/error.js diff --git a/packages/next/src/client/components/app-router.tsx b/packages/next/src/client/components/app-router.tsx index 7fc2e65f5618b..73bbd87adb8f2 100644 --- a/packages/next/src/client/components/app-router.tsx +++ b/packages/next/src/client/components/app-router.tsx @@ -369,11 +369,10 @@ function Router({ let url: URL try { url = new URL(addBasePath(href), window.location.href) - } catch (err) { - console.error( + } catch (_) { + throw new Error( `Cannot prefetch '${href}' because it cannot be converted to a URL.` ) - return } // Don't prefetch during development (improves compilation performance) diff --git a/packages/next/src/client/link.tsx b/packages/next/src/client/link.tsx index d8e9318f2ac2d..1c3b149d8b615 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/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/prefetching.test.ts b/test/e2e/app-dir/app-prefetch/prefetching.test.ts index 4846f2e0b511c..06d1cf0c0eb7d 100644 --- a/test/e2e/app-dir/app-prefetch/prefetching.test.ts +++ b/test/e2e/app-dir/app-prefetch/prefetching.test.ts @@ -330,19 +330,21 @@ describe('app dir - prefetching', () => { }) }) - describe('should not crash for invalid URLs', () => { - it.each([ - { title: 'passed to Link', path: '/invalid-url/from-link' }, - { - title: 'passed to router.prefetch', - path: '/invalid-url/from-router-prefetch', - }, - ])('$title', async ({ path }) => { - const browser = await next.browser(path) + 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' + ) + }) }) })