From d00ad51afa627a0ab18b6a308edab4cfc4fefcd1 Mon Sep 17 00:00:00 2001 From: Janka Uryga Date: Thu, 28 Nov 2024 19:53:39 +0100 Subject: [PATCH] fix(after): no request APIs in force-static (#73321) 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)* --- .../next/src/server/request/connection.ts | 10 +- packages/next/src/server/request/cookies.ts | 11 +- packages/next/src/server/request/headers.ts | 10 +- .../next-after-app-api-usage/app/layout.js | 12 ++ .../app/request-apis/helpers.js | 31 ++++ .../request-apis/page-dynamic-error/page.js | 8 + .../app/request-apis/page-dynamic/page.js | 8 + .../request-apis/page-force-static/page.js | 8 + .../route-handler-dynamic-error/route.js | 8 + .../route-handler-dynamic/route.js | 6 + .../route-handler-force-static/route.js | 8 + .../app/request-apis/server-action/page.js | 14 ++ .../next-after-app-api-usage/index.test.ts | 143 ++++++++++++++++++ .../next-after-app-api-usage/next.config.js | 6 + 14 files changed, 271 insertions(+), 12 deletions(-) create mode 100644 test/e2e/app-dir/next-after-app-api-usage/app/layout.js create mode 100644 test/e2e/app-dir/next-after-app-api-usage/app/request-apis/helpers.js create mode 100644 test/e2e/app-dir/next-after-app-api-usage/app/request-apis/page-dynamic-error/page.js create mode 100644 test/e2e/app-dir/next-after-app-api-usage/app/request-apis/page-dynamic/page.js create mode 100644 test/e2e/app-dir/next-after-app-api-usage/app/request-apis/page-force-static/page.js create mode 100644 test/e2e/app-dir/next-after-app-api-usage/app/request-apis/route-handler-dynamic-error/route.js create mode 100644 test/e2e/app-dir/next-after-app-api-usage/app/request-apis/route-handler-dynamic/route.js create mode 100644 test/e2e/app-dir/next-after-app-api-usage/app/request-apis/route-handler-force-static/route.js create mode 100644 test/e2e/app-dir/next-after-app-api-usage/app/request-apis/server-action/page.js create mode 100644 test/e2e/app-dir/next-after-app-api-usage/index.test.ts create mode 100644 test/e2e/app-dir/next-after-app-api-usage/next.config.js diff --git a/packages/next/src/server/request/connection.ts b/packages/next/src/server/request/connection.ts index d4e2a61df94b6..936109a83174f 100644 --- a/packages/next/src/server/request/connection.ts +++ b/packages/next/src/server/request/connection.ts @@ -18,6 +18,12 @@ export function connection(): Promise { 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 @@ -33,10 +39,6 @@ export function connection(): Promise { 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) { diff --git a/packages/next/src/server/request/cookies.ts b/packages/next/src/server/request/cookies.ts index 5b18fcfc1b122..1b06953233bb8 100644 --- a/packages/next/src/server/request/cookies.ts +++ b/packages/next/src/server/request/cookies.ts @@ -52,6 +52,13 @@ export function cookies(): Promise { 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 @@ -68,10 +75,6 @@ export function cookies(): Promise { 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) { diff --git a/packages/next/src/server/request/headers.ts b/packages/next/src/server/request/headers.ts index 87cfa1fadb38f..a7cd4fa42917a 100644 --- a/packages/next/src/server/request/headers.ts +++ b/packages/next/src/server/request/headers.ts @@ -57,6 +57,12 @@ export function headers(): Promise { 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 @@ -73,10 +79,6 @@ export function headers(): Promise { 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) { diff --git a/test/e2e/app-dir/next-after-app-api-usage/app/layout.js b/test/e2e/app-dir/next-after-app-api-usage/app/layout.js new file mode 100644 index 0000000000000..8525f5f8c0b2a --- /dev/null +++ b/test/e2e/app-dir/next-after-app-api-usage/app/layout.js @@ -0,0 +1,12 @@ +export const metadata = { + title: 'Next.js', + description: 'Generated by Next.js', +} + +export default function RootLayout({ children }) { + return ( + + {children} + + ) +} diff --git a/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/helpers.js b/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/helpers.js new file mode 100644 index 0000000000000..e1a91a9e4e6a8 --- /dev/null +++ b/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/helpers.js @@ -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) + } + }) +} diff --git a/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/page-dynamic-error/page.js b/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/page-dynamic-error/page.js new file mode 100644 index 0000000000000..1c789bfd8382c --- /dev/null +++ b/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/page-dynamic-error/page.js @@ -0,0 +1,8 @@ +import { testRequestAPIs } from '../helpers' + +export const dynamic = 'error' + +export default async function Page() { + testRequestAPIs() + return null +} diff --git a/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/page-dynamic/page.js b/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/page-dynamic/page.js new file mode 100644 index 0000000000000..021d35af2f353 --- /dev/null +++ b/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/page-dynamic/page.js @@ -0,0 +1,8 @@ +import { connection } from 'next/server' +import { testRequestAPIs } from '../helpers' + +export default async function Page() { + await connection() + testRequestAPIs() + return null +} diff --git a/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/page-force-static/page.js b/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/page-force-static/page.js new file mode 100644 index 0000000000000..551850796273f --- /dev/null +++ b/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/page-force-static/page.js @@ -0,0 +1,8 @@ +import { testRequestAPIs } from '../helpers' + +export const dynamic = 'force-static' + +export default async function Page() { + testRequestAPIs() + return null +} diff --git a/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/route-handler-dynamic-error/route.js b/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/route-handler-dynamic-error/route.js new file mode 100644 index 0000000000000..e6e67409782b7 --- /dev/null +++ b/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/route-handler-dynamic-error/route.js @@ -0,0 +1,8 @@ +import { testRequestAPIs } from '../helpers' + +export const dynamic = 'error' + +export async function GET() { + testRequestAPIs() + return new Response() +} diff --git a/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/route-handler-dynamic/route.js b/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/route-handler-dynamic/route.js new file mode 100644 index 0000000000000..fbdf88980d3e1 --- /dev/null +++ b/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/route-handler-dynamic/route.js @@ -0,0 +1,6 @@ +import { testRequestAPIs } from '../helpers' + +export async function GET() { + testRequestAPIs() + return new Response() +} diff --git a/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/route-handler-force-static/route.js b/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/route-handler-force-static/route.js new file mode 100644 index 0000000000000..9518203975beb --- /dev/null +++ b/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/route-handler-force-static/route.js @@ -0,0 +1,8 @@ +import { testRequestAPIs } from '../helpers' + +export const dynamic = 'force-static' + +export async function GET() { + testRequestAPIs() + return new Response() +} diff --git a/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/server-action/page.js b/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/server-action/page.js new file mode 100644 index 0000000000000..fd485790ca568 --- /dev/null +++ b/test/e2e/app-dir/next-after-app-api-usage/app/request-apis/server-action/page.js @@ -0,0 +1,14 @@ +import { testRequestAPIs } from '../helpers' + +export default async function Page() { + return ( +
{ + 'use server' + testRequestAPIs() + }} + > + +
+ ) +} diff --git a/test/e2e/app-dir/next-after-app-api-usage/index.test.ts b/test/e2e/app-dir/next-after-app-api-usage/index.test.ts new file mode 100644 index 0000000000000..babe799b468cf --- /dev/null +++ b/test/e2e/app-dir/next-after-app-api-usage/index.test.ts @@ -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(...)".` + ) + }) + }) + }) + }) +}) diff --git a/test/e2e/app-dir/next-after-app-api-usage/next.config.js b/test/e2e/app-dir/next-after-app-api-usage/next.config.js new file mode 100644 index 0000000000000..ec0f3bcc9dad4 --- /dev/null +++ b/test/e2e/app-dir/next-after-app-api-usage/next.config.js @@ -0,0 +1,6 @@ +/** @type {import('next').NextConfig} */ +module.exports = { + experimental: { + after: true, + }, +}