From 330dca0ceee048d05dbed4958e3c33e1008c3ae0 Mon Sep 17 00:00:00 2001 From: Michal Piechowiak Date: Thu, 13 Feb 2025 20:59:45 +0100 Subject: [PATCH 01/15] fix: apply caching headers to pages router 404 with getStaticProps --- src/build/content/prerendered.ts | 4 +++ src/run/handlers/cache.cts | 10 ++++++ src/run/handlers/request-context.cts | 1 + src/run/headers.ts | 46 ++++++++++++++++++++------- tests/integration/page-router.test.ts | 12 +++++++ 5 files changed, 61 insertions(+), 12 deletions(-) diff --git a/src/build/content/prerendered.ts b/src/build/content/prerendered.ts index 3510aefe12..6511b16c13 100644 --- a/src/build/content/prerendered.ts +++ b/src/build/content/prerendered.ts @@ -57,6 +57,7 @@ const routeToFilePath = (path: string) => { const buildPagesCacheValue = async ( path: string, + initialRevalidateSeconds: number | false | undefined, shouldUseEnumKind: boolean, shouldSkipJson = false, ): Promise => ({ @@ -65,6 +66,7 @@ const buildPagesCacheValue = async ( pageData: shouldSkipJson ? {} : JSON.parse(await readFile(`${path}.json`, 'utf-8')), headers: undefined, status: undefined, + revalidate: initialRevalidateSeconds, }) const buildAppCacheValue = async ( @@ -178,6 +180,7 @@ export const copyPrerenderedContent = async (ctx: PluginContext): Promise } value = await buildPagesCacheValue( join(ctx.publishDir, 'server/pages', key), + meta.initialRevalidateSeconds, shouldUseEnumKind, ) break @@ -210,6 +213,7 @@ export const copyPrerenderedContent = async (ctx: PluginContext): Promise const key = routeToFilePath(route) const value = await buildPagesCacheValue( join(ctx.publishDir, 'server/pages', key), + undefined, shouldUseEnumKind, true, // there is no corresponding json file for fallback, so we are skipping it for this entry ) diff --git a/src/run/handlers/cache.cts b/src/run/handlers/cache.cts index c00a4d5e14..ba6f79794c 100644 --- a/src/run/handlers/cache.cts +++ b/src/run/handlers/cache.cts @@ -260,6 +260,11 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions { const { revalidate, ...restOfPageValue } = blob.value + const requestContext = getRequestContext() + if (requestContext) { + requestContext.pageHandlerRevalidate = revalidate + } + await this.injectEntryToPrerenderManifest(key, revalidate) return { @@ -272,6 +277,11 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions { const { revalidate, rscData, ...restOfPageValue } = blob.value + const requestContext = getRequestContext() + if (requestContext) { + requestContext.pageHandlerRevalidate = revalidate + } + await this.injectEntryToPrerenderManifest(key, revalidate) return { diff --git a/src/run/handlers/request-context.cts b/src/run/handlers/request-context.cts index cc67739242..af4083e1ca 100644 --- a/src/run/handlers/request-context.cts +++ b/src/run/handlers/request-context.cts @@ -21,6 +21,7 @@ export type RequestContext = { didPagesRouterOnDemandRevalidate?: boolean serverTiming?: string routeHandlerRevalidate?: NetlifyCachedRouteValue['revalidate'] + pageHandlerRevalidate?: NetlifyCachedRouteValue['revalidate'] /** * Track promise running in the background and need to be waited for. * Uses `context.waitUntil` if available, otherwise stores promises to diff --git a/src/run/headers.ts b/src/run/headers.ts index c93c0a605b..d206ec4e73 100644 --- a/src/run/headers.ts +++ b/src/run/headers.ts @@ -2,6 +2,7 @@ import type { Span } from '@opentelemetry/api' import type { NextConfigComplete } from 'next/dist/server/config-shared.js' import { encodeBlobKey } from '../shared/blobkey.js' +import type { NetlifyCachedRouteValue } from '../shared/cache-types.cjs' import { getLogger, RequestContext } from './handlers/request-context.cjs' import type { RuntimeTracer } from './handlers/tracer.cjs' @@ -208,6 +209,19 @@ export const adjustDateHeader = async ({ headers.set('date', lastModifiedDate.toUTCString()) } +function setCacheControlFromRequestContext( + headers: Headers, + revalidate: NetlifyCachedRouteValue['revalidate'], +) { + const cdnCacheControl = + // if we are serving already stale response, instruct edge to not attempt to cache that response + headers.get('x-nextjs-cache') === 'STALE' + ? 'public, max-age=0, must-revalidate, durable' + : `s-maxage=${revalidate === false ? 31536000 : revalidate}, stale-while-revalidate=31536000, durable` + + headers.set('netlify-cdn-cache-control', cdnCacheControl) +} + /** * Ensure stale-while-revalidate and s-maxage don't leak to the client, but * assume the user knows what they are doing if CDN cache controls are set @@ -225,13 +239,7 @@ export const setCacheControlHeaders = ( !headers.has('netlify-cdn-cache-control') ) { // handle CDN Cache Control on Route Handler responses - const cdnCacheControl = - // if we are serving already stale response, instruct edge to not attempt to cache that response - headers.get('x-nextjs-cache') === 'STALE' - ? 'public, max-age=0, must-revalidate, durable' - : `s-maxage=${requestContext.routeHandlerRevalidate === false ? 31536000 : requestContext.routeHandlerRevalidate}, stale-while-revalidate=31536000, durable` - - headers.set('netlify-cdn-cache-control', cdnCacheControl) + setCacheControlFromRequestContext(headers, requestContext.routeHandlerRevalidate) return } @@ -242,11 +250,25 @@ export const setCacheControlHeaders = ( .log('NetlifyHeadersHandler.trailingSlashRedirect') } - if (status === 404 && request.url.endsWith('.php')) { - // temporary CDN Cache Control handling for bot probes on PHP files - // https://linear.app/netlify/issue/FRB-1344/prevent-excessive-ssr-invocations-due-to-404-routes - headers.set('cache-control', 'public, max-age=0, must-revalidate') - headers.set('netlify-cdn-cache-control', `max-age=31536000, durable`) + if (status === 404) { + if (request.url.endsWith('.php')) { + // temporary CDN Cache Control handling for bot probes on PHP files + // https://linear.app/netlify/issue/FRB-1344/prevent-excessive-ssr-invocations-due-to-404-routes + headers.set('cache-control', 'public, max-age=0, must-revalidate') + headers.set('netlify-cdn-cache-control', `max-age=31536000, durable`) + return + } + + if ( + typeof requestContext.pageHandlerRevalidate !== 'undefined' && + ['GET', 'HEAD'].includes(request.method) && + !headers.has('cdn-cache-control') && + !headers.has('netlify-cdn-cache-control') + ) { + // handle CDN Cache Control on 404 Page responses + setCacheControlFromRequestContext(headers, requestContext.pageHandlerRevalidate) + return + } } const cacheControl = headers.get('cache-control') diff --git a/tests/integration/page-router.test.ts b/tests/integration/page-router.test.ts index eb039eb79d..79eb960728 100644 --- a/tests/integration/page-router.test.ts +++ b/tests/integration/page-router.test.ts @@ -126,6 +126,10 @@ test('Should serve correct locale-aware custom 404 pages', a load(responseImplicitDefaultLocale.body)('[data-testid="locale"]').text(), 'Served 404 page content should use default locale if locale is not explicitly used in pathname (after basePath)', ).toBe('en') + expect( + responseImplicitDefaultLocale.headers['netlify-cdn-cache-control'], + 'Response for not existing route if locale is not explicitly used in pathname (after basePath) should have 404 status', + ).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable') const responseExplicitDefaultLocale = await invokeFunction(ctx, { url: '/base/path/en/not-existing-page', @@ -139,6 +143,10 @@ test('Should serve correct locale-aware custom 404 pages', a load(responseExplicitDefaultLocale.body)('[data-testid="locale"]').text(), 'Served 404 page content should use default locale if default locale is explicitly used in pathname (after basePath)', ).toBe('en') + expect( + responseExplicitDefaultLocale.headers['netlify-cdn-cache-control'], + 'Response for not existing route if locale is not explicitly used in pathname (after basePath) should have 404 status', + ).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable') const responseNonDefaultLocale = await invokeFunction(ctx, { url: '/base/path/fr/not-existing-page', @@ -152,4 +160,8 @@ test('Should serve correct locale-aware custom 404 pages', a load(responseNonDefaultLocale.body)('[data-testid="locale"]').text(), 'Served 404 page content should use non-default locale if non-default locale is explicitly used in pathname (after basePath)', ).toBe('fr') + expect( + responseNonDefaultLocale.headers['netlify-cdn-cache-control'], + 'Response for not existing route if locale is not explicitly used in pathname (after basePath) should have 404 status', + ).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable') }) From bdfb31a904cc36f692973a3eb400987e7a9ca715 Mon Sep 17 00:00:00 2001 From: Michal Piechowiak Date: Mon, 17 Feb 2025 11:57:18 +0100 Subject: [PATCH 02/15] chore: don't store revalidate on request context for app router pages to limit potentially unwanted impact of the change --- src/run/handlers/cache.cts | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/run/handlers/cache.cts b/src/run/handlers/cache.cts index ba6f79794c..8c2a06956d 100644 --- a/src/run/handlers/cache.cts +++ b/src/run/handlers/cache.cts @@ -277,11 +277,6 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions { const { revalidate, rscData, ...restOfPageValue } = blob.value - const requestContext = getRequestContext() - if (requestContext) { - requestContext.pageHandlerRevalidate = revalidate - } - await this.injectEntryToPrerenderManifest(key, revalidate) return { From c1712f87f8c4fb587e720df5378f54ff2a5b88ea Mon Sep 17 00:00:00 2001 From: Michal Piechowiak Date: Mon, 17 Feb 2025 12:16:15 +0100 Subject: [PATCH 03/15] chore: don't apply new 404 caching header code path, if existing method would have set cacheable header --- src/run/headers.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/run/headers.ts b/src/run/headers.ts index d206ec4e73..4a1f4682ab 100644 --- a/src/run/headers.ts +++ b/src/run/headers.ts @@ -250,6 +250,7 @@ export const setCacheControlHeaders = ( .log('NetlifyHeadersHandler.trailingSlashRedirect') } + const cacheControl = headers.get('cache-control') if (status === 404) { if (request.url.endsWith('.php')) { // temporary CDN Cache Control handling for bot probes on PHP files @@ -260,6 +261,7 @@ export const setCacheControlHeaders = ( } if ( + (!cacheControl || cacheControl.includes('no-store')) && typeof requestContext.pageHandlerRevalidate !== 'undefined' && ['GET', 'HEAD'].includes(request.method) && !headers.has('cdn-cache-control') && @@ -271,7 +273,6 @@ export const setCacheControlHeaders = ( } } - const cacheControl = headers.get('cache-control') if ( cacheControl !== null && ['GET', 'HEAD'].includes(request.method) && From 7b947dc9e8d3ffb9154a0047a3bd1dc80589733f Mon Sep 17 00:00:00 2001 From: Michal Piechowiak Date: Mon, 17 Feb 2025 12:17:17 +0100 Subject: [PATCH 04/15] test: add notFound cases for 404 with getStaticProps without revalidate --- .../pages/products/[slug].js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/fixtures/page-router-base-path-i18n/pages/products/[slug].js b/tests/fixtures/page-router-base-path-i18n/pages/products/[slug].js index f41d142c67..306314ab1f 100644 --- a/tests/fixtures/page-router-base-path-i18n/pages/products/[slug].js +++ b/tests/fixtures/page-router-base-path-i18n/pages/products/[slug].js @@ -8,7 +8,19 @@ const Product = ({ time, slug }) => ( ) +/** @type {import('next').GetStaticProps} */ export async function getStaticProps({ params }) { + if (params.slug === 'not-found-no-revalidate') { + return { + notFound: true, + } + } else if (params.slug === 'not-found-with-revalidate') { + return { + notFound: true, + revalidate: 600, + } + } + return { props: { time: new Date().toISOString(), From 3423330ce1672619f1cba3e395575e52054a9521 Mon Sep 17 00:00:00 2001 From: Michal Piechowiak Date: Mon, 17 Feb 2025 12:17:42 +0100 Subject: [PATCH 05/15] test: add fixture for 404 with getStaticProps with revalidate --- .../next.config.js | 10 +++++ .../package.json | 15 +++++++ .../pages/404.js | 17 ++++++++ .../pages/products/[slug].js | 40 +++++++++++++++++++ 4 files changed, 82 insertions(+) create mode 100644 tests/fixtures/page-router-404-get-static-props-with-revalidate/next.config.js create mode 100644 tests/fixtures/page-router-404-get-static-props-with-revalidate/package.json create mode 100644 tests/fixtures/page-router-404-get-static-props-with-revalidate/pages/404.js create mode 100644 tests/fixtures/page-router-404-get-static-props-with-revalidate/pages/products/[slug].js diff --git a/tests/fixtures/page-router-404-get-static-props-with-revalidate/next.config.js b/tests/fixtures/page-router-404-get-static-props-with-revalidate/next.config.js new file mode 100644 index 0000000000..6346ab0742 --- /dev/null +++ b/tests/fixtures/page-router-404-get-static-props-with-revalidate/next.config.js @@ -0,0 +1,10 @@ +/** @type {import('next').NextConfig} */ +const nextConfig = { + output: 'standalone', + eslint: { + ignoreDuringBuilds: true, + }, + generateBuildId: () => 'build-id', +} + +module.exports = nextConfig diff --git a/tests/fixtures/page-router-404-get-static-props-with-revalidate/package.json b/tests/fixtures/page-router-404-get-static-props-with-revalidate/package.json new file mode 100644 index 0000000000..4506b1ddb2 --- /dev/null +++ b/tests/fixtures/page-router-404-get-static-props-with-revalidate/package.json @@ -0,0 +1,15 @@ +{ + "name": "page-router-404-get-static-props-with-revalidate", + "version": "0.1.0", + "private": true, + "scripts": { + "postinstall": "next build", + "dev": "next dev", + "build": "next build" + }, + "dependencies": { + "next": "latest", + "react": "18.2.0", + "react-dom": "18.2.0" + } +} diff --git a/tests/fixtures/page-router-404-get-static-props-with-revalidate/pages/404.js b/tests/fixtures/page-router-404-get-static-props-with-revalidate/pages/404.js new file mode 100644 index 0000000000..cb047699af --- /dev/null +++ b/tests/fixtures/page-router-404-get-static-props-with-revalidate/pages/404.js @@ -0,0 +1,17 @@ +export default function NotFound({ timestamp }) { + return ( +

+ Custom 404 page with revalidate: 

{timestamp}
+

+ ) +} + +/** @type {import('next').GetStaticProps} */ +export const getStaticProps = ({ locale }) => { + return { + props: { + timestamp: Date.now(), + }, + revalidate: 300, + } +} diff --git a/tests/fixtures/page-router-404-get-static-props-with-revalidate/pages/products/[slug].js b/tests/fixtures/page-router-404-get-static-props-with-revalidate/pages/products/[slug].js new file mode 100644 index 0000000000..714c8ac143 --- /dev/null +++ b/tests/fixtures/page-router-404-get-static-props-with-revalidate/pages/products/[slug].js @@ -0,0 +1,40 @@ +const Product = ({ time, slug }) => ( +
+

Product {slug}

+

+ This page uses getStaticProps() and getStaticPaths() to pre-fetch a Product + {time} +

+
+) + +/** @type {import('next').GetStaticProps} */ +export async function getStaticProps({ params }) { + if (params.slug === 'not-found-no-revalidate') { + return { + notFound: true, + } + } else if (params.slug === 'not-found-with-revalidate') { + return { + notFound: true, + revalidate: 600, + } + } + + return { + props: { + time: new Date().toISOString(), + slug: params.slug, + }, + } +} + +/** @type {import('next').GetStaticPaths} */ +export const getStaticPaths = () => { + return { + paths: [], + fallback: 'blocking', // false or "blocking" + } +} + +export default Product From 25ad18c12218d2134d256a4d1a3d165ece2d8b27 Mon Sep 17 00:00:00 2001 From: Michal Piechowiak Date: Mon, 17 Feb 2025 12:19:49 +0100 Subject: [PATCH 06/15] tmp: outline 404 integration test cases for page router --- tests/integration/page-router.test.ts | 147 +++++++++++++++++++++++--- 1 file changed, 134 insertions(+), 13 deletions(-) diff --git a/tests/integration/page-router.test.ts b/tests/integration/page-router.test.ts index 79eb960728..3c6b624862 100644 --- a/tests/integration/page-router.test.ts +++ b/tests/integration/page-router.test.ts @@ -4,7 +4,7 @@ import { HttpResponse, http, passthrough } from 'msw' import { setupServer } from 'msw/node' import { platform } from 'node:process' import { v4 } from 'uuid' -import { afterAll, afterEach, beforeAll, beforeEach, expect, test, vi } from 'vitest' +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, test, vi } from 'vitest' import { type FixtureTestContext } from '../utils/contexts.js' import { createFixture, invokeFunction, runPlugin } from '../utils/fixture.js' import { encodeBlobKey, generateRandomObjectID, startMockBlobStore } from '../utils/helpers.js' @@ -126,10 +126,6 @@ test('Should serve correct locale-aware custom 404 pages', a load(responseImplicitDefaultLocale.body)('[data-testid="locale"]').text(), 'Served 404 page content should use default locale if locale is not explicitly used in pathname (after basePath)', ).toBe('en') - expect( - responseImplicitDefaultLocale.headers['netlify-cdn-cache-control'], - 'Response for not existing route if locale is not explicitly used in pathname (after basePath) should have 404 status', - ).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable') const responseExplicitDefaultLocale = await invokeFunction(ctx, { url: '/base/path/en/not-existing-page', @@ -143,10 +139,6 @@ test('Should serve correct locale-aware custom 404 pages', a load(responseExplicitDefaultLocale.body)('[data-testid="locale"]').text(), 'Served 404 page content should use default locale if default locale is explicitly used in pathname (after basePath)', ).toBe('en') - expect( - responseExplicitDefaultLocale.headers['netlify-cdn-cache-control'], - 'Response for not existing route if locale is not explicitly used in pathname (after basePath) should have 404 status', - ).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable') const responseNonDefaultLocale = await invokeFunction(ctx, { url: '/base/path/fr/not-existing-page', @@ -160,8 +152,137 @@ test('Should serve correct locale-aware custom 404 pages', a load(responseNonDefaultLocale.body)('[data-testid="locale"]').text(), 'Served 404 page content should use non-default locale if non-default locale is explicitly used in pathname (after basePath)', ).toBe('fr') - expect( - responseNonDefaultLocale.headers['netlify-cdn-cache-control'], - 'Response for not existing route if locale is not explicitly used in pathname (after basePath) should have 404 status', - ).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable') +}) + +describe('404 caching', () => { + describe('default nextjs 404 (no custom 404)', () => { + test.todo('not matching dynamic paths') + test.todo('matching dynamic path without revalidate') + test.todo('matching dynamic path with revalidate') + }) + + describe('404 without getStaticProps', () => { + test.todo('not matching dynamic paths') + test.todo('matching dynamic path without revalidate') + test.todo('matching dynamic path with revalidate') + }) + + describe('404 with getStaticProps without revalidate', () => { + test('not matching dynamic paths should be cached permanently', async (ctx) => { + console.log('[test] not matching dynamic paths') + + await createFixture('page-router-base-path-i18n', ctx) + await runPlugin(ctx) + + const notExistingPage = await invokeFunction(ctx, { + url: '/base/path/not-existing-page', + }) + + expect( + notExistingPage.headers['netlify-cdn-cache-control'], + 'should be cached permanently', + ).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable') + }) + test('matching dynamic path without revalidate should be cached permanently', async (ctx) => { + console.log('[test] matching dynamic path without revalidate') + + await createFixture('page-router-base-path-i18n', ctx) + await runPlugin(ctx) + + const notExistingPage = await invokeFunction(ctx, { + url: '/base/path/products/not-found-no-revalidate', + }) + + expect( + notExistingPage.headers['netlify-cdn-cache-control'], + 'should be cached permanently', + ).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable') + }) + test('matching dynamic path with revalidate should be cached for revalidate time', async (ctx) => { + console.log('[test] matching dynamic path with revalidate') + + await createFixture('page-router-base-path-i18n', ctx) + await runPlugin(ctx) + + const notExistingPage = await invokeFunction(ctx, { + url: '/base/path/products/not-found-with-revalidate', + }) + + expect(notExistingPage.statusCode).toBe(404) + // this page was not prerendered, so no STALE case here + expect( + notExistingPage.headers['netlify-cdn-cache-control'], + 'should be cached for 600 seconds', + ).toBe('s-maxage=600, stale-while-revalidate=31536000, durable') + }) + }) + + describe('404 with getStaticProps with revalidate', () => { + test('not matching dynamic paths should be cached for 404 page revalidate', async (ctx) => { + console.log('[test] not matching dynamic paths') + + await createFixture('page-router-404-get-static-props-with-revalidate', ctx) + await runPlugin(ctx) + + // ignoring initial stale case + await invokeFunction(ctx, { + url: 'not-existing-page', + }) + await new Promise((res) => setTimeout(res, 100)) + + const notExistingPage = await invokeFunction(ctx, { + url: 'not-existing-page', + }) + + expect( + notExistingPage.headers['netlify-cdn-cache-control'], + 'should be cached permanently', + ).toBe('s-maxage=300, stale-while-revalidate=31536000, durable') + }) + test.only('matching dynamic path without revalidate should be cached permanently', async (ctx) => { + console.log('[test] matching dynamic path without revalidate') + + await createFixture('page-router-404-get-static-props-with-revalidate', ctx) + await runPlugin(ctx) + + // // ignoring initial stale case + // await invokeFunction(ctx, { + // url: 'products/not-found-no-revalidate', + // }) + // await new Promise((res) => setTimeout(res, 100)) + + const notExistingPage = await invokeFunction(ctx, { + url: 'products/not-found-no-revalidate', + }) + + expect( + notExistingPage.headers['netlify-cdn-cache-control'], + 'should be cached permanently', + ).toBe('s-maxage=31536000, durable') + }) + + test.only('matching dynamic path with revalidate should be cached for revalidate time', async (ctx) => { + console.log('[test] matching dynamic path with revalidate') + + await createFixture('page-router-404-get-static-props-with-revalidate', ctx) + await runPlugin(ctx) + + // // ignoring initial stale case + // await invokeFunction(ctx, { + // url: 'products/not-found-with-revalidate', + // }) + // await new Promise((res) => setTimeout(res, 100)) + + const notExistingPage = await invokeFunction(ctx, { + url: 'products/not-found-with-revalidate', + }) + + expect(notExistingPage.statusCode).toBe(404) + // this page was not prerendered, so no STALE case here + expect( + notExistingPage.headers['netlify-cdn-cache-control'], + 'should be cached for 600 seconds', + ).toBe('s-maxage=600, stale-while-revalidate=31535400, durable') + }) + }) }) From bbedbdb0ac4dc290bc919d2fc55ca44e5dc8204f Mon Sep 17 00:00:00 2001 From: Rob Stanford Date: Mon, 24 Feb 2025 10:40:36 +0000 Subject: [PATCH 07/15] test: update 404 caching tests --- tests/fixtures/page-router/pages/404.js | 3 + .../page-router/pages/products/[slug].js | 7 ++ tests/integration/page-router.test.ts | 100 ++++++++---------- 3 files changed, 54 insertions(+), 56 deletions(-) create mode 100644 tests/fixtures/page-router/pages/404.js diff --git a/tests/fixtures/page-router/pages/404.js b/tests/fixtures/page-router/pages/404.js new file mode 100644 index 0000000000..3c251e6665 --- /dev/null +++ b/tests/fixtures/page-router/pages/404.js @@ -0,0 +1,3 @@ +export default function NotFound() { + return

Custom 404 page

+} diff --git a/tests/fixtures/page-router/pages/products/[slug].js b/tests/fixtures/page-router/pages/products/[slug].js index a55c3d0991..319b1f9213 100644 --- a/tests/fixtures/page-router/pages/products/[slug].js +++ b/tests/fixtures/page-router/pages/products/[slug].js @@ -9,6 +9,13 @@ const Product = ({ time, slug }) => ( ) export async function getStaticProps({ params }) { + if (params.slug === 'not-found-with-revalidate') { + return { + notFound: true, + revalidate: 600, + } + } + return { props: { time: new Date().toISOString(), diff --git a/tests/integration/page-router.test.ts b/tests/integration/page-router.test.ts index 3c6b624862..8066f1e639 100644 --- a/tests/integration/page-router.test.ts +++ b/tests/integration/page-router.test.ts @@ -154,51 +154,59 @@ test('Should serve correct locale-aware custom 404 pages', a ).toBe('fr') }) -describe('404 caching', () => { - describe('default nextjs 404 (no custom 404)', () => { - test.todo('not matching dynamic paths') - test.todo('matching dynamic path without revalidate') - test.todo('matching dynamic path with revalidate') - }) - +describe.only('404 caching', () => { describe('404 without getStaticProps', () => { - test.todo('not matching dynamic paths') - test.todo('matching dynamic path without revalidate') - test.todo('matching dynamic path with revalidate') - }) - - describe('404 with getStaticProps without revalidate', () => { test('not matching dynamic paths should be cached permanently', async (ctx) => { - console.log('[test] not matching dynamic paths') + await createFixture('page-router', ctx) + await runPlugin(ctx) - await createFixture('page-router-base-path-i18n', ctx) + const notExistingPage = await invokeFunction(ctx, { + url: '/not-existing-page', + }) + + expect(notExistingPage.statusCode).toBe(404) + + expect( + notExistingPage.headers['netlify-cdn-cache-control'], + 'should be cached permanently', + ).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable') + }) + test('matching dynamic path with revalidate should be cached permanently', async (ctx) => { + await createFixture('page-router', ctx) await runPlugin(ctx) const notExistingPage = await invokeFunction(ctx, { - url: '/base/path/not-existing-page', + url: '/products/not-found-with-revalidate', }) + expect(notExistingPage.statusCode).toBe(404) + expect( notExistingPage.headers['netlify-cdn-cache-control'], 'should be cached permanently', ).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable') }) - test('matching dynamic path without revalidate should be cached permanently', async (ctx) => { - console.log('[test] matching dynamic path without revalidate') + }) + + describe('404 with getStaticProps without revalidate', () => { + test('not matching dynamic paths should be cached permanently', async (ctx) => { + console.log('[test] not matching dynamic paths') await createFixture('page-router-base-path-i18n', ctx) await runPlugin(ctx) const notExistingPage = await invokeFunction(ctx, { - url: '/base/path/products/not-found-no-revalidate', + url: '/base/path/not-existing-page', }) + expect(notExistingPage.statusCode).toBe(404) + expect( notExistingPage.headers['netlify-cdn-cache-control'], 'should be cached permanently', ).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable') }) - test('matching dynamic path with revalidate should be cached for revalidate time', async (ctx) => { + test('matching dynamic path with revalidate should be cached permanently', async (ctx) => { console.log('[test] matching dynamic path with revalidate') await createFixture('page-router-base-path-i18n', ctx) @@ -209,18 +217,16 @@ describe('404 caching', () => { }) expect(notExistingPage.statusCode).toBe(404) - // this page was not prerendered, so no STALE case here + expect( notExistingPage.headers['netlify-cdn-cache-control'], - 'should be cached for 600 seconds', - ).toBe('s-maxage=600, stale-while-revalidate=31536000, durable') + 'should be cached permanently', + ).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable') }) }) describe('404 with getStaticProps with revalidate', () => { test('not matching dynamic paths should be cached for 404 page revalidate', async (ctx) => { - console.log('[test] not matching dynamic paths') - await createFixture('page-router-404-get-static-props-with-revalidate', ctx) await runPlugin(ctx) @@ -228,61 +234,43 @@ describe('404 caching', () => { await invokeFunction(ctx, { url: 'not-existing-page', }) + await new Promise((res) => setTimeout(res, 100)) const notExistingPage = await invokeFunction(ctx, { url: 'not-existing-page', }) - expect( - notExistingPage.headers['netlify-cdn-cache-control'], - 'should be cached permanently', - ).toBe('s-maxage=300, stale-while-revalidate=31536000, durable') - }) - test.only('matching dynamic path without revalidate should be cached permanently', async (ctx) => { - console.log('[test] matching dynamic path without revalidate') - - await createFixture('page-router-404-get-static-props-with-revalidate', ctx) - await runPlugin(ctx) - - // // ignoring initial stale case - // await invokeFunction(ctx, { - // url: 'products/not-found-no-revalidate', - // }) - // await new Promise((res) => setTimeout(res, 100)) - - const notExistingPage = await invokeFunction(ctx, { - url: 'products/not-found-no-revalidate', - }) + expect(notExistingPage.statusCode).toBe(404) expect( notExistingPage.headers['netlify-cdn-cache-control'], - 'should be cached permanently', - ).toBe('s-maxage=31536000, durable') + 'should be cached for 404 page revalidate', + ).toBe('s-maxage=300, stale-while-revalidate=31536000, durable') }) - test.only('matching dynamic path with revalidate should be cached for revalidate time', async (ctx) => { + test('matching dynamic path with revalidate should be cached for 404 page revalidate', async (ctx) => { console.log('[test] matching dynamic path with revalidate') await createFixture('page-router-404-get-static-props-with-revalidate', ctx) await runPlugin(ctx) - // // ignoring initial stale case - // await invokeFunction(ctx, { - // url: 'products/not-found-with-revalidate', - // }) - // await new Promise((res) => setTimeout(res, 100)) + // ignoring initial stale case + await invokeFunction(ctx, { + url: 'products/not-found-with-revalidate', + }) + await new Promise((res) => setTimeout(res, 100)) const notExistingPage = await invokeFunction(ctx, { url: 'products/not-found-with-revalidate', }) expect(notExistingPage.statusCode).toBe(404) - // this page was not prerendered, so no STALE case here + expect( notExistingPage.headers['netlify-cdn-cache-control'], - 'should be cached for 600 seconds', - ).toBe('s-maxage=600, stale-while-revalidate=31535400, durable') + 'should be cached for 404 page revalidate', + ).toBe('s-maxage=300, stale-while-revalidate=31536000, durable') }) }) }) From 3ea93759d51d6a793382c473e451de024739c3a8 Mon Sep 17 00:00:00 2001 From: Rob Stanford Date: Mon, 24 Feb 2025 10:46:35 +0000 Subject: [PATCH 08/15] fix: relax the conditions for caching 404s --- src/run/headers.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/run/headers.ts b/src/run/headers.ts index 4a1f4682ab..2dd998258f 100644 --- a/src/run/headers.ts +++ b/src/run/headers.ts @@ -217,7 +217,7 @@ function setCacheControlFromRequestContext( // if we are serving already stale response, instruct edge to not attempt to cache that response headers.get('x-nextjs-cache') === 'STALE' ? 'public, max-age=0, must-revalidate, durable' - : `s-maxage=${revalidate === false ? 31536000 : revalidate}, stale-while-revalidate=31536000, durable` + : `s-maxage=${revalidate || 31536000}, stale-while-revalidate=31536000, durable` headers.set('netlify-cdn-cache-control', cdnCacheControl) } @@ -261,8 +261,6 @@ export const setCacheControlHeaders = ( } if ( - (!cacheControl || cacheControl.includes('no-store')) && - typeof requestContext.pageHandlerRevalidate !== 'undefined' && ['GET', 'HEAD'].includes(request.method) && !headers.has('cdn-cache-control') && !headers.has('netlify-cdn-cache-control') From 5d91e73462f06e374da6927bd10efb72b41e7abb Mon Sep 17 00:00:00 2001 From: Rob Stanford Date: Mon, 24 Feb 2025 10:54:53 +0000 Subject: [PATCH 09/15] test: remove only --- tests/integration/page-router.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/page-router.test.ts b/tests/integration/page-router.test.ts index 8066f1e639..bd5e35b35a 100644 --- a/tests/integration/page-router.test.ts +++ b/tests/integration/page-router.test.ts @@ -154,7 +154,7 @@ test('Should serve correct locale-aware custom 404 pages', a ).toBe('fr') }) -describe.only('404 caching', () => { +describe('404 caching', () => { describe('404 without getStaticProps', () => { test('not matching dynamic paths should be cached permanently', async (ctx) => { await createFixture('page-router', ctx) From 2962d0b24f704eb5a54afb8f27bd0a89a2ef84a5 Mon Sep 17 00:00:00 2001 From: Rob Stanford Date: Tue, 25 Feb 2025 13:22:41 +0000 Subject: [PATCH 10/15] feat: hide 404 caching behind env var --- src/run/headers.ts | 1 + tests/integration/page-router.test.ts | 10 +++++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/run/headers.ts b/src/run/headers.ts index aff964aeab..3faf1f6698 100644 --- a/src/run/headers.ts +++ b/src/run/headers.ts @@ -261,6 +261,7 @@ export const setCacheControlHeaders = ( } if ( + process.env.ENABLE_404_CACHING && ['GET', 'HEAD'].includes(request.method) && !headers.has('cdn-cache-control') && !headers.has('netlify-cdn-cache-control') diff --git a/tests/integration/page-router.test.ts b/tests/integration/page-router.test.ts index bd5e35b35a..ee0ac1291c 100644 --- a/tests/integration/page-router.test.ts +++ b/tests/integration/page-router.test.ts @@ -154,7 +154,15 @@ test('Should serve correct locale-aware custom 404 pages', a ).toBe('fr') }) -describe('404 caching', () => { +describe.only('404 caching', () => { + beforeAll(() => { + process.env.ENABLE_404_CACHING = 'true' + }) + + afterAll(() => { + delete process.env.ENABLE_404_CACHING + }) + describe('404 without getStaticProps', () => { test('not matching dynamic paths should be cached permanently', async (ctx) => { await createFixture('page-router', ctx) From 238e6e6e0577c3b6086e7de6b8a6aa40849d4b33 Mon Sep 17 00:00:00 2001 From: Rob Stanford Date: Tue, 25 Feb 2025 13:28:18 +0000 Subject: [PATCH 11/15] test: remove only and console logs --- tests/integration/page-router.test.ts | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/tests/integration/page-router.test.ts b/tests/integration/page-router.test.ts index ee0ac1291c..9661f12bce 100644 --- a/tests/integration/page-router.test.ts +++ b/tests/integration/page-router.test.ts @@ -19,7 +19,6 @@ beforeAll(() => { // and passthrough everything else server = setupServer( http.post('https://api.netlify.com/api/v1/purge', () => { - console.log('intercepted purge api call') return HttpResponse.json({}) }), http.all(/.*/, () => passthrough()), @@ -91,7 +90,6 @@ test('Should revalidate path with On-demand Revalidation', a expect(staticPageRevalidated.headers?.['cache-status']).toMatch(/"Next.js"; hit/) const dateCacheRevalidated = load(staticPageRevalidated.body)('[data-testid="date-now"]').text() - console.log({ dateCacheInitial, dateCacheRevalidated }) expect(dateCacheInitial).not.toBe(dateCacheRevalidated) }) @@ -154,7 +152,7 @@ test('Should serve correct locale-aware custom 404 pages', a ).toBe('fr') }) -describe.only('404 caching', () => { +describe('404 caching', () => { beforeAll(() => { process.env.ENABLE_404_CACHING = 'true' }) @@ -198,8 +196,6 @@ describe.only('404 caching', () => { describe('404 with getStaticProps without revalidate', () => { test('not matching dynamic paths should be cached permanently', async (ctx) => { - console.log('[test] not matching dynamic paths') - await createFixture('page-router-base-path-i18n', ctx) await runPlugin(ctx) @@ -215,8 +211,6 @@ describe.only('404 caching', () => { ).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable') }) test('matching dynamic path with revalidate should be cached permanently', async (ctx) => { - console.log('[test] matching dynamic path with revalidate') - await createFixture('page-router-base-path-i18n', ctx) await runPlugin(ctx) @@ -258,8 +252,6 @@ describe.only('404 caching', () => { }) test('matching dynamic path with revalidate should be cached for 404 page revalidate', async (ctx) => { - console.log('[test] matching dynamic path with revalidate') - await createFixture('page-router-404-get-static-props-with-revalidate', ctx) await runPlugin(ctx) From bb64d825be2763993ab67967715fd3ce07532c81 Mon Sep 17 00:00:00 2001 From: Rob Stanford Date: Tue, 25 Feb 2025 14:58:47 +0000 Subject: [PATCH 12/15] test: update static 404 test --- tests/integration/static.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/static.test.ts b/tests/integration/static.test.ts index c0fb42fd74..b332577ca4 100644 --- a/tests/integration/static.test.ts +++ b/tests/integration/static.test.ts @@ -54,7 +54,7 @@ test('requesting a non existing page route that needs to be // test that it should request the 404.html file const call1 = await invokeFunction(ctx, { url: 'static/revalidate-not-existing' }) expect(call1.statusCode).toBe(404) - expect(load(call1.body)('h1').text()).toBe('404') + expect(load(call1.body)('p').text()).toBe('Custom 404 page') // https://github.com/vercel/next.js/pull/69802 made changes to returned cache-control header, // after that (14.2.10 and canary.147) 404 pages would have `private` directive, before that it From f31d30c11cd5ae1b9e83e763e06acb9ed0c6981d Mon Sep 17 00:00:00 2001 From: Rob Stanford Date: Thu, 27 Feb 2025 18:21:51 +0000 Subject: [PATCH 13/15] feat: reduce 404 caching scope to just the 404 page itself --- src/run/headers.ts | 7 +- tests/integration/page-router.test.ts | 99 ++++++++++++++++++++++----- 2 files changed, 84 insertions(+), 22 deletions(-) diff --git a/src/run/headers.ts b/src/run/headers.ts index 3faf1f6698..cb4c1f4640 100644 --- a/src/run/headers.ts +++ b/src/run/headers.ts @@ -261,10 +261,9 @@ export const setCacheControlHeaders = ( } if ( - process.env.ENABLE_404_CACHING && - ['GET', 'HEAD'].includes(request.method) && - !headers.has('cdn-cache-control') && - !headers.has('netlify-cdn-cache-control') + process.env.CACHE_404_PAGE && + request.url.endsWith('/404') && + ['GET', 'HEAD'].includes(request.method) ) { // handle CDN Cache Control on 404 Page responses setCacheControlFromRequestContext(headers, requestContext.pageHandlerRevalidate) diff --git a/tests/integration/page-router.test.ts b/tests/integration/page-router.test.ts index 9661f12bce..8fda544d67 100644 --- a/tests/integration/page-router.test.ts +++ b/tests/integration/page-router.test.ts @@ -152,15 +152,10 @@ test('Should serve correct locale-aware custom 404 pages', a ).toBe('fr') }) -describe('404 caching', () => { - beforeAll(() => { - process.env.ENABLE_404_CACHING = 'true' - }) - - afterAll(() => { - delete process.env.ENABLE_404_CACHING - }) - +// These tests describe how the 404 caching should work, but unfortunately it doesn't work like +// this in v5 and a fix would represent a breaking change so we are skipping them for now, but +// leaving them here for future reference when considering the next major version +describe.skip('404 caching', () => { describe('404 without getStaticProps', () => { test('not matching dynamic paths should be cached permanently', async (ctx) => { await createFixture('page-router', ctx) @@ -177,7 +172,7 @@ describe('404 caching', () => { 'should be cached permanently', ).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable') }) - test('matching dynamic path with revalidate should be cached permanently', async (ctx) => { + test('matching dynamic path with revalidate should be cached for revalidate time', async (ctx) => { await createFixture('page-router', ctx) await runPlugin(ctx) @@ -189,8 +184,8 @@ describe('404 caching', () => { expect( notExistingPage.headers['netlify-cdn-cache-control'], - 'should be cached permanently', - ).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable') + 'should be cached for revalidate time', + ).toBe('s-maxage=600, stale-while-revalidate=31536000, durable') }) }) @@ -210,7 +205,7 @@ describe('404 caching', () => { 'should be cached permanently', ).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable') }) - test('matching dynamic path with revalidate should be cached permanently', async (ctx) => { + test('matching dynamic path with revalidate should be cached for revalidate time', async (ctx) => { await createFixture('page-router-base-path-i18n', ctx) await runPlugin(ctx) @@ -222,8 +217,8 @@ describe('404 caching', () => { expect( notExistingPage.headers['netlify-cdn-cache-control'], - 'should be cached permanently', - ).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable') + 'should be cached for revalidate time', + ).toBe('s-maxage=600, stale-while-revalidate=31536000, durable') }) }) @@ -251,7 +246,7 @@ describe('404 caching', () => { ).toBe('s-maxage=300, stale-while-revalidate=31536000, durable') }) - test('matching dynamic path with revalidate should be cached for 404 page revalidate', async (ctx) => { + test('matching dynamic path with revalidate should be cached for revalidate time', async (ctx) => { await createFixture('page-router-404-get-static-props-with-revalidate', ctx) await runPlugin(ctx) @@ -269,8 +264,76 @@ describe('404 caching', () => { expect( notExistingPage.headers['netlify-cdn-cache-control'], - 'should be cached for 404 page revalidate', - ).toBe('s-maxage=300, stale-while-revalidate=31536000, durable') + 'should be cached for revalidate time', + ).toBe('s-maxage=600, stale-while-revalidate=31536000, durable') + }) + }) +}) + +// This is a temporary fix to ensure that the 404 page itself is cached correctly when requested +// directly. This is a workaround for a specific customer and should be removed once the 404 caching +// is fixed in the next major version. +describe.only('404 page caching', () => { + beforeAll(() => { + process.env.CACHE_404_PAGE = 'true' + }) + + afterAll(() => { + delete process.env.CACHE_404_PAGE + }) + + test('404 without getStaticProps', async (ctx) => { + await createFixture('page-router', ctx) + await runPlugin(ctx) + + const notExistingPage = await invokeFunction(ctx, { + url: '/404', + }) + + expect(notExistingPage.statusCode).toBe(404) + + expect( + notExistingPage.headers['netlify-cdn-cache-control'], + 'should be cached permanently', + ).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable') + }) + + test('404 with getStaticProps without revalidate', async (ctx) => { + await createFixture('page-router-base-path-i18n', ctx) + await runPlugin(ctx) + + const notExistingPage = await invokeFunction(ctx, { + url: '/base/404', + }) + + expect(notExistingPage.statusCode).toBe(404) + + expect( + notExistingPage.headers['netlify-cdn-cache-control'], + 'should be cached permanently', + ).toBe('s-maxage=31536000, stale-while-revalidate=31536000, durable') + }) + + test('404 with getStaticProps with revalidate', async (ctx) => { + await createFixture('page-router-404-get-static-props-with-revalidate', ctx) + await runPlugin(ctx) + + // ignoring initial stale case + await invokeFunction(ctx, { + url: '/404', }) + + await new Promise((res) => setTimeout(res, 100)) + + const notExistingPage = await invokeFunction(ctx, { + url: '/404', + }) + + expect(notExistingPage.statusCode).toBe(404) + + expect( + notExistingPage.headers['netlify-cdn-cache-control'], + 'should be cached for 404 page revalidate', + ).toBe('s-maxage=300, stale-while-revalidate=31536000, durable') }) }) From a6f37e2cf630485608fbd4c36995ec5035d7cc34 Mon Sep 17 00:00:00 2001 From: Rob Stanford Date: Thu, 27 Feb 2025 19:18:19 +0000 Subject: [PATCH 14/15] test: remove only modifier --- tests/integration/page-router.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/page-router.test.ts b/tests/integration/page-router.test.ts index 8fda544d67..25dbfaba41 100644 --- a/tests/integration/page-router.test.ts +++ b/tests/integration/page-router.test.ts @@ -273,7 +273,7 @@ describe.skip('404 caching', () => { // This is a temporary fix to ensure that the 404 page itself is cached correctly when requested // directly. This is a workaround for a specific customer and should be removed once the 404 caching // is fixed in the next major version. -describe.only('404 page caching', () => { +describe('404 page caching', () => { beforeAll(() => { process.env.CACHE_404_PAGE = 'true' }) From 70a3ae6e6b3f59666c525406dba01d92d67d73c3 Mon Sep 17 00:00:00 2001 From: Rob Stanford Date: Fri, 28 Feb 2025 14:46:28 +0000 Subject: [PATCH 15/15] test: fix tests for new custom 404 pate --- tests/e2e/page-router.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/e2e/page-router.test.ts b/tests/e2e/page-router.test.ts index 06f8abc29d..7be19bee58 100644 --- a/tests/e2e/page-router.test.ts +++ b/tests/e2e/page-router.test.ts @@ -1,6 +1,6 @@ import { expect } from '@playwright/test' -import { test } from '../utils/playwright-helpers.js' import { nextVersionSatisfies } from '../utils/next-version-helpers.mjs' +import { test } from '../utils/playwright-helpers.js' export function waitFor(millis: number) { return new Promise((resolve) => setTimeout(resolve, millis)) @@ -405,7 +405,7 @@ test.describe('Simple Page Router (no basePath, no i18n)', () => { const headers = response?.headers() || {} expect(response?.status()).toBe(404) - expect(await page.textContent('h1')).toBe('404') + expect(await page.textContent('p')).toBe('Custom 404 page') // https://github.com/vercel/next.js/pull/69802 made changes to returned cache-control header, // after that (14.2.10 and canary.147) 404 pages would have `private` directive, before that @@ -429,7 +429,7 @@ test.describe('Simple Page Router (no basePath, no i18n)', () => { const headers = response?.headers() || {} expect(response?.status()).toBe(404) - expect(await page.textContent('h1')).toBe('404') + expect(await page.textContent('p')).toBe('Custom 404 page') expect(headers['netlify-cdn-cache-control']).toBe( nextVersionSatisfies('>=15.0.0-canary.187') @@ -1269,7 +1269,7 @@ test.describe('Page Router with basePath and i18n', () => { const headers = response?.headers() || {} expect(response?.status()).toBe(404) - expect(await page.textContent('h1')).toBe('404') + expect(await page.textContent('p')).toBe('Custom 404 page') // https://github.com/vercel/next.js/pull/69802 made changes to returned cache-control header, // after that 404 pages would have `private` directive, before that it would not @@ -1292,7 +1292,7 @@ test.describe('Page Router with basePath and i18n', () => { const headers = response?.headers() || {} expect(response?.status()).toBe(404) - expect(await page.textContent('h1')).toBe('404') + expect(await page.textContent('p')).toBe('Custom 404 page') expect(headers['netlify-cdn-cache-control']).toBe( nextVersionSatisfies('>=15.0.0-canary.187')