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

[form] allow turning prefetching off #68305

Merged
merged 5 commits into from
Sep 27, 2024
Merged
Show file tree
Hide file tree
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
37 changes: 32 additions & 5 deletions packages/next/src/client/form.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,18 @@ type InternalFormProps = {
* - If `action` is a function, it will be called when the form is submitted. See the [React docs](https://react.dev/reference/react-dom/components/form#props) for more.
*/
action: NonNullable<HTMLFormProps['action']>
/**
* Controls how the route specified by `action` is prefetched.
* Any `<Form />` that is in the viewport (initially or through scroll) will be prefetched.
* Prefetch can be disabled by passing `prefetch={false}`. Prefetching is only enabled in production.
*
* Options:
* - `null` (default): For statically generated pages, this will prefetch the full React Server Component data. For dynamic pages, this will prefetch up to the nearest route segment with a [`loading.js`](https://nextjs.org/docs/app/api-reference/file-conventions/loading) file. If there is no loading file, it will not fetch the full tree to avoid fetching too much data.
* - `false`: This will not prefetch any data.
*
* @defaultValue `null`
*/
prefetch?: false | null
/**
* Whether submitting the form should replace the current `history` state instead of adding a new url into the stack.
* Only valid if `action` is a string.
Expand All @@ -45,12 +57,21 @@ export type FormProps<RouteInferType = any> = InternalFormProps
export default function Form({
replace,
scroll,
prefetch: prefetchProp = null,
ref: externalRef,
...props
}: FormProps) {
const actionProp = props.action
const isNavigatingForm = typeof actionProp === 'string'

if (process.env.NODE_ENV === 'development') {
if (!(prefetchProp === false || prefetchProp === null)) {
console.error('The `prefetch` prop of <Form> must be `false` or `null`')
}
}
const prefetch =
prefetchProp === false || prefetchProp === null ? prefetchProp : null

for (const key of DISALLOWED_FORM_PROPS) {
if (key in props) {
if (process.env.NODE_ENV === 'development') {
Expand Down Expand Up @@ -83,18 +104,19 @@ export default function Form({
return
}

if (!isVisible) {
const isPrefetchEnabled = prefetch === null

if (!isVisible || !isPrefetchEnabled) {
return
}

try {
// TODO: do we need to take the current field values here?
// or are we assuming that queryparams can't affect this (but what about rewrites)?
router.prefetch(actionProp, { kind: PrefetchKind.AUTO })
const prefetchKind = PrefetchKind.AUTO
router.prefetch(actionProp, { kind: prefetchKind })
} catch (err) {
console.error(err)
}
}, [isNavigatingForm, isVisible, actionProp, router])
}, [isNavigatingForm, isVisible, actionProp, prefetch, router])

if (!isNavigatingForm) {
if (process.env.NODE_ENV === 'development') {
Expand All @@ -106,6 +128,11 @@ export default function Form({
' `router.replace()` - https://nextjs.org/docs/app/api-reference/functions/use-router#userouter\n'
)
}
if (prefetchProp !== undefined) {
console.error(
'Passing `prefetch` to a <Form> whose `action` is a function has no effect.'
)
}
}
return <form {...props} ref={ownRef} />
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import * as React from 'react'
import Form from 'next/form'

export default function Home() {
return (
<Form action="/search" prefetch={false} id="search-form">
<input name="query" />
<button type="submit">Submit</button>
</Form>
)
}
257 changes: 257 additions & 0 deletions test/e2e/app-dir/next-form/default/next-form-prefetch.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,257 @@
import { nextTestSetup, isNextDev } from 'e2e-utils'
import {
NEXT_ROUTER_PREFETCH_HEADER,
NEXT_RSC_UNION_QUERY,
RSC_HEADER,
} from 'next/src/client/components/app-router-headers'
import type { Route, Page, Request as PlaywrightRequest } from 'playwright'
import { WebdriverOptions } from '../../../../lib/next-webdriver'
import { retry } from '../../../../lib/next-test-utils'

const _describe =
// prefetching is disabled in dev.
isNextDev ||
// FIXME(form): currently failing in PPR, unflag this when https://github.com/vercel/next.js/pull/70532 is ready
process.env.__NEXT_EXPERIMENTAL_PPR
? describe.skip
: describe
Comment on lines +14 to +17
Copy link
Member Author

@lubieowoce lubieowoce Sep 26, 2024

Choose a reason for hiding this comment

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

This is waiting on #70532. The bug was there before, we just didn't have a test that ran into it. Don't think it makes sense to block this feature


_describe('app dir - form prefetching', () => {
const { next } = nextTestSetup({
files: __dirname,
})

it("should prefetch a loading state for the form's target", async () => {
const interceptor = new RequestInterceptor({
pattern: '**/search*',
requestFilter: async (request: PlaywrightRequest) => {
// only capture RSC requests that *aren't* prefetches
const headers = await request.allHeaders()
const isRSC = headers[RSC_HEADER.toLowerCase()] === '1'
const isPrefetch = !!headers[NEXT_ROUTER_PREFETCH_HEADER.toLowerCase()]
return isRSC && !isPrefetch
},
log: true,
})

const session = await next.browser('/forms/basic', {
beforePageLoad: interceptor.beforePageLoad,
})

await session.waitForIdleNetwork()
interceptor.start()

const searchInput = await session.elementByCss('input[name="query"]')
const query = 'my search'
await searchInput.fill(query)

const submitButton = await session.elementByCss('[type="submit"]')
await submitButton.click()

