Skip to content

Commit

Permalink
bugfix: actions that self-redirect should be handled gracefully
Browse files Browse the repository at this point in the history
  • Loading branch information
ztanner committed Sep 11, 2024
1 parent f760882 commit e311f1e
Show file tree
Hide file tree
Showing 12 changed files with 190 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ import type { FlightDataPath } from '../../../server/app-render/types'
import { createHrefFromUrl } from './create-href-from-url'
import { fillLazyItemsTillLeafWithHead } from './fill-lazy-items-till-leaf-with-head'
import { extractPathFromFlightRouterState } from './compute-changed-path'
import { createPrefetchCacheEntryForInitialLoad } from './prefetch-cache-utils'
import type { PrefetchCacheEntry } from './router-reducer-types'
import { createSeededPrefetchCacheEntry } from './prefetch-cache-utils'
import { PrefetchKind, type PrefetchCacheEntry } from './router-reducer-types'
import { addRefreshMarkerToActiveParallelSegments } from './refetch-inactive-parallel-segments'
import { getFlightDataPartsFromPath } from '../../flight-data-helpers'

Expand Down Expand Up @@ -112,7 +112,7 @@ export function createInitialRouterState({
location.origin
)

createPrefetchCacheEntryForInitialLoad({
createSeededPrefetchCacheEntry({
url,
data: {
flightData: [normalizedFlightData],
Expand All @@ -125,6 +125,7 @@ export function createInitialRouterState({
tree: initialState.tree,
prefetchCache: initialState.prefetchCache,
nextUrl: initialState.nextUrl,
kind: PrefetchKind.AUTO,
})
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -249,19 +249,20 @@ function prefixExistingPrefetchCacheEntry({
/**
* Use to seed the prefetch cache with data that has already been fetched.
*/
export function createPrefetchCacheEntryForInitialLoad({
export function createSeededPrefetchCacheEntry({
nextUrl,
tree,
prefetchCache,
url,
data,
kind,
}: Pick<ReadonlyReducerState, 'nextUrl' | 'tree' | 'prefetchCache'> & {
url: URL
data: FetchServerResponseResult
kind: PrefetchKind
}) {
// The initial cache entry technically includes full data, but it isn't explicitly prefetched -- we just seed the
// prefetch cache so that we can skip an extra prefetch request later, since we already have the data.
const kind = PrefetchKind.AUTO
// if the prefetch corresponds with an interception route, we use the nextUrl to prefix the cache key
const prefetchCacheKey = data.couldBeIntercepted
? createPrefetchCacheKey(url, kind, nextUrl)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,12 @@ const { createFromFetch, encodeReply } = (
require('react-server-dom-webpack/client')
) as typeof import('react-server-dom-webpack/client')

import type {
ReadonlyReducerState,
ReducerState,
ServerActionAction,
ServerActionMutable,
import {
PrefetchKind,
type ReadonlyReducerState,
type ReducerState,
type ServerActionAction,
type ServerActionMutable,
} from '../router-reducer-types'
import { addBasePath } from '../../../add-base-path'
import { createHrefFromUrl } from '../create-href-from-url'
Expand All @@ -43,6 +44,8 @@ import {
normalizeFlightData,
type NormalizedFlightData,
} from '../../../flight-data-helpers'
import { getRedirectError, RedirectType } from '../../redirect'
import { createSeededPrefetchCacheEntry } from '../prefetch-cache-utils'

type FetchServerActionResult = {
redirectLocation: URL | undefined
Expand Down Expand Up @@ -221,7 +224,35 @@ export function serverActionReducer(

if (redirectLocation) {
const newHref = createHrefFromUrl(redirectLocation, false)
mutable.canonicalUrl = newHref

// Because the RedirectBoundary will trigger a navigation, we need to seed the prefetch cache
// with the FlightData that we got from the server action for the target page, so that it's
// available when the page is navigated to and doesn't need to be re-fetched.
createSeededPrefetchCacheEntry({
url: redirectLocation,
data: {
flightData,
canonicalUrl: undefined,
couldBeIntercepted: false,
isPrerender: false,
postponed: false,
},
tree: state.tree,
prefetchCache: state.prefetchCache,
nextUrl: state.nextUrl,
kind: PrefetchKind.FULL,
})

mutable.prefetchCache = state.prefetchCache
// If the action triggered a redirect, we reject the action promise with a redirect
// so that it's handled by RedirectBoundary as we won't have a valid
// action result to resolve the promise with. This will effectively reset the state of
// the component that called the action as the error boundary will remount the tree.
// The status code doesn't matter here as the action handler will have already sent
// a response with the correct status code.
reject(getRedirectError(newHref, RedirectType.push))

return handleMutable(state, mutable)
}

for (const normalizedFlightData of flightData) {
Expand Down
58 changes: 58 additions & 0 deletions test/e2e/app-dir/actions/app-action.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,64 @@ describe('app-dir action handling', () => {
await check(() => browser.elementByCss('h1').text(), 'Transition is: idle')
})

it('should reset the form state when the action redirects to a page that contains the same form', async () => {
const browser = await next.browser('/redirect')
const input = await browser.elementByCss('input[name="name"]')
const submit = await browser.elementByCss('button')

expect(await browser.hasElementByCssSelector('#error')).toBe(false)

await input.fill('foo')
await submit.click()

// The server action will fail validation and will return error state
// verify that the error state is displayed
await retry(async () => {
expect(await browser.hasElementByCssSelector('#error')).toBe(true)
expect(await browser.elementByCss('#error').text()).toBe(
"Only 'justputit' is accepted."
)
})

// The server action won't return an error state, it will just call redirect to itself
// Validate that the form state is reset
await input.fill('justputit')
await submit.click()

await retry(async () => {
expect(await browser.hasElementByCssSelector('#error')).toBe(false)
})
})

it('should reset the form state when the action redirects to itself', async () => {
const browser = await next.browser('/self-redirect')
const input = await browser.elementByCss('input[name="name"]')
const submit = await browser.elementByCss('button')

expect(await browser.hasElementByCssSelector('#error')).toBe(false)

await input.fill('foo')
await submit.click()

// The server action will fail validation and will return error state
// verify that the error state is displayed
await retry(async () => {
expect(await browser.hasElementByCssSelector('#error')).toBe(true)
expect(await browser.elementByCss('#error').text()).toBe(
"Only 'justputit' is accepted."
)
})

// The server action won't return an error state, it will just call redirect to itself
// Validate that the form state is reset
await input.fill('justputit')
await submit.click()

await retry(async () => {
expect(await browser.hasElementByCssSelector('#error')).toBe(false)
})
})

// This is disabled when deployed because the 404 page will be served as a static route
// which will not support POST requests, and will return a 405 instead.
if (!isNextDeploy) {
Expand Down
17 changes: 17 additions & 0 deletions test/e2e/app-dir/actions/app/redirect/actions.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
'use server'

import { redirect } from 'next/navigation'

type State = {
errors: Record<string, string>
}

export async function action(previousState: State, formData: FormData) {
const name = formData.get('name')

if (name !== 'justputit') {
return { errors: { name: "Only 'justputit' is accepted." } }
}

redirect('/redirect/other')
}
21 changes: 21 additions & 0 deletions test/e2e/app-dir/actions/app/redirect/layout.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
'use client'

import React, { useActionState } from 'react'
import { action } from './actions'

export default function Page({ children }) {
const [{ errors }, dispatch] = useActionState(action, {
errors: { name: '' },
})

return (
<div>
<form action={dispatch}>
<input type="text" name="name" />
<button type="submit">Submit</button>
{errors.name && <p id="error">{errors.name}</p>}
</form>
{children}
</div>
)
}
3 changes: 3 additions & 0 deletions test/e2e/app-dir/actions/app/redirect/other/page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <div>Other Page</div>
}
3 changes: 3 additions & 0 deletions test/e2e/app-dir/actions/app/redirect/page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <div>Main Page</div>
}
17 changes: 17 additions & 0 deletions test/e2e/app-dir/actions/app/self-redirect/actions.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
'use server'

import { redirect } from 'next/navigation'

type State = {
errors: Record<string, string>
}

export async function action(previousState: State, formData: FormData) {
const name = formData.get('name')

if (name !== 'justputit') {
return { errors: { name: "Only 'justputit' is accepted." } }
}

redirect('/self-redirect')
}
21 changes: 21 additions & 0 deletions test/e2e/app-dir/actions/app/self-redirect/layout.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
'use client'

import React, { useActionState } from 'react'
import { action } from './actions'

export default function Page({ children }) {
const [{ errors }, dispatch] = useActionState(action, {
errors: { name: '' },
})

return (
<div>
<form action={dispatch}>
<input type="text" name="name" />
<button type="submit">Submit</button>
{errors.name && <p id="error">{errors.name}</p>}
</form>
{children}
</div>
)
}
3 changes: 3 additions & 0 deletions test/e2e/app-dir/actions/app/self-redirect/other/page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <div>Other Page</div>
}
3 changes: 3 additions & 0 deletions test/e2e/app-dir/actions/app/self-redirect/page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <div>Main Page</div>
}

0 comments on commit e311f1e

Please sign in to comment.