From 1e2493e0a3904a82c9dc4dab1133df048f33bfd5 Mon Sep 17 00:00:00 2001 From: Zack Tanner <1939140+ztanner@users.noreply.github.com> Date: Thu, 19 Sep 2024 13:06:46 -0700 Subject: [PATCH] bugfix: actions that redirect should reject the action promise (#69945) When calling `redirect` in a server action, the action handler will invoke a worker to fetch the RSC payload from the URL being redirected to, and will return it in the action request. This allows the redirect to be handled in a single request rather than requiring a second request to fetch the RSC payload on the URL being redirected to. As part of this special redirection handling, no action result is returned, so the action promise is resolved with `undefined`. As a result of this behavior, if your server action redirects to a page that has the same form that was just submitted, and you're leveraging `useActionState`, the updated state value will be undefined rather than being reset to the initial state. **More generally, redirect errors are handled by the client currently by simply resolving the promise with undefined, which isn't the correct behavior.** This PR will reject the server action promise with a redirect error, so that it'll be caught by our `RedirectBoundary`. Since React will remount the tree as part of the error boundary recovery, this effectively resets the form to it's initial state. If the action is rejected in a global `startTransition`, then it'll be handled by a global `error` handler, which will perform the redirect. And if it occurs outside of a transition, the redirect will be handled by a `unhandledrejection` handler. Because we're not able to apply the flight data as-is in the server action reducer (and instead defer to the navigation action), we seed the prefetch cache for the target page with the flight data from the action result so that we do not need to make another server roundtrip. We apply the same static/dynamic cache staleTime heuristics for these cache entries. Fixes #68549 Closes NDX-229 --- .../next/src/client/components/app-router.tsx | 33 ++++++++ .../internal/helpers/use-error-handler.ts | 5 ++ .../create-initial-router-state.ts | 7 +- .../router-reducer/prefetch-cache-utils.ts | 5 +- .../reducers/server-action-reducer.ts | 58 ++++++++++++-- packages/next/src/server/base-server.ts | 27 +++---- test/e2e/app-dir/actions/app-action.test.ts | 78 +++++++++++++++++-- .../app-dir/actions/app/redirect/actions.ts | 17 ++++ .../app-dir/actions/app/redirect/layout.tsx | 21 +++++ .../actions/app/redirect/other/page.tsx | 3 + .../e2e/app-dir/actions/app/redirect/page.tsx | 3 + .../actions/app/self-redirect/actions.ts | 17 ++++ .../actions/app/self-redirect/layout.tsx | 21 +++++ .../actions/app/self-redirect/other/page.tsx | 3 + .../actions/app/self-redirect/page.tsx | 3 + test/e2e/app-dir/app-basepath/index.test.ts | 12 ++- .../parallel-routes-revalidation.test.ts | 6 +- 17 files changed, 283 insertions(+), 36 deletions(-) create mode 100644 test/e2e/app-dir/actions/app/redirect/actions.ts create mode 100644 test/e2e/app-dir/actions/app/redirect/layout.tsx create mode 100644 test/e2e/app-dir/actions/app/redirect/other/page.tsx create mode 100644 test/e2e/app-dir/actions/app/redirect/page.tsx create mode 100644 test/e2e/app-dir/actions/app/self-redirect/actions.ts create mode 100644 test/e2e/app-dir/actions/app/self-redirect/layout.tsx create mode 100644 test/e2e/app-dir/actions/app/self-redirect/other/page.tsx create mode 100644 test/e2e/app-dir/actions/app/self-redirect/page.tsx diff --git a/packages/next/src/client/components/app-router.tsx b/packages/next/src/client/components/app-router.tsx index 78a66a3741ca6..42357ab3090ef 100644 --- a/packages/next/src/client/components/app-router.tsx +++ b/packages/next/src/client/components/app-router.tsx @@ -58,6 +58,12 @@ import type { FlightRouterState } from '../../server/app-render/types' import { useNavFailureHandler } from './nav-failure-handler' import { useServerActionDispatcher } from '../app-call-server' import type { AppRouterActionQueue } from '../../shared/lib/router/action-queue' +import { + getRedirectTypeFromError, + getURLFromRedirectError, + isRedirectError, + RedirectType, +} from './redirect' const globalMutable: { pendingMpaPath?: string @@ -369,6 +375,33 @@ function Router({ } }, [dispatch]) + useEffect(() => { + // Ensure that any redirect errors that bubble up outside of the RedirectBoundary + // are caught and handled by the router. + function handleUnhandledRedirect( + event: ErrorEvent | PromiseRejectionEvent + ) { + const error = 'reason' in event ? event.reason : event.error + if (isRedirectError(error)) { + event.preventDefault() + const url = getURLFromRedirectError(error) + const redirectType = getRedirectTypeFromError(error) + if (redirectType === RedirectType.push) { + appRouter.push(url, {}) + } else { + appRouter.replace(url, {}) + } + } + } + window.addEventListener('error', handleUnhandledRedirect) + window.addEventListener('unhandledrejection', handleUnhandledRedirect) + + return () => { + window.removeEventListener('error', handleUnhandledRedirect) + window.removeEventListener('unhandledrejection', handleUnhandledRedirect) + } + }, [appRouter]) + // When mpaNavigation flag is set do a hard navigation to the new url. // Infinitely suspend because we don't actually want to rerender any child // components with the new URL and any entangled state updates shouldn't diff --git a/packages/next/src/client/components/react-dev-overlay/internal/helpers/use-error-handler.ts b/packages/next/src/client/components/react-dev-overlay/internal/helpers/use-error-handler.ts index d8006f6cf0062..a84402337f497 100644 --- a/packages/next/src/client/components/react-dev-overlay/internal/helpers/use-error-handler.ts +++ b/packages/next/src/client/components/react-dev-overlay/internal/helpers/use-error-handler.ts @@ -58,6 +58,11 @@ if (typeof window !== 'undefined') { 'unhandledrejection', (ev: WindowEventMap['unhandledrejection']): void => { const reason = ev?.reason + if (isNextRouterError(reason)) { + ev.preventDefault() + return + } + if ( !reason || !(reason instanceof Error) || diff --git a/packages/next/src/client/components/router-reducer/create-initial-router-state.ts b/packages/next/src/client/components/router-reducer/create-initial-router-state.ts index 465349fdffdc2..ebfd4bc33dbdd 100644 --- a/packages/next/src/client/components/router-reducer/create-initial-router-state.ts +++ b/packages/next/src/client/components/router-reducer/create-initial-router-state.ts @@ -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' @@ -112,7 +112,7 @@ export function createInitialRouterState({ location.origin ) - createPrefetchCacheEntryForInitialLoad({ + createSeededPrefetchCacheEntry({ url, data: { flightData: [normalizedFlightData], @@ -125,6 +125,7 @@ export function createInitialRouterState({ tree: initialState.tree, prefetchCache: initialState.prefetchCache, nextUrl: initialState.nextUrl, + kind: PrefetchKind.AUTO, }) } diff --git a/packages/next/src/client/components/router-reducer/prefetch-cache-utils.ts b/packages/next/src/client/components/router-reducer/prefetch-cache-utils.ts index 5767ea5e914a9..a2133966c90a0 100644 --- a/packages/next/src/client/components/router-reducer/prefetch-cache-utils.ts +++ b/packages/next/src/client/components/router-reducer/prefetch-cache-utils.ts @@ -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 & { 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) diff --git a/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts index a65adaf756efc..803dc7f54175f 100644 --- a/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts +++ b/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts @@ -5,6 +5,7 @@ import type { import { callServer } from '../../../app-call-server' import { ACTION_HEADER, + NEXT_IS_PRERENDER_HEADER, NEXT_ROUTER_STATE_TREE_HEADER, NEXT_URL, RSC_CONTENT_TYPE_HEADER, @@ -21,11 +22,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' @@ -43,11 +45,16 @@ import { normalizeFlightData, type NormalizedFlightData, } from '../../../flight-data-helpers' +import { getRedirectError, RedirectType } from '../../redirect' +import { createSeededPrefetchCacheEntry } from '../prefetch-cache-utils' +import { removeBasePath } from '../../../remove-base-path' +import { hasBasePath } from '../../../has-base-path' type FetchServerActionResult = { redirectLocation: URL | undefined actionResult?: ActionResult actionFlightData?: NormalizedFlightData[] | string + isPrerender: boolean revalidatedParts: { tag: boolean cookie: boolean @@ -85,6 +92,7 @@ async function fetchServerAction( }) const location = res.headers.get('x-action-redirect') + const isPrerender = !!res.headers.get(NEXT_IS_PRERENDER_HEADER) let revalidatedParts: FetchServerActionResult['revalidatedParts'] try { const revalidatedHeader = JSON.parse( @@ -127,6 +135,7 @@ async function fetchServerAction( actionFlightData: normalizeFlightData(response.f), redirectLocation, revalidatedParts, + isPrerender, } } @@ -135,6 +144,7 @@ async function fetchServerAction( actionFlightData: normalizeFlightData(response.f), redirectLocation, revalidatedParts, + isPrerender, } } @@ -153,6 +163,7 @@ async function fetchServerAction( return { redirectLocation, revalidatedParts, + isPrerender, } } @@ -186,6 +197,7 @@ export function serverActionReducer( actionResult, actionFlightData: flightData, redirectLocation, + isPrerender, }) => { // Make sure the redirection is a push instead of a replace. // Issue: https://github.com/vercel/next.js/issues/53911 @@ -221,7 +233,41 @@ 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: isPrerender ? PrefetchKind.FULL : PrefetchKind.AUTO, + }) + + 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( + hasBasePath(newHref) ? removeBasePath(newHref) : newHref, + RedirectType.push + ) + ) + + return handleMutable(state, mutable) } for (const normalizedFlightData of flightData) { diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index 90faf34bd94b2..3391c0ca2dc95 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -3068,25 +3068,26 @@ export default abstract class Server< if ( isSSG && - !this.minimalMode && // We don't want to send a cache header for requests that contain dynamic // data. If this is a Dynamic RSC request or wasn't a Prefetch RSC // request, then we should set the cache header. !isDynamicRSCRequest && (!didPostpone || isPrefetchRSCRequest) ) { - // set x-nextjs-cache header to match the header - // we set for the image-optimizer - res.setHeader( - 'x-nextjs-cache', - isOnDemandRevalidate - ? 'REVALIDATED' - : cacheEntry.isMiss - ? 'MISS' - : cacheEntry.isStale - ? 'STALE' - : 'HIT' - ) + if (!this.minimalMode) { + // set x-nextjs-cache header to match the header + // we set for the image-optimizer + res.setHeader( + 'x-nextjs-cache', + isOnDemandRevalidate + ? 'REVALIDATED' + : cacheEntry.isMiss + ? 'MISS' + : cacheEntry.isStale + ? 'STALE' + : 'HIT' + ) + } // Set a header used by the client router to signal the response is static // and should respect the `static` cache staleTime value. res.setHeader(NEXT_IS_PRERENDER_HEADER, '1') diff --git a/test/e2e/app-dir/actions/app-action.test.ts b/test/e2e/app-dir/actions/app-action.test.ts index 376e10fab86ed..0f7a795c5679a 100644 --- a/test/e2e/app-dir/actions/app-action.test.ts +++ b/test/e2e/app-dir/actions/app-action.test.ts @@ -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) { @@ -899,9 +957,13 @@ describe('app-dir action handling', () => { 'redirected' ) - // no other requests should be made - expect(requests).toHaveLength(1) - expect(responses).toHaveLength(1) + // This verifies the redirect & server response happens in a single roundtrip, + // if the redirect resource was static. In development, these responses are always + // dynamically generated, so we only expect a single request for build/deploy. + if (!isNextDev) { + expect(requests).toHaveLength(1) + expect(responses).toHaveLength(1) + } const request = requests[0] const response = responses[0] @@ -998,9 +1060,13 @@ describe('app-dir action handling', () => { await browser.elementById(`redirect-${redirectType}`).click() await check(() => browser.url(), `${next.url}${destinationPagePath}`) - // no other requests should be made - expect(requests).toHaveLength(1) - expect(responses).toHaveLength(1) + // This verifies the redirect & server response happens in a single roundtrip, + // if the redirect resource was static. In development, these responses are always + // dynamically generated, so we only expect a single request for build/deploy. + if (!isNextDev) { + expect(requests).toHaveLength(1) + expect(responses).toHaveLength(1) + } const request = requests[0] const response = responses[0] diff --git a/test/e2e/app-dir/actions/app/redirect/actions.ts b/test/e2e/app-dir/actions/app/redirect/actions.ts new file mode 100644 index 0000000000000..f47a8d45da1ac --- /dev/null +++ b/test/e2e/app-dir/actions/app/redirect/actions.ts @@ -0,0 +1,17 @@ +'use server' + +import { redirect } from 'next/navigation' + +type State = { + errors: Record +} + +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') +} diff --git a/test/e2e/app-dir/actions/app/redirect/layout.tsx b/test/e2e/app-dir/actions/app/redirect/layout.tsx new file mode 100644 index 0000000000000..ad419941eebc7 --- /dev/null +++ b/test/e2e/app-dir/actions/app/redirect/layout.tsx @@ -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 ( +
+
+ + + {errors.name &&

{errors.name}

} +
+ {children} +
+ ) +} diff --git a/test/e2e/app-dir/actions/app/redirect/other/page.tsx b/test/e2e/app-dir/actions/app/redirect/other/page.tsx new file mode 100644 index 0000000000000..187b865111066 --- /dev/null +++ b/test/e2e/app-dir/actions/app/redirect/other/page.tsx @@ -0,0 +1,3 @@ +export default function Page() { + return
Other Page
+} diff --git a/test/e2e/app-dir/actions/app/redirect/page.tsx b/test/e2e/app-dir/actions/app/redirect/page.tsx new file mode 100644 index 0000000000000..b2bb7da42de65 --- /dev/null +++ b/test/e2e/app-dir/actions/app/redirect/page.tsx @@ -0,0 +1,3 @@ +export default function Page() { + return
Main Page
+} diff --git a/test/e2e/app-dir/actions/app/self-redirect/actions.ts b/test/e2e/app-dir/actions/app/self-redirect/actions.ts new file mode 100644 index 0000000000000..0ea200e257077 --- /dev/null +++ b/test/e2e/app-dir/actions/app/self-redirect/actions.ts @@ -0,0 +1,17 @@ +'use server' + +import { redirect } from 'next/navigation' + +type State = { + errors: Record +} + +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') +} diff --git a/test/e2e/app-dir/actions/app/self-redirect/layout.tsx b/test/e2e/app-dir/actions/app/self-redirect/layout.tsx new file mode 100644 index 0000000000000..ad419941eebc7 --- /dev/null +++ b/test/e2e/app-dir/actions/app/self-redirect/layout.tsx @@ -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 ( +
+
+ + + {errors.name &&

{errors.name}

} +
+ {children} +
+ ) +} diff --git a/test/e2e/app-dir/actions/app/self-redirect/other/page.tsx b/test/e2e/app-dir/actions/app/self-redirect/other/page.tsx new file mode 100644 index 0000000000000..187b865111066 --- /dev/null +++ b/test/e2e/app-dir/actions/app/self-redirect/other/page.tsx @@ -0,0 +1,3 @@ +export default function Page() { + return
Other Page
+} diff --git a/test/e2e/app-dir/actions/app/self-redirect/page.tsx b/test/e2e/app-dir/actions/app/self-redirect/page.tsx new file mode 100644 index 0000000000000..b2bb7da42de65 --- /dev/null +++ b/test/e2e/app-dir/actions/app/self-redirect/page.tsx @@ -0,0 +1,3 @@ +export default function Page() { + return
Main Page
+} diff --git a/test/e2e/app-dir/app-basepath/index.test.ts b/test/e2e/app-dir/app-basepath/index.test.ts index c66eaf4e5282b..f7161723f8254 100644 --- a/test/e2e/app-dir/app-basepath/index.test.ts +++ b/test/e2e/app-dir/app-basepath/index.test.ts @@ -3,7 +3,7 @@ import { check, retry } from 'next-test-utils' import type { Request, Response } from 'playwright' describe('app dir - basepath', () => { - const { next } = nextTestSetup({ + const { next, isNextDev } = nextTestSetup({ files: __dirname, dependencies: { sass: 'latest', @@ -132,9 +132,13 @@ describe('app dir - basepath', () => { expect(await browser.waitForElementByCss('#page-2').text()).toBe(`Page 2`) - // verify that the POST request was only made to the action handler - expect(requests).toHaveLength(1) - expect(responses).toHaveLength(1) + // This verifies the redirect & server response happens in a single roundtrip, + // if the redirect resource was static. In development, these responses are always + // dynamically generated, so we only expect a single request for build/deploy. + if (!isNextDev) { + expect(requests).toHaveLength(1) + expect(responses).toHaveLength(1) + } const request = requests[0] const response = responses[0] diff --git a/test/e2e/app-dir/parallel-routes-revalidation/parallel-routes-revalidation.test.ts b/test/e2e/app-dir/parallel-routes-revalidation/parallel-routes-revalidation.test.ts index 257a02b897805..cdbda05745b21 100644 --- a/test/e2e/app-dir/parallel-routes-revalidation/parallel-routes-revalidation.test.ts +++ b/test/e2e/app-dir/parallel-routes-revalidation/parallel-routes-revalidation.test.ts @@ -2,7 +2,7 @@ import { nextTestSetup } from 'e2e-utils' import { check, retry } from 'next-test-utils' describe('parallel-routes-revalidation', () => { - const { next, isNextStart, isNextDeploy } = nextTestSetup({ + const { next, isNextDev, isNextStart, isNextDeploy } = nextTestSetup({ files: __dirname, }) @@ -447,7 +447,9 @@ describe('parallel-routes-revalidation', () => { await browser.waitForIdleNetwork() await retry(async () => { - expect(rscRequests.length).toBe(0) + if (!isNextDev) { + expect(rscRequests.length).toBe(0) + } if (isNextStart) { expect(prefetchRequests.length).toBe(4)