const targetHref = '/search?' + new URLSearchParams({ query })

// we're blocking requests, so the navigation won't go through,
// but we should still see the prefetched loading state
expect(await session.waitForElementByCss('#loading').text()).toBe(
'Loading...'
)

// We only resolve the dynamic request after we've confirmed loading exists,
// to avoid a race where the dynamic request handles the loading state instead.
const navigationRequest = interceptor.pendingRequests.get(targetHref)
expect(navigationRequest).toBeDefined()
// allow the navigation to continue
await navigationRequest.resolve()

const result = await session.waitForElementByCss('#search-results').text()
expect(result).toInclude(`query: "${query}"`)
})

it('should not prefetch when prefetch is set to false`', async () => {
const interceptor = new RequestInterceptor({
pattern: '**/search*',
requestFilter: async (request: PlaywrightRequest) => {
// capture all RSC requests, prefetch or not
const headers = await request.allHeaders()
const isRSC = headers[RSC_HEADER.toLowerCase()] === '1'
return isRSC
},
log: true,
})

interceptor.start()

const session = await next.browser('/forms/prefetch-false', {
beforePageLoad: interceptor.beforePageLoad,
})

await session.waitForIdleNetwork()

// no prefetches should have been captured
expect(interceptor.pendingRequests).toBeEmpty()

const searchInput = await session.elementByCss('input[name="query"]')
const query = 'my search'
await searchInput.fill(query)

const submitButton = await session.elementByCss('[type="submit"]')
await submitButton.click()

const targetHref = '/search?' + new URLSearchParams({ query })

// wait for the pending request to appear
const navigationRequest = await retry(
async () => {
const request = interceptor.pendingRequests.get(targetHref)
expect(request).toBeDefined()
return request
},
undefined,
100
)

// the loading state should not be visible, because we didn't prefetch it
expect(await session.elementsByCss('#loading')).toEqual([])

// allow the navigation to continue
navigationRequest.resolve()

// now, the loading state should stream in dynamically
expect(await session.waitForElementByCss('#loading').text()).toBe(
'Loading...'
)

const result = await session.waitForElementByCss('#search-results').text()
expect(result).toInclude(`query: "${query}"`)
})
})

// =====================================================================

type PlaywrightRoutePattern = Parameters<Page['route']>[0]
type PlaywrightRouteOptions = Parameters<Page['route']>[2]
type BeforePageLoadFn = NonNullable<WebdriverOptions['beforePageLoad']>

type RequestInterceptorOptions = {
pattern: PlaywrightRoutePattern
requestFilter?: (request: PlaywrightRequest) => boolean | Promise<boolean>
timeout?: number
log?: boolean
routeOptions?: PlaywrightRouteOptions
}

type InterceptedRequest = {
request: PlaywrightRequest
resolve(): Promise<void>
}

class RequestInterceptor {
pendingRequests: Map<string, InterceptedRequest> = new Map()
isEnabled = false
constructor(private opts: RequestInterceptorOptions) {}

private getRequestKey(request: PlaywrightRequest) {
// could cause issues for a generic util,
// but it's fine for the purposes of intercepting navigations.
// if we wanted this to be more generic, we could expose a `getRequest(...)` method
// to allow partial matching (e.g. by pathname + search only)
const url = new URL(request.url())
url.searchParams.delete(NEXT_RSC_UNION_QUERY)
return url.pathname + url.search
}

start() {
this.isEnabled = true
}

stop() {
this.isEnabled = false
}

beforePageLoad: BeforePageLoadFn = (page: Page) => {
const { opts } = this
page.route(
opts.pattern,
async (route: Route, request) => {
if (!this.isEnabled) {
return route.continue()
}

const shouldIntercept = opts.requestFilter
? opts.requestFilter(request)
: true

const url = request.url()

if (!shouldIntercept) {
if (opts.log) {
console.log('[interceptor] not intercepting:', url)
}
return route.continue()
}

const requestKey = this.getRequestKey(request)

if (opts.log) {
console.log(
'[interceptor] intercepting request:',
url,
`(key: ${requestKey})`
)
}

if (this.pendingRequests.has(requestKey)) {
throw new Error(
`Interceptor received duplicate request (key: ${JSON.stringify(requestKey)}, url: ${JSON.stringify(url)})`
)
}

// Create a promise that will be resolved by the later test code
const blocked = promiseWithResolvers<void>()

this.pendingRequests.set(requestKey, {
request,
resolve: async () => {
if (opts.log) {
console.log('[interceptor] resolving intercepted request:', url)
}
this.pendingRequests.delete(requestKey)
await route.continue()
// wait a moment to ensure the response is received
await new Promise((res) => setTimeout(res, 500))
blocked.resolve()
},
})

// Await the promise to effectively stall the request
const timeout = opts.timeout ?? 5000
await Promise.race([
blocked.promise,
timeoutPromise(
opts.timeout ?? 5000,
`Intercepted request for '${url}' was not resolved within ${timeout}ms`
),
])
},
opts.routeOptions
)
}
}

function promiseWithResolvers<T>() {
let resolve: (value: T) => void = undefined!
let reject: (error: unknown) => void = undefined!
const promise = new Promise((_resolve, _reject) => {
resolve = _resolve
reject = _reject
})
return { promise, resolve, reject }
}

function timeoutPromise(duration: number, message = 'Timeout') {
return new Promise<never>((_, reject) =>
AbortSignal.timeout(duration).addEventListener('abort', () =>
reject(new Error(message))
)
)
}
Loading