Skip to content

Commit

Permalink
fix: server actions initiated from static pages (#51534)
Browse files Browse the repository at this point in the history
### What?
Pages marked with `generateStaticParams` don't currently support server actions, and instead return a 405 Method Not Allowed, with no action being taken on the client. Additionally, pages that are marked static & use server actions are opted into dynamic rendering.

### Why?
The page that has `generateStaticParams` is marked as `isSSG` [here](https://github.com/ztanner/next.js/blob/ee2ec3dd1de2f5cdd26ddc64c1036f02c92e1888/packages/next/src/server/base-server.ts#L1337).

As a result, the request is short-circuited because a POST request isn't supported on static pages. Upon detecting a server action on a page marked SSG, we bypass the static cache and go straight to the lambda. 

This PR introduces an experimental option to the prerender manifest that will allow for selectively bypassing the static cache

This also removes the need to bail out of static generation

Closes NEXT-1167
Closes NEXT-1453
Fixes #49408
Fixes #52840
Fixes #50932
  • Loading branch information
ztanner authored Sep 14, 2023
1 parent be38d02 commit a73abad
Show file tree
Hide file tree
Showing 13 changed files with 982 additions and 382 deletions.
102 changes: 50 additions & 52 deletions packages/next/src/build/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import loadCustomRoutes, {
normalizeRouteRegex,
Redirect,
Rewrite,
RouteHas,
RouteType,
} from '../lib/load-custom-routes'
import { getRedirectStatus, modifyRouteRegex } from '../lib/redirect-status'
Expand Down Expand Up @@ -130,6 +131,7 @@ import { flatReaddir } from '../lib/flat-readdir'
import { eventSwcPlugins } from '../telemetry/events/swc-plugins'
import { normalizeAppPath } from '../shared/lib/router/utils/app-paths'
import {
ACTION,
NEXT_ROUTER_PREFETCH,
RSC,
RSC_CONTENT_TYPE_HEADER,
Expand Down Expand Up @@ -160,13 +162,15 @@ export type SsgRoute = {
dataRoute: string | null
initialStatus?: number
initialHeaders?: Record<string, string>
experimentalBypassFor?: RouteHas[]
}

export type DynamicSsgRoute = {
routeRegex: string
fallback: string | null | false
dataRoute: string | null
dataRouteRegex: string | null
experimentalBypassFor?: RouteHas[]
}

export type PrerenderManifest = {
Expand Down Expand Up @@ -1612,14 +1616,6 @@ export default async function build(
`Using edge runtime on a page currently disables static generation for that page`
)
} else {
// If a page has action and it is static, we need to
// change it to SSG to keep the worker created.
// TODO: This is a workaround for now, we should have a
// dedicated worker defined in a heuristic way.
const hasAction = entriesWithAction?.has(
'app' + originalAppPath
)

if (
workerResult.encodedPrerenderRoutes &&
workerResult.prerenderRoutes
Expand All @@ -1638,47 +1634,39 @@ export default async function build(

const appConfig = workerResult.appConfig || {}
if (appConfig.revalidate !== 0) {
if (hasAction) {
Log.warnOnce(
`Using server actions on a page currently disables static generation for that page`
const isDynamic = isDynamicRoute(page)
const hasGenerateStaticParams =
!!workerResult.prerenderRoutes?.length

if (
config.output === 'export' &&
isDynamic &&
!hasGenerateStaticParams
) {
throw new Error(
`Page "${page}" is missing "generateStaticParams()" so it cannot be used with "output: export" config.`
)
} else {
const isDynamic = isDynamicRoute(page)
const hasGenerateStaticParams =
!!workerResult.prerenderRoutes?.length

if (
config.output === 'export' &&
isDynamic &&
!hasGenerateStaticParams
) {
throw new Error(
`Page "${page}" is missing "generateStaticParams()" so it cannot be used with "output: export" config.`
)
}

if (
// Mark the app as static if:
// - It has no dynamic param
// - It doesn't have generateStaticParams but `dynamic` is set to
// `error` or `force-static`
!isDynamic
) {
appStaticPaths.set(originalAppPath, [page])
appStaticPathsEncoded.set(originalAppPath, [
page,
])
isStatic = true
} else if (
isDynamic &&
!hasGenerateStaticParams &&
(appConfig.dynamic === 'error' ||
appConfig.dynamic === 'force-static')
) {
appStaticPaths.set(originalAppPath, [])
appStaticPathsEncoded.set(originalAppPath, [])
isStatic = true
}
}

if (
// Mark the app as static if:
// - It has no dynamic param
// - It doesn't have generateStaticParams but `dynamic` is set to
// `error` or `force-static`
!isDynamic
) {
appStaticPaths.set(originalAppPath, [page])
appStaticPathsEncoded.set(originalAppPath, [page])
isStatic = true
} else if (
isDynamic &&
!hasGenerateStaticParams &&
(appConfig.dynamic === 'error' ||
appConfig.dynamic === 'force-static')
) {
appStaticPaths.set(originalAppPath, [])
appStaticPathsEncoded.set(originalAppPath, [])
isStatic = true
}
}

Expand Down Expand Up @@ -2681,6 +2669,17 @@ export default async function build(

const isRouteHandler = isAppRouteRoute(originalAppPath)

// this flag is used to selectively bypass the static cache and invoke the lambda directly
// to enable server actions on static routes
const bypassFor: RouteHas[] = [
{ type: 'header', key: ACTION },
{
type: 'header',
key: 'content-type',
value: 'multipart/form-data',
},
]

routes.forEach((route) => {
if (isDynamicRoute(page) && route === page) return
if (route === '/_not-found') return
Expand Down Expand Up @@ -2708,10 +2707,7 @@ export default async function build(
? null
: path.posix.join(`${normalizedRoute}.rsc`)

const routeMeta: {
initialStatus?: SsgRoute['initialStatus']
initialHeaders?: SsgRoute['initialHeaders']
} = {}
const routeMeta: Partial<SsgRoute> = {}

const exportRouteMeta: {
status?: number
Expand Down Expand Up @@ -2748,6 +2744,7 @@ export default async function build(

finalPrerenderRoutes[route] = {
...routeMeta,
experimentalBypassFor: bypassFor,
initialRevalidateSeconds: revalidate,
srcRoute: page,
dataRoute,
Expand All @@ -2771,6 +2768,7 @@ export default async function build(
// TODO: create a separate manifest to allow enforcing
// dynamicParams for non-static paths?
finalDynamicRoutes[page] = {
experimentalBypassFor: bypassFor,
routeRegex: normalizeRouteRegex(
getNamedRouteRegex(page, false).re.source
),
Expand Down
10 changes: 10 additions & 0 deletions packages/next/src/server/app-render/app-render.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,16 @@ export async function renderToHTMLOrFlight(
appDirDevErrorLogger,
} = renderOpts

// We need to expose the bundled `require` API globally for
// react-server-dom-webpack. This is a hack until we find a better way.
if (ComponentMod.__next_app__) {
// @ts-ignore
globalThis.__next_require__ = ComponentMod.__next_app__.require

// @ts-ignore
globalThis.__next_chunk_load__ = ComponentMod.__next_app__.loadChunk
}

const extraRenderResultMeta: RenderResultMetadata = {}

const appUsingSizeAdjust = !!nextFontManifest?.appUsingSizeAdjust
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,6 @@ export function createServerComponentRenderer<Props>(
serverComponentsErrorHandler: ReturnType<typeof createErrorHandler>,
nonce?: string
): (props: Props) => JSX.Element {
// We need to expose the bundled `require` API globally for
// react-server-dom-webpack. This is a hack until we find a better way.
if (ComponentMod.__next_app__) {
// @ts-ignore
globalThis.__next_require__ = ComponentMod.__next_app__.require

// @ts-ignore
globalThis.__next_chunk_load__ = ComponentMod.__next_app__.loadChunk
}

let RSCStream: ReadableStream<Uint8Array>
const createRSCStream = (props: Props) => {
if (!RSCStream) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export type StaticGenerationContext = {
nextExport?: boolean
fetchCache?: StaticGenerationStore['fetchCache']
isDraftMode?: boolean
isServerAction?: boolean

/**
* A hack around accessing the store value outside the context of the
Expand Down Expand Up @@ -49,11 +50,15 @@ export const StaticGenerationAsyncStorageWrapper: AsyncStorageWrapper<
*
* 3.) If the request is in draft mode, we must generate dynamic HTML.
*
* 4.) If the request is a server action, we must generate dynamic HTML.
*
* These rules help ensure that other existing features like request caching,
* coalescing, and ISR continue working as intended.
*/
const isStaticGeneration =
!renderOpts.supportsDynamicHTML && !renderOpts.isDraftMode
!renderOpts.supportsDynamicHTML &&
!renderOpts.isDraftMode &&
!renderOpts.isServerAction

const store: StaticGenerationStore = {
isStaticGeneration,
Expand Down
17 changes: 14 additions & 3 deletions packages/next/src/server/base-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ import {
RSC_VARY_HEADER,
FLIGHT_PARAMETERS,
NEXT_RSC_UNION_QUERY,
ACTION,
NEXT_ROUTER_PREFETCH,
RSC_CONTENT_TYPE_HEADER,
} from '../client/components/app-router-headers'
Expand Down Expand Up @@ -1588,7 +1589,15 @@ export default abstract class Server<ServerOptions extends Options = Options> {
const isAppPath = components.isAppPath === true
const hasServerProps = !!components.getServerSideProps
let hasStaticPaths = !!components.getStaticPaths

const actionId = req.headers[ACTION.toLowerCase()] as string
const contentType = req.headers['content-type']
const isMultipartAction =
req.method === 'POST' && contentType?.startsWith('multipart/form-data')
const isFetchAction =
actionId !== undefined &&
typeof actionId === 'string' &&
req.method === 'POST'
const isServerAction = isFetchAction || isMultipartAction
const hasGetInitialProps = !!components.Component?.getInitialProps
let isSSG = !!components.getStaticProps

Expand Down Expand Up @@ -1725,6 +1734,7 @@ export default abstract class Server<ServerOptions extends Options = Options> {
// requests so ensure we respond with 405 for
// invalid requests
if (
!isServerAction &&
!is404Page &&
!is500Page &&
pathname !== '/_error' &&
Expand Down Expand Up @@ -1879,8 +1889,8 @@ export default abstract class Server<ServerOptions extends Options = Options> {
}

let ssgCacheKey =
isPreviewMode || !isSSG || opts.supportsDynamicHTML
? null // Preview mode, on-demand revalidate, flight request can bypass the cache
isPreviewMode || !isSSG || opts.supportsDynamicHTML || isServerAction
? null // Preview mode, on-demand revalidate, server actions, flight request can bypass the cache
: `${locale ? `/${locale}` : ''}${
(pathname === '/' || resolvedUrlPathname === '/') && locale
? ''
Expand Down Expand Up @@ -1996,6 +2006,7 @@ export default abstract class Server<ServerOptions extends Options = Options> {
supportsDynamicHTML,
isOnDemandRevalidate,
isDraftMode: isPreviewMode,
isServerAction,
}

// Legacy render methods will return a render result that needs to be
Expand Down
1 change: 1 addition & 0 deletions packages/next/src/server/render.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,7 @@ export type RenderOptsPartial = {
strictNextHead: boolean
isDraftMode?: boolean
deploymentId?: string
isServerAction?: boolean
}

export type RenderOpts = LoadComponentsReturnType & RenderOptsPartial
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ export function appendMutableCookies(
}

// Return a new response that extends the response with
// the modified cookies as fallbacks. `res`' cookies
// the modified cookies as fallbacks. `res` cookies
// will still take precedence.
const resCookies = new ResponseCookies(headers)
const returnedCookies = resCookies.getAll()
Expand Down
19 changes: 11 additions & 8 deletions test/e2e/app-dir/actions/app-action.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,6 @@ createNextDescribe(
},
},
({ next, isNextDev, isNextStart, isNextDeploy }) => {
if (isNextStart) {
it('should warn for server actions + ISR incompat', async () => {
expect(next.cliOutput).toContain(
'Using server actions on a page currently disables static generation for that page'
)
})
}

it('should handle basic actions correctly', async () => {
const browser = await next.browser('/server')

Expand Down Expand Up @@ -499,6 +491,17 @@ createNextDescribe(
})

describe('fetch actions', () => {
it('should handle a fetch action initiated from a static page', async () => {
const browser = await next.browser('/client-static')
await check(() => browser.elementByCss('#count').text(), '0')

await browser.elementByCss('#increment').click()
await check(() => browser.elementByCss('#count').text(), '1')

await browser.elementByCss('#increment').click()
await check(() => browser.elementByCss('#count').text(), '2')
})

it('should handle redirect to a relative URL in a single pass', async () => {
const browser = await next.browser('/client')

Expand Down
16 changes: 16 additions & 0 deletions test/e2e/app-dir/actions/app/client-static/[[...path]]/page.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { Counter } from '../../../components/Counter'
import { incrementCounter } from '../actions'

export default function Page() {
return (
<div>
<Counter onClick={incrementCounter} />
</div>
)
}

export const revalidate = 60

export async function generateStaticParams() {
return [{ path: ['asdf'] }]
}
10 changes: 10 additions & 0 deletions test/e2e/app-dir/actions/app/client-static/actions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
'use server'

let counter = 0

export async function incrementCounter() {
console.log('Button clicked!')

counter++
return counter
}
3 changes: 3 additions & 0 deletions test/e2e/app-dir/actions/app/client-static/layout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Layout({ children }) {
return children
}
20 changes: 20 additions & 0 deletions test/e2e/app-dir/actions/components/Counter.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
'use client'
import React from 'react'

export function Counter({ onClick }) {
const [count, setCount] = React.useState(0)
return (
<>
<h1 id="count">{count}</h1>
<button
id="increment"
onClick={async () => {
const newCount = await onClick()
setCount(newCount)
}}
>
+1
</button>
</>
)
}
Loading

0 comments on commit a73abad

Please sign in to comment.