From 45150fc75bb28dda08beeb3add809ddaef44f1df Mon Sep 17 00:00:00 2001 From: Janka Uryga Date: Fri, 27 Sep 2024 13:47:02 +0200 Subject: [PATCH] [after] fix: execute revalidates added within unstable_after() (#70458) ### What? Execute revalidations (written into `staticGenerationStore.{revalidatedTags,pendingRevalidates,pendingRevalidateWrites}`) that were added during `unstable_after` callbacks. ### Why? Previously, if `revalidatePath`/`revalidateTag` were called in an `unstable_after` callback, nothing would happen. I missed the fact that other codepaths (app-render, action-handler, app-route) all [manually call `staticGenerationStore.incrementalCache.revalidateTag`](https://github.com/vercel/next.js/blob/79f4490b0abda3fa7129c49b402dbadf6eadd79e/packages/next/src/server/app-render/app-render.tsx#L1072-L1080) to actually perform revalidations scheduled via `revalidatePath`/`revalidateTag`, and `after-context` wasn't doing that, so nothing was happening. --- .../next/src/server/after/after-context.ts | 14 +- .../src/server/after/revalidation-utils.ts | 77 +++++++++++ .../app/edge/dynamic-page/page.js | 1 + .../next-after-app-deploy/app/edge/layout.js | 3 + .../app/edge/middleware/page.js | 1 + .../app/edge/route/route.js | 4 + .../app/edge/server-action/page.js | 1 + .../next-after-app-deploy/app/layout.js | 10 ++ .../app/nodejs/dynamic-page/page.js | 11 ++ .../app/nodejs/layout.js | 5 + .../app/nodejs/middleware/page.js | 3 + .../app/nodejs/route/route.js | 15 +++ .../app/nodejs/server-action/page.js | 20 +++ .../next-after-app-deploy/app/path-prefix.js | 1 + .../app/timestamp/key/[key]/page.js | 36 +++++ .../app/timestamp/revalidate.js | 33 +++++ .../app/timestamp/trigger-revalidate/route.js | 14 ++ .../next-after-app-deploy/index.test.ts | 126 ++++++++++++++++++ .../next-after-app-deploy/middleware.js | 30 +++++ .../next-after-app-deploy/next.config.js | 8 ++ 20 files changed, 409 insertions(+), 4 deletions(-) create mode 100644 packages/next/src/server/after/revalidation-utils.ts create mode 100644 test/e2e/app-dir/next-after-app-deploy/app/edge/dynamic-page/page.js create mode 100644 test/e2e/app-dir/next-after-app-deploy/app/edge/layout.js create mode 100644 test/e2e/app-dir/next-after-app-deploy/app/edge/middleware/page.js create mode 100644 test/e2e/app-dir/next-after-app-deploy/app/edge/route/route.js create mode 100644 test/e2e/app-dir/next-after-app-deploy/app/edge/server-action/page.js create mode 100644 test/e2e/app-dir/next-after-app-deploy/app/layout.js create mode 100644 test/e2e/app-dir/next-after-app-deploy/app/nodejs/dynamic-page/page.js create mode 100644 test/e2e/app-dir/next-after-app-deploy/app/nodejs/layout.js create mode 100644 test/e2e/app-dir/next-after-app-deploy/app/nodejs/middleware/page.js create mode 100644 test/e2e/app-dir/next-after-app-deploy/app/nodejs/route/route.js create mode 100644 test/e2e/app-dir/next-after-app-deploy/app/nodejs/server-action/page.js create mode 100644 test/e2e/app-dir/next-after-app-deploy/app/path-prefix.js create mode 100644 test/e2e/app-dir/next-after-app-deploy/app/timestamp/key/[key]/page.js create mode 100644 test/e2e/app-dir/next-after-app-deploy/app/timestamp/revalidate.js create mode 100644 test/e2e/app-dir/next-after-app-deploy/app/timestamp/trigger-revalidate/route.js create mode 100644 test/e2e/app-dir/next-after-app-deploy/index.test.ts create mode 100644 test/e2e/app-dir/next-after-app-deploy/middleware.js create mode 100644 test/e2e/app-dir/next-after-app-deploy/next.config.js diff --git a/packages/next/src/server/after/after-context.ts b/packages/next/src/server/after/after-context.ts index ee877fb698b4b..c71541e4c9be5 100644 --- a/packages/next/src/server/after/after-context.ts +++ b/packages/next/src/server/after/after-context.ts @@ -8,6 +8,8 @@ import type { RequestLifecycleOpts } from '../base-server' import type { AfterCallback, AfterTask } from './after' import { InvariantError } from '../../shared/lib/invariant-error' import { isThenable } from '../../shared/lib/is-thenable' +import { staticGenerationAsyncStorage } from '../../client/components/static-generation-async-storage.external' +import { withExecuteRevalidates } from './revalidation-utils' export type AfterContextOpts = { waitUntil: RequestLifecycleOpts['waitUntil'] | undefined @@ -105,10 +107,14 @@ export class AfterContext { const readonlyRequestStore: RequestStore = wrapRequestStoreForAfterCallbacks(requestStore) - return requestAsyncStorage.run(readonlyRequestStore, () => { - this.callbackQueue.start() - return this.callbackQueue.onIdle() - }) + const staticGenerationStore = staticGenerationAsyncStorage.getStore() + + return withExecuteRevalidates(staticGenerationStore, () => + requestAsyncStorage.run(readonlyRequestStore, async () => { + this.callbackQueue.start() + await this.callbackQueue.onIdle() + }) + ) } } diff --git a/packages/next/src/server/after/revalidation-utils.ts b/packages/next/src/server/after/revalidation-utils.ts new file mode 100644 index 0000000000000..ee7fa8f54f0b1 --- /dev/null +++ b/packages/next/src/server/after/revalidation-utils.ts @@ -0,0 +1,77 @@ +import type { StaticGenerationStore } from '../../client/components/static-generation-async-storage.external' + +/** Run a callback, and execute any *new* revalidations added during its runtime. */ +export async function withExecuteRevalidates( + store: StaticGenerationStore | undefined, + callback: () => Promise +): Promise { + if (!store) { + return callback() + } + // If we executed any revalidates during the request, then we don't want to execute them again. + // save the state so we can check if anything changed after we're done running callbacks. + const savedRevalidationState = cloneRevalidationState(store) + try { + return await callback() + } finally { + // Check if we have any new revalidates, and if so, wait until they are all resolved. + const newRevalidates = diffRevalidationState( + savedRevalidationState, + cloneRevalidationState(store) + ) + await executeRevalidates(store, newRevalidates) + } +} + +type RevalidationState = Required< + Pick< + StaticGenerationStore, + 'revalidatedTags' | 'pendingRevalidates' | 'pendingRevalidateWrites' + > +> + +function cloneRevalidationState( + store: StaticGenerationStore +): RevalidationState { + return { + revalidatedTags: store.revalidatedTags ? [...store.revalidatedTags] : [], + pendingRevalidates: { ...store.pendingRevalidates }, + pendingRevalidateWrites: store.pendingRevalidateWrites + ? [...store.pendingRevalidateWrites] + : [], + } +} + +function diffRevalidationState( + prev: RevalidationState, + curr: RevalidationState +): RevalidationState { + const prevTags = new Set(prev.revalidatedTags) + const prevRevalidateWrites = new Set(prev.pendingRevalidateWrites) + return { + revalidatedTags: curr.revalidatedTags.filter((tag) => !prevTags.has(tag)), + pendingRevalidates: Object.fromEntries( + Object.entries(curr.pendingRevalidates).filter( + ([key]) => !(key in prev.pendingRevalidates) + ) + ), + pendingRevalidateWrites: curr.pendingRevalidateWrites.filter( + (promise) => !prevRevalidateWrites.has(promise) + ), + } +} + +async function executeRevalidates( + staticGenerationStore: StaticGenerationStore, + { + revalidatedTags, + pendingRevalidates, + pendingRevalidateWrites, + }: RevalidationState +) { + return Promise.all([ + staticGenerationStore.incrementalCache?.revalidateTag(revalidatedTags), + ...Object.values(pendingRevalidates), + ...pendingRevalidateWrites, + ]) +} diff --git a/test/e2e/app-dir/next-after-app-deploy/app/edge/dynamic-page/page.js b/test/e2e/app-dir/next-after-app-deploy/app/edge/dynamic-page/page.js new file mode 100644 index 0000000000000..9f7255965b751 --- /dev/null +++ b/test/e2e/app-dir/next-after-app-deploy/app/edge/dynamic-page/page.js @@ -0,0 +1 @@ +export { default } from '../../nodejs/dynamic-page/page' diff --git a/test/e2e/app-dir/next-after-app-deploy/app/edge/layout.js b/test/e2e/app-dir/next-after-app-deploy/app/edge/layout.js new file mode 100644 index 0000000000000..b19ff8efbdce8 --- /dev/null +++ b/test/e2e/app-dir/next-after-app-deploy/app/edge/layout.js @@ -0,0 +1,3 @@ +export { default } from '../nodejs/layout' + +export const runtime = 'edge' diff --git a/test/e2e/app-dir/next-after-app-deploy/app/edge/middleware/page.js b/test/e2e/app-dir/next-after-app-deploy/app/edge/middleware/page.js new file mode 100644 index 0000000000000..6a1b84751684f --- /dev/null +++ b/test/e2e/app-dir/next-after-app-deploy/app/edge/middleware/page.js @@ -0,0 +1 @@ +export { default } from '../../nodejs/middleware/page' diff --git a/test/e2e/app-dir/next-after-app-deploy/app/edge/route/route.js b/test/e2e/app-dir/next-after-app-deploy/app/edge/route/route.js new file mode 100644 index 0000000000000..8426f52cb33cd --- /dev/null +++ b/test/e2e/app-dir/next-after-app-deploy/app/edge/route/route.js @@ -0,0 +1,4 @@ +export { GET } from '../../nodejs/route/route' + +export const runtime = 'edge' +export const dynamic = 'force-dynamic' diff --git a/test/e2e/app-dir/next-after-app-deploy/app/edge/server-action/page.js b/test/e2e/app-dir/next-after-app-deploy/app/edge/server-action/page.js new file mode 100644 index 0000000000000..2f493b57190a5 --- /dev/null +++ b/test/e2e/app-dir/next-after-app-deploy/app/edge/server-action/page.js @@ -0,0 +1 @@ +export { default } from '../../nodejs/server-action/page' diff --git a/test/e2e/app-dir/next-after-app-deploy/app/layout.js b/test/e2e/app-dir/next-after-app-deploy/app/layout.js new file mode 100644 index 0000000000000..a55016aeb623b --- /dev/null +++ b/test/e2e/app-dir/next-after-app-deploy/app/layout.js @@ -0,0 +1,10 @@ +export default function AppLayout({ children }) { + return ( + + + after + + {children} + + ) +} diff --git a/test/e2e/app-dir/next-after-app-deploy/app/nodejs/dynamic-page/page.js b/test/e2e/app-dir/next-after-app-deploy/app/nodejs/dynamic-page/page.js new file mode 100644 index 0000000000000..7e230bc7697ed --- /dev/null +++ b/test/e2e/app-dir/next-after-app-deploy/app/nodejs/dynamic-page/page.js @@ -0,0 +1,11 @@ +import { unstable_after as after } from 'next/server' +import { revalidateTimestampPage } from '../../timestamp/revalidate' +import { pathPrefix } from '../../path-prefix' + +export default function Page() { + after(async () => { + await revalidateTimestampPage(pathPrefix + `/dynamic-page`) + }) + + return
Page with after()
+} diff --git a/test/e2e/app-dir/next-after-app-deploy/app/nodejs/layout.js b/test/e2e/app-dir/next-after-app-deploy/app/nodejs/layout.js new file mode 100644 index 0000000000000..44f23cfc048f6 --- /dev/null +++ b/test/e2e/app-dir/next-after-app-deploy/app/nodejs/layout.js @@ -0,0 +1,5 @@ +export const runtime = 'nodejs' + +export default function Layout({ children }) { + return <>{children} +} diff --git a/test/e2e/app-dir/next-after-app-deploy/app/nodejs/middleware/page.js b/test/e2e/app-dir/next-after-app-deploy/app/nodejs/middleware/page.js new file mode 100644 index 0000000000000..6321fa7d95987 --- /dev/null +++ b/test/e2e/app-dir/next-after-app-deploy/app/nodejs/middleware/page.js @@ -0,0 +1,3 @@ +export default function Page() { + return
Redirect
+} diff --git a/test/e2e/app-dir/next-after-app-deploy/app/nodejs/route/route.js b/test/e2e/app-dir/next-after-app-deploy/app/nodejs/route/route.js new file mode 100644 index 0000000000000..439bb58e455f3 --- /dev/null +++ b/test/e2e/app-dir/next-after-app-deploy/app/nodejs/route/route.js @@ -0,0 +1,15 @@ +import { unstable_after as after } from 'next/server' +import { revalidateTimestampPage } from '../../timestamp/revalidate' +import { pathPrefix } from '../../path-prefix' + +export const runtime = 'nodejs' +export const dynamic = 'force-dynamic' + +export async function GET() { + const data = { message: 'Hello, world!' } + after(async () => { + await revalidateTimestampPage(pathPrefix + `/route`) + }) + + return Response.json({ data }) +} diff --git a/test/e2e/app-dir/next-after-app-deploy/app/nodejs/server-action/page.js b/test/e2e/app-dir/next-after-app-deploy/app/nodejs/server-action/page.js new file mode 100644 index 0000000000000..d46488e9292d9 --- /dev/null +++ b/test/e2e/app-dir/next-after-app-deploy/app/nodejs/server-action/page.js @@ -0,0 +1,20 @@ +import { unstable_after as after } from 'next/server' +import { revalidateTimestampPage } from '../../timestamp/revalidate' +import { pathPrefix } from '../../path-prefix' + +export default function Page() { + return ( +
+
{ + 'use server' + after(async () => { + await revalidateTimestampPage(pathPrefix + `/server-action`) + }) + }} + > + +
+
+ ) +} diff --git a/test/e2e/app-dir/next-after-app-deploy/app/path-prefix.js b/test/e2e/app-dir/next-after-app-deploy/app/path-prefix.js new file mode 100644 index 0000000000000..dfb4e0c7f8ff1 --- /dev/null +++ b/test/e2e/app-dir/next-after-app-deploy/app/path-prefix.js @@ -0,0 +1 @@ +export const pathPrefix = '/' + process.env.NEXT_RUNTIME diff --git a/test/e2e/app-dir/next-after-app-deploy/app/timestamp/key/[key]/page.js b/test/e2e/app-dir/next-after-app-deploy/app/timestamp/key/[key]/page.js new file mode 100644 index 0000000000000..49d8a485561b5 --- /dev/null +++ b/test/e2e/app-dir/next-after-app-deploy/app/timestamp/key/[key]/page.js @@ -0,0 +1,36 @@ +import Link from 'next/link' + +export const dynamic = 'error' +export const revalidate = 3600 // arbitrarily long, just so that it doesn't happen during a test run +export const dynamicParams = true + +export async function generateStaticParams() { + return [] +} + +export default async function Page({ params }) { + const { key } = await params + const data = { + key, + timestamp: Date.now(), + } + console.log('/timestamp/key/[key] rendered', data) + + let path = null + try { + const decoded = decodeURIComponent(key) + new URL(decoded, 'http://__n') + path = decoded + } catch (err) {} + + return ( + <> + {path !== null && ( + + Go to {path} + + )} +
{JSON.stringify(data)}
+ + ) +} diff --git a/test/e2e/app-dir/next-after-app-deploy/app/timestamp/revalidate.js b/test/e2e/app-dir/next-after-app-deploy/app/timestamp/revalidate.js new file mode 100644 index 0000000000000..e83c98bf560c4 --- /dev/null +++ b/test/e2e/app-dir/next-after-app-deploy/app/timestamp/revalidate.js @@ -0,0 +1,33 @@ +import { revalidatePath } from 'next/cache' + +export async function revalidateTimestampPage(/** @type {string} */ key) { + const path = `/timestamp/key/${encodeURIComponent(key)}` + + const sleepDuration = getSleepDuration() + if (sleepDuration > 0) { + console.log(`revalidateTimestampPage :: sleeping for ${sleepDuration} ms`) + await sleep(sleepDuration) + } + + console.log('revalidateTimestampPage :: revalidating', path) + revalidatePath(path) +} + +const WAIT_BEFORE_REVALIDATING_DEFAULT = 1000 + +function getSleepDuration() { + const raw = process.env.WAIT_BEFORE_REVALIDATING + if (!raw) return WAIT_BEFORE_REVALIDATING_DEFAULT + + const parsed = Number.parseInt(raw) + if (Number.isNaN(parsed)) { + throw new Error( + `WAIT_BEFORE_REVALIDATING must be a valid number, got: ${JSON.stringify(raw)}` + ) + } + return parsed +} + +function sleep(ms) { + return new Promise((resolve) => setTimeout(resolve, ms)) +} diff --git a/test/e2e/app-dir/next-after-app-deploy/app/timestamp/trigger-revalidate/route.js b/test/e2e/app-dir/next-after-app-deploy/app/timestamp/trigger-revalidate/route.js new file mode 100644 index 0000000000000..18eb6a554db42 --- /dev/null +++ b/test/e2e/app-dir/next-after-app-deploy/app/timestamp/trigger-revalidate/route.js @@ -0,0 +1,14 @@ +import { revalidateTimestampPage } from '../revalidate' + +export async function POST(/** @type {Request} */ request) { + // we can't call revalidatePath from middleware, so we need to do it from here instead + const path = new URL(request.url).searchParams.get('path') + if (!path) { + return Response.json( + { message: 'Missing "path" search param' }, + { status: 400 } + ) + } + await revalidateTimestampPage(path) + return Response.json({}) +} diff --git a/test/e2e/app-dir/next-after-app-deploy/index.test.ts b/test/e2e/app-dir/next-after-app-deploy/index.test.ts new file mode 100644 index 0000000000000..08ea9893a01a6 --- /dev/null +++ b/test/e2e/app-dir/next-after-app-deploy/index.test.ts @@ -0,0 +1,126 @@ +/* eslint-env jest */ +import { nextTestSetup, isNextDev } from 'e2e-utils' +import { retry } from 'next-test-utils' + +const runtimes = ['nodejs', 'edge'] + +const WAIT_BEFORE_REVALIDATING = 1000 + +// If we want to verify that `unstable_after()` ran its callback, +// we need it to perform some kind of side effect (because it can't affect the response). +// In other tests, we often use logs for this, but we don't have access to those in deploy tests. +// So instead this test relies on calling `revalidatePath` inside `unstable_after` +// to revalidate an ISR page '/timestamp/key/[key]', and then checking if the timestamp changed -- +// if it did, we successfully ran the callback (and performed a side effect). + +// This test relies on revalidating a static page, so it can't work in dev mode. +const _describe = isNextDev ? describe.skip : describe + +_describe.each(runtimes)('unstable_after() in %s runtime', (runtimeValue) => { + const { next, skipped } = nextTestSetup({ + files: __dirname, + env: { WAIT_BEFORE_REVALIDATING: WAIT_BEFORE_REVALIDATING + '' }, + }) + const retryDuration = WAIT_BEFORE_REVALIDATING * 2 + + if (skipped) return + const pathPrefix = '/' + runtimeValue + + type PageInfo = { + key: string + timestamp: number + } + + const getTimestampPageData = async (path: string): Promise => { + const fullPath = `/timestamp/key/${encodeURIComponent(path)}` + const $ = await next.render$(fullPath) + const dataStr = $('#page-info').text() + if (!dataStr) { + throw new Error(`No page data found for '${fullPath}'`) + } + return JSON.parse(dataStr) as PageInfo + } + + const getInitialTimestampPageData = async (path: string) => { + // FIXME: this seems like a bug in PPR -- + // despite the page being static, the first two requests both cause a render + // and only the second one gets cached and re-used. + // we work around it by doing a dummy request to get that first "uncached" request out of the way. + if (process.env.__NEXT_EXPERIMENTAL_PPR) { + await getTimestampPageData(path) + } + + const data = await getTimestampPageData(path) + expect(data).toEqual(await getTimestampPageData(path)) // sanity check that it's static + return data + } + + it('triggers revalidate from a page', async () => { + const path = pathPrefix + '/dynamic-page' + const dataBefore = await getInitialTimestampPageData(path) + + await next.render(path) // trigger revalidate + + await retry( + async () => { + const dataAfter = await getTimestampPageData(path) + expect(dataAfter.timestamp).toBeGreaterThan(dataBefore.timestamp) + }, + retryDuration, + 1000, + 'check if timestamp page updated' + ) + }) + + it('triggers revalidate from a server action', async () => { + const path = pathPrefix + '/server-action' + const dataBefore = await getInitialTimestampPageData(path) + + const session = await next.browser(path) + await session.elementByCss('button[type="submit"]').click() // trigger revalidate + + await retry( + async () => { + const dataAfter = await getTimestampPageData(path) + expect(dataAfter.timestamp).toBeGreaterThan(dataBefore.timestamp) + }, + retryDuration, + 1000, + 'check if timestamp page updated' + ) + }) + + it('triggers revalidate from a route handler', async () => { + const path = pathPrefix + '/route' + const dataBefore = await getInitialTimestampPageData(path) + + await next.fetch(path).then((res) => res.text()) // trigger revalidate + + await retry( + async () => { + const dataAfter = await getTimestampPageData(path) + expect(dataAfter.timestamp).toBeGreaterThan(dataBefore.timestamp) + }, + retryDuration, + 1000, + 'check if timestamp page updated' + ) + }) + + it('triggers revalidate from middleware', async () => { + const path = pathPrefix + '/middleware' + const dataBefore = await getInitialTimestampPageData(path) + + await next.render(path) // trigger revalidate + + await retry( + async () => { + const dataAfter = await getTimestampPageData(path) + expect(dataAfter.timestamp).toBeGreaterThan(dataBefore.timestamp) + }, + retryDuration, + 1000, + 'check if timestamp page updated' + ) + }) +}) diff --git a/test/e2e/app-dir/next-after-app-deploy/middleware.js b/test/e2e/app-dir/next-after-app-deploy/middleware.js new file mode 100644 index 0000000000000..b3ca2ecbd3def --- /dev/null +++ b/test/e2e/app-dir/next-after-app-deploy/middleware.js @@ -0,0 +1,30 @@ +import { unstable_after as after } from 'next/server' + +export function middleware( + /** @type {import ('next/server').NextRequest} */ request +) { + const url = new URL(request.url) + + const match = url.pathname.match(/^(?\/[^/]+?)\/middleware/) + if (match) { + const pathPrefix = match.groups.prefix + after(async () => { + // we can't call revalidatePath from middleware, so we need to do it via an endpoint instead + const pathToRevalidate = pathPrefix + `/middleware` + + const postUrl = new URL('/timestamp/trigger-revalidate', url.href) + postUrl.searchParams.append('path', pathToRevalidate) + + const response = await fetch(postUrl, { method: 'POST' }) + if (!response.ok) { + throw new Error( + `Failed to revalidate path '${pathToRevalidate}' (status: ${response.status})` + ) + } + }) + } +} + +export const config = { + matcher: ['/:prefix/middleware'], +} diff --git a/test/e2e/app-dir/next-after-app-deploy/next.config.js b/test/e2e/app-dir/next-after-app-deploy/next.config.js new file mode 100644 index 0000000000000..992cdf8821d4e --- /dev/null +++ b/test/e2e/app-dir/next-after-app-deploy/next.config.js @@ -0,0 +1,8 @@ +/** @type {import('next').NextConfig} */ +module.exports = { + experimental: { + after: true, + // DO NOT turn this on, it disables the incremental cache! (see `disableForTestmode`) + // testProxy: true, + }, +}