Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: app-router prefetch crash when an invalid URL is passed to Link #66755

Merged
merged 4 commits into from
Jun 11, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions packages/next/src/client/components/app-router.tsx
Original file line number Diff line number Diff line change
@@ -362,14 +362,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
14 changes: 10 additions & 4 deletions packages/next/src/client/link.tsx
Original file line number Diff line number Diff line change
@@ -161,15 +161,21 @@ function prefetch(
prefetched.add(prefetchedKey)
}

const prefetchPromise = isAppRouter
? (router as AppRouterInstance).prefetch(href, appOptions)
Copy link
Member Author

@lubieowoce lubieowoce Jun 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in app router, router.prefetch is sync, so this does not return a promise, and the .catch() below does nothing -- we never reach it. looks like this part was missed when router.prefetch was changed from sync to async here: f4a01b4

: (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
15 changes: 15 additions & 0 deletions test/e2e/app-dir/app-prefetch/app/invalid-url/delay.js
Original file line number Diff line number Diff line change
@@ -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}</>
}
4 changes: 4 additions & 0 deletions test/e2e/app-dir/app-prefetch/app/invalid-url/error.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
'use client'
export default function Error() {
return <h1>A prefetch threw an error</h1>
}
16 changes: 16 additions & 0 deletions test/e2e/app-dir/app-prefetch/app/invalid-url/from-link/page.js
Original file line number Diff line number Diff line change
@@ -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 (
<>
<Link href={INVALID_URL}>invalid link</Link>
<Delay>
<h1>Hello, world!</h1>
</Delay>
</>
)
}
Original file line number Diff line number Diff line change
@@ -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 (
<Delay>
<h1>Hello, world!</h1>
</Delay>
)
}
8 changes: 8 additions & 0 deletions test/e2e/app-dir/app-prefetch/app/invalid-url/invalid-url.js
Original file line number Diff line number Diff line change
@@ -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 = '///'
18 changes: 18 additions & 0 deletions test/e2e/app-dir/app-prefetch/prefetching.test.ts
Original file line number Diff line number Diff line change
@@ -329,4 +329,22 @@ describe('app dir - prefetching', () => {
})
})
})

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 () => {
Copy link
Member Author

@lubieowoce lubieowoce Jun 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to change this later (for v15) and make router.prefetch not throw for invalid URLs -- arguably, prefetches shouldn't blow anything up even if they failed. But this matches existing behavior, and this PR is intended to go into the v14.2 backport branch, so i feel like we may want to preserve what was there for now.

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'
)
})
})
})

Unchanged files with check annotations Beta

}
}
/// Subscribes to lifecycle events of the compilation.

Check warning on line 886 in packages/next-swc/crates/napi/src/next_api/project.rs

GitHub Actions / rustdoc check / build

public documentation for `project_update_info_subscribe` links to private item `UpdateMessage::Start`

Check warning on line 886 in packages/next-swc/crates/napi/src/next_api/project.rs

GitHub Actions / rustdoc check / build

public documentation for `project_update_info_subscribe` links to private item `UpdateMessage::End`

Check warning on line 886 in packages/next-swc/crates/napi/src/next_api/project.rs

GitHub Actions / rustdoc check / build

public documentation for `project_update_info_subscribe` links to private item `UpdateMessage::End`

Check warning on line 886 in packages/next-swc/crates/napi/src/next_api/project.rs

GitHub Actions / rustdoc check / build

public documentation for `project_update_info_subscribe` links to private item `UpdateMessage::Start`
/// Emits an [UpdateMessage::Start] event when any computation starts.
/// Emits an [UpdateMessage::End] event when there was no computation for the
/// specified time (`aggregation_ms`). The [UpdateMessage::End] event contains