Skip to content

Commit

Permalink
fix(after): no request APIs in force-static (#73321)
Browse files Browse the repository at this point in the history
Fixes a bug where `after(() => headers())` was allowed if `force-static` was on (and `cookies()` + `connection()` too). We shouldn't allow that, because it essentially just hides an error -- if the page is switched to dynamic, the same code would now start erroring.

*(Note that this is still without the upcoming changes to `headers()` in `after`, so the behavior in route handlers will change. i discovered this bug while working on that update, but it was confusing to try and change both at the same time, so i pulled it out into a separate PR)*
  • Loading branch information
lubieowoce authored Nov 28, 2024
1 parent 64f2702 commit d00ad51
Show file tree
Hide file tree
Showing 14 changed files with 271 additions and 12 deletions.
10 changes: 6 additions & 4 deletions packages/next/src/server/request/connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@ export function connection(): Promise<void> {
const workUnitStore = workUnitAsyncStorage.getStore()

if (workStore) {
if (workUnitStore && workUnitStore.phase === 'after') {
throw new Error(
`Route ${workStore.route} used "connection" inside "unstable_after(...)". The \`connection()\` function is used to indicate the subsequent code must only run when there is an actual Request, but "unstable_after(...)" executes after the request, so this function is not allowed in this scope. See more info here: https://nextjs.org/docs/canary/app/api-reference/functions/unstable_after`
)
}

if (workStore.forceStatic) {
// When using forceStatic we override all other logic and always just return an empty
// headers object without tracking
Expand All @@ -33,10 +39,6 @@ export function connection(): Promise<void> {
throw new Error(
`Route ${workStore.route} used "connection" inside a function cached with "unstable_cache(...)". The \`connection()\` function is used to indicate the subsequent code must only run when there is an actual Request, but caches must be able to be produced before a Request so this function is not allowed in this scope. See more info here: https://nextjs.org/docs/app/api-reference/functions/unstable_cache`
)
} else if (workUnitStore.phase === 'after') {
throw new Error(
`Route ${workStore.route} used "connection" inside "unstable_after(...)". The \`connection()\` function is used to indicate the subsequent code must only run when there is an actual Request, but "unstable_after(...)" executes after the request, so this function is not allowed in this scope. See more info here: https://nextjs.org/docs/canary/app/api-reference/functions/unstable_after`
)
}
}
if (workStore.dynamicShouldError) {
Expand Down
11 changes: 7 additions & 4 deletions packages/next/src/server/request/cookies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,13 @@ export function cookies(): Promise<ReadonlyRequestCookies> {
const workUnitStore = workUnitAsyncStorage.getStore()

if (workStore) {
if (workUnitStore && workUnitStore.phase === 'after') {
throw new Error(
// TODO(after): clarify that this only applies to pages?
`Route ${workStore.route} used "cookies" inside "unstable_after(...)". This is not supported. If you need this data inside an "unstable_after" callback, use "cookies" outside of the callback. See more info here: https://nextjs.org/docs/canary/app/api-reference/functions/unstable_after`
)
}

if (workStore.forceStatic) {
// When using forceStatic we override all other logic and always just return an empty
// cookies object without tracking
Expand All @@ -68,10 +75,6 @@ export function cookies(): Promise<ReadonlyRequestCookies> {
throw new Error(
`Route ${workStore.route} used "cookies" inside a function cached with "unstable_cache(...)". Accessing Dynamic data sources inside a cache scope is not supported. If you need this data inside a cached function use "cookies" outside of the cached function and pass the required dynamic data in as an argument. See more info here: https://nextjs.org/docs/app/api-reference/functions/unstable_cache`
)
} else if (workUnitStore.phase === 'after') {
throw new Error(
`Route ${workStore.route} used "cookies" inside "unstable_after(...)". This is not supported. If you need this data inside an "unstable_after" callback, use "cookies" outside of the callback. See more info here: https://nextjs.org/docs/canary/app/api-reference/functions/unstable_after`
)
}
}
if (workStore.dynamicShouldError) {
Expand Down
10 changes: 6 additions & 4 deletions packages/next/src/server/request/headers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,12 @@ export function headers(): Promise<ReadonlyHeaders> {
const workUnitStore = workUnitAsyncStorage.getStore()

if (workStore) {
if (workUnitStore && workUnitStore.phase === 'after') {
throw new Error(
`Route ${workStore.route} used "headers" inside "unstable_after(...)". This is not supported. If you need this data inside an "unstable_after" callback, use "headers" outside of the callback. See more info here: https://nextjs.org/docs/canary/app/api-reference/functions/unstable_after`
)
}

if (workStore.forceStatic) {
// When using forceStatic we override all other logic and always just return an empty
// headers object without tracking
Expand All @@ -73,10 +79,6 @@ export function headers(): Promise<ReadonlyHeaders> {
throw new Error(
`Route ${workStore.route} used "headers" inside a function cached with "unstable_cache(...)". Accessing Dynamic data sources inside a cache scope is not supported. If you need this data inside a cached function use "headers" outside of the cached function and pass the required dynamic data in as an argument. See more info here: https://nextjs.org/docs/app/api-reference/functions/unstable_cache`
)
} else if (workUnitStore.phase === 'after') {
throw new Error(
`Route ${workStore.route} used "headers" inside "unstable_after(...)". This is not supported. If you need this data inside an "unstable_after" callback, use "headers" outside of the callback. See more info here: https://nextjs.org/docs/canary/app/api-reference/functions/unstable_after`
)
}
}
if (workStore.dynamicShouldError) {
Expand Down
12 changes: 12 additions & 0 deletions test/e2e/app-dir/next-after-app-api-usage/app/layout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
export const metadata = {
title: 'Next.js',
description: 'Generated by Next.js',
}

export default function RootLayout({ children }) {
return (
<html lang="en">
<body>{children}</body>
</html>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { cookies, headers } from 'next/headers'
import { unstable_after as after, connection } from 'next/server'

export function testRequestAPIs() {
after(async () => {
try {
await headers()
console.log('headers(): ok')
} catch (err) {
console.error(err)
}
})

after(async () => {
try {
await cookies()
console.log('cookies(): ok')
} catch (err) {
console.error(err)
}
})

after(async () => {
try {
await connection()
console.log('connection(): ok')
} catch (err) {
console.error(err)
}
})
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { testRequestAPIs } from '../helpers'

export const dynamic = 'error'

export default async function Page() {
testRequestAPIs()
return null
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { connection } from 'next/server'
import { testRequestAPIs } from '../helpers'

export default async function Page() {
await connection()
testRequestAPIs()
return null
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { testRequestAPIs } from '../helpers'

export const dynamic = 'force-static'

export default async function Page() {
testRequestAPIs()
return null
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { testRequestAPIs } from '../helpers'

export const dynamic = 'error'

export async function GET() {
testRequestAPIs()
return new Response()
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { testRequestAPIs } from '../helpers'

export async function GET() {
testRequestAPIs()
return new Response()
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { testRequestAPIs } from '../helpers'

export const dynamic = 'force-static'

export async function GET() {
testRequestAPIs()
return new Response()
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { testRequestAPIs } from '../helpers'

export default async function Page() {
return (
<form
action={async () => {
'use server'
testRequestAPIs()
}}
>
<button type="submit">Submit</button>
</form>
)
}
143 changes: 143 additions & 0 deletions test/e2e/app-dir/next-after-app-api-usage/index.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
/* eslint-env jest */
import { nextTestSetup } from 'e2e-utils'
import { retry } from 'next-test-utils'

describe('nextjs APIs in unstable_after()', () => {
const { next, skipped, isNextDev } = nextTestSetup({
files: __dirname,
skipStart: true,
skipDeployment: true, // reading runtime logs is not supported in deploy tests
})

if (skipped) return

let currentCliOutputIndex = 0

const ignorePreviousLogs = () => {
currentCliOutputIndex = next.cliOutput.length
}

const getLogs = () => {
return next.cliOutput.slice(currentCliOutputIndex)
}

beforeEach(() => {
ignorePreviousLogs()
})

let buildLogs: string
beforeAll(async () => {
if (!isNextDev) {
await next.build()
buildLogs = next.cliOutput
} else {
buildLogs = '(no build logs in dev)'
}
await next.start()
})

describe('request APIs cannot be called inside unstable_after()', () => {
it('in a dynamic page', async () => {
const path = '/request-apis/page-dynamic'
await next.render(path)
await retry(() => {
expect(getLogs()).toContain(
`Error: Route ${path} used "headers" inside "unstable_after(...)". This is not supported.`
)
expect(getLogs()).toContain(
`Error: Route ${path} used "cookies" inside "unstable_after(...)". This is not supported.`
)
expect(getLogs()).toContain(
`Error: Route ${path} used "connection" inside "unstable_after(...)".`
)
})
})

describe('in a prerendered page', () => {
it.each([
{
title: 'with `dynamic = "error"`',
path: '/request-apis/page-dynamic-error',
},
{
title: 'with `dynamic = "force-static"`',
path: '/request-apis/page-force-static',
},
])('$title', async ({ path }) => {
await next.render(path)
await retry(() => {
const logs = isNextDev ? getLogs() : buildLogs // in `next start` the error was logged at build time
expect(logs).toContain(
`Error: Route ${path} used "headers" inside "unstable_after(...)". This is not supported.`
)
expect(logs).toContain(
`Error: Route ${path} used "cookies" inside "unstable_after(...)". This is not supported.`
)
expect(logs).toContain(
`Error: Route ${path} used "connection" inside "unstable_after(...)".`
)
})
})
})

it('in server actions', async () => {
const path = '/request-apis/server-action'
const browser = await next.browser(path)
await browser.elementByCss('button[type="submit"]').click()
await retry(() => {
expect(getLogs()).toContain(
`Error: Route ${path} used "headers" inside "unstable_after(...)". This is not supported.`
)
expect(getLogs()).toContain(
`Error: Route ${path} used "cookies" inside "unstable_after(...)". This is not supported.`
)
expect(getLogs()).toContain(
`Error: Route ${path} used "connection" inside "unstable_after(...)".`
)
})
})

it('in a dynamic route handler', async () => {
const path = '/request-apis/route-handler-dynamic'
await next.render(path)
await retry(() => {
expect(getLogs()).toContain(
`Error: Route ${path} used "headers" inside "unstable_after(...)". This is not supported.`
)
expect(getLogs()).toContain(
`Error: Route ${path} used "cookies" inside "unstable_after(...)". This is not supported.`
)
expect(getLogs()).toContain(
`Error: Route ${path} used "connection" inside "unstable_after(...)".`
)
})
})

describe('in a prerendered route handler', () => {
it.each([
{
title: 'with `dynamic = "error"`',
path: '/request-apis/route-handler-dynamic-error',
},
{
title: 'with `dynamic = "force-static"`',
path: '/request-apis/route-handler-force-static',
},
])('$title', async ({ path }) => {
await next.render(path)
await retry(() => {
const logs = isNextDev ? getLogs() : buildLogs // in `next start` the error was logged at build time
expect(logs).toContain(
`Error: Route ${path} used "headers" inside "unstable_after(...)". This is not supported.`
)
expect(logs).toContain(
`Error: Route ${path} used "cookies" inside "unstable_after(...)". This is not supported.`
)
expect(logs).toContain(
`Error: Route ${path} used "connection" inside "unstable_after(...)".`
)
})
})
})
})
})
6 changes: 6 additions & 0 deletions test/e2e/app-dir/next-after-app-api-usage/next.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/** @type {import('next').NextConfig} */
module.exports = {
experimental: {
after: true,
},
}

0 comments on commit d00ad51

Please sign in to comment.