From 4cdc33fa885c2caff0ab1f0a9cb38970acb8268f Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Tue, 1 Mar 2022 11:53:09 -0800 Subject: [PATCH 1/5] Handle de-duping revalidations in minimal mode --- packages/next/server/base-server.ts | 12 +++- packages/next/server/response-cache.ts | 65 +++++++++++++++---- test/production/required-server-files.test.ts | 28 ++++++++ .../required-server-files/pages/gsp.js | 4 ++ 4 files changed, 93 insertions(+), 16 deletions(-) diff --git a/packages/next/server/base-server.ts b/packages/next/server/base-server.ts index 9a6e81bf2590..7c4b632d9d0e 100644 --- a/packages/next/server/base-server.ts +++ b/packages/next/server/base-server.ts @@ -373,7 +373,10 @@ export default abstract class Server { } }, }) - this.responseCache = new ResponseCache(this.incrementalCache) + this.responseCache = new ResponseCache( + this.incrementalCache, + this.minimalMode + ) } public logError(err: Error): void { @@ -1275,9 +1278,13 @@ export default abstract class Server { resolvedUrlPathname = stripNextDataPath(resolvedUrlPathname) urlPathname = stripNextDataPath(urlPathname) } + const canLeveragePreviousCache = !!req.headers['x-nextjs-dedupe'] let ssgCacheKey = - isPreviewMode || !isSSG || this.minimalMode || opts.supportsDynamicHTML + isPreviewMode || + !isSSG || + (this.minimalMode && !canLeveragePreviousCache) || + opts.supportsDynamicHTML ? null // Preview mode and manual revalidate bypasses the cache : `${locale ? `/${locale}` : ''}${ (pathname === '/' || resolvedUrlPathname === '/') && locale @@ -1513,6 +1520,7 @@ export default abstract class Server { }, { isManualRevalidate, + canLeveragePreviousCache, } ) diff --git a/packages/next/server/response-cache.ts b/packages/next/server/response-cache.ts index 4887c9f31a37..9d3f71155371 100644 --- a/packages/next/server/response-cache.ts +++ b/packages/next/server/response-cache.ts @@ -79,16 +79,26 @@ interface IncrementalCache { export default class ResponseCache { incrementalCache: IncrementalCache pendingResponses: Map> + previousCacheItem?: { + key: string + entry: ResponseCacheEntry | null + expiresAt: number | false + } + minimalMode?: boolean - constructor(incrementalCache: IncrementalCache) { + constructor(incrementalCache: IncrementalCache, minimalMode: boolean) { this.incrementalCache = incrementalCache this.pendingResponses = new Map() + this.minimalMode = minimalMode } public get( key: string | null, responseGenerator: ResponseGenerator, - context: { isManualRevalidate?: boolean } + context: { + isManualRevalidate?: boolean + canLeveragePreviousCache?: boolean + } ): Promise { const pendingResponse = key ? this.pendingResponses.get(key) : null if (pendingResponse) { @@ -119,12 +129,28 @@ export default class ResponseCache { } } + // we keep the previous cache entry around to leverage + // when the incremental cache is disabled in minimal mode + if ( + key && + this.minimalMode && + context.canLeveragePreviousCache && + this.previousCacheItem?.key === key && + (this.previousCacheItem.expiresAt === false || + this.previousCacheItem.expiresAt > Date.now()) + ) { + resolve(this.previousCacheItem.entry) + return promise + } + // We wait to do any async work until after we've added our promise to // `pendingResponses` to ensure that any any other calls will reuse the // same promise until we've fully finished our work. ;(async () => { try { - const cachedResponse = key ? await this.incrementalCache.get(key) : null + const cachedResponse = + key && !this.minimalMode ? await this.incrementalCache.get(key) : null + if (cachedResponse && !context.isManualRevalidate) { resolve({ isStale: cachedResponse.isStale, @@ -156,17 +182,28 @@ export default class ResponseCache { ) if (key && cacheEntry && typeof cacheEntry.revalidate !== 'undefined') { - await this.incrementalCache.set( - key, - cacheEntry.value?.kind === 'PAGE' - ? { - kind: 'PAGE', - html: cacheEntry.value.html.toUnchunkedString(), - pageData: cacheEntry.value.pageData, - } - : cacheEntry.value, - cacheEntry.revalidate - ) + if (this.minimalMode) { + this.previousCacheItem = { + key, + entry: cacheEntry, + expiresAt: + typeof cacheEntry.revalidate !== 'number' + ? false + : Date.now() + cacheEntry?.revalidate * 1000, + } + } else { + await this.incrementalCache.set( + key, + cacheEntry.value?.kind === 'PAGE' + ? { + kind: 'PAGE', + html: cacheEntry.value.html.toUnchunkedString(), + pageData: cacheEntry.value.pageData, + } + : cacheEntry.value, + cacheEntry.revalidate + ) + } } } catch (err) { // while revalidating in the background we can't reject as diff --git a/test/production/required-server-files.test.ts b/test/production/required-server-files.test.ts index 04b6156b24c7..3a732681a12b 100644 --- a/test/production/required-server-files.test.ts +++ b/test/production/required-server-files.test.ts @@ -155,6 +155,34 @@ describe('should set-up next', () => { expect(typeof requiredFilesManifest.appDir).toBe('string') }) + it('should de-dupe HTML/data requests with x-nextjs-dedupe', async () => { + const res = await fetchViaHTTP(appPort, '/gsp', undefined, { + redirect: 'manual', + headers: { + 'x-nextjs-dedupe': '1', + }, + }) + expect(res.status).toBe(200) + const $ = cheerio.load(await res.text()) + const props = JSON.parse($('#props').text()) + expect(props.gspCalls).toBeDefined() + + const res2 = await fetchViaHTTP( + appPort, + `/_next/data/${next.buildId}/gsp.json`, + undefined, + { + redirect: 'manual', + headers: { + 'x-nextjs-dedupe': '1', + }, + } + ) + expect(res2.status).toBe(200) + const { pageProps: props2 } = await res2.json() + expect(props2.gspCalls).toBe(props.gspCalls) + }) + it('should set correct SWR headers with notFound gsp', async () => { await next.patchFile('standalone/data.txt', 'show') diff --git a/test/production/required-server-files/pages/gsp.js b/test/production/required-server-files/pages/gsp.js index 33b90f80db2c..2ab4e866214a 100644 --- a/test/production/required-server-files/pages/gsp.js +++ b/test/production/required-server-files/pages/gsp.js @@ -1,11 +1,14 @@ import fs from 'fs' import path from 'path' +let gspCalls = 0 + export async function getStaticProps() { const data = await fs.promises.readFile( path.join(process.cwd(), 'data.txt'), 'utf8' ) + gspCalls += 1 if (data.trim() === 'hide') { return { @@ -18,6 +21,7 @@ export async function getStaticProps() { props: { hello: 'world', data, + gspCalls, }, revalidate: 1, } From ec23ec996a7041833b6b5f2a23259f3396c08941 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Tue, 1 Mar 2022 11:55:19 -0800 Subject: [PATCH 2/5] fix type --- packages/next/server/next-server.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/next/server/next-server.ts b/packages/next/server/next-server.ts index 69d30e7357d4..42bdcaf9ce7f 100644 --- a/packages/next/server/next-server.ts +++ b/packages/next/server/next-server.ts @@ -120,7 +120,8 @@ export default class NextNodeServer extends BaseServer { new ImageOptimizerCache({ distDir: this.distDir, nextConfig: this.nextConfig, - }) + }), + this.minimalMode ) } From 2db1ab0af2c6095c7be5e61de45941ea5f534056 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Wed, 2 Mar 2022 11:49:35 -0800 Subject: [PATCH 3/5] remove header check --- packages/next/server/base-server.ts | 7 +------ packages/next/server/response-cache.ts | 12 ++++++------ test/production/required-server-files.test.ts | 12 +++++------- 3 files changed, 12 insertions(+), 19 deletions(-) diff --git a/packages/next/server/base-server.ts b/packages/next/server/base-server.ts index 7c4b632d9d0e..c8ac3ba1e7f7 100644 --- a/packages/next/server/base-server.ts +++ b/packages/next/server/base-server.ts @@ -1278,13 +1278,9 @@ export default abstract class Server { resolvedUrlPathname = stripNextDataPath(resolvedUrlPathname) urlPathname = stripNextDataPath(urlPathname) } - const canLeveragePreviousCache = !!req.headers['x-nextjs-dedupe'] let ssgCacheKey = - isPreviewMode || - !isSSG || - (this.minimalMode && !canLeveragePreviousCache) || - opts.supportsDynamicHTML + isPreviewMode || !isSSG || opts.supportsDynamicHTML ? null // Preview mode and manual revalidate bypasses the cache : `${locale ? `/${locale}` : ''}${ (pathname === '/' || resolvedUrlPathname === '/') && locale @@ -1520,7 +1516,6 @@ export default abstract class Server { }, { isManualRevalidate, - canLeveragePreviousCache, } ) diff --git a/packages/next/server/response-cache.ts b/packages/next/server/response-cache.ts index 9d3f71155371..aadebfccaa6a 100644 --- a/packages/next/server/response-cache.ts +++ b/packages/next/server/response-cache.ts @@ -82,7 +82,7 @@ export default class ResponseCache { previousCacheItem?: { key: string entry: ResponseCacheEntry | null - expiresAt: number | false + expiresAt: number } minimalMode?: boolean @@ -97,7 +97,6 @@ export default class ResponseCache { responseGenerator: ResponseGenerator, context: { isManualRevalidate?: boolean - canLeveragePreviousCache?: boolean } ): Promise { const pendingResponse = key ? this.pendingResponses.get(key) : null @@ -134,12 +133,11 @@ export default class ResponseCache { if ( key && this.minimalMode && - context.canLeveragePreviousCache && this.previousCacheItem?.key === key && - (this.previousCacheItem.expiresAt === false || - this.previousCacheItem.expiresAt > Date.now()) + this.previousCacheItem.expiresAt > Date.now() ) { resolve(this.previousCacheItem.entry) + this.pendingResponses.delete(key) return promise } @@ -188,7 +186,7 @@ export default class ResponseCache { entry: cacheEntry, expiresAt: typeof cacheEntry.revalidate !== 'number' - ? false + ? Date.now() + 1000 : Date.now() + cacheEntry?.revalidate * 1000, } } else { @@ -204,6 +202,8 @@ export default class ResponseCache { cacheEntry.revalidate ) } + } else { + this.previousCacheItem = undefined } } catch (err) { // while revalidating in the background we can't reject as diff --git a/test/production/required-server-files.test.ts b/test/production/required-server-files.test.ts index 3a732681a12b..4f8eb6969c6c 100644 --- a/test/production/required-server-files.test.ts +++ b/test/production/required-server-files.test.ts @@ -11,6 +11,7 @@ import { initNextServerScript, killApp, renderViaHTTP, + waitFor, } from 'next-test-utils' describe('should set-up next', () => { @@ -155,12 +156,9 @@ describe('should set-up next', () => { expect(typeof requiredFilesManifest.appDir).toBe('string') }) - it('should de-dupe HTML/data requests with x-nextjs-dedupe', async () => { + it('should de-dupe HTML/data requests', async () => { const res = await fetchViaHTTP(appPort, '/gsp', undefined, { redirect: 'manual', - headers: { - 'x-nextjs-dedupe': '1', - }, }) expect(res.status).toBe(200) const $ = cheerio.load(await res.text()) @@ -173,9 +171,6 @@ describe('should set-up next', () => { undefined, { redirect: 'manual', - headers: { - 'x-nextjs-dedupe': '1', - }, } ) expect(res2.status).toBe(200) @@ -184,6 +179,7 @@ describe('should set-up next', () => { }) it('should set correct SWR headers with notFound gsp', async () => { + await waitFor(2000) await next.patchFile('standalone/data.txt', 'show') const res = await fetchViaHTTP(appPort, '/gsp', undefined, { @@ -194,6 +190,7 @@ describe('should set-up next', () => { 's-maxage=1, stale-while-revalidate' ) + await waitFor(2000) await next.patchFile('standalone/data.txt', 'hide') const res2 = await fetchViaHTTP(appPort, '/gsp', undefined, { @@ -272,6 +269,7 @@ describe('should set-up next', () => { expect($('#slug').text()).toBe('first') expect(data.hello).toBe('world') + await waitFor(2000) const html2 = await renderViaHTTP(appPort, '/fallback/first') const $2 = cheerio.load(html2) const data2 = JSON.parse($2('#props').text()) From 4a618c246a5b2afb9da99b98a372995012990926 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Wed, 2 Mar 2022 12:06:36 -0800 Subject: [PATCH 4/5] update other tests --- .../required-server-files-ssr-404/test/index.test.js | 3 ++- test/production/required-server-files-i18n.test.ts | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/test/integration/required-server-files-ssr-404/test/index.test.js b/test/integration/required-server-files-ssr-404/test/index.test.js index 58ed20c8ffbf..0c91394b9b6a 100644 --- a/test/integration/required-server-files-ssr-404/test/index.test.js +++ b/test/integration/required-server-files-ssr-404/test/index.test.js @@ -4,7 +4,7 @@ import http from 'http' import fs from 'fs-extra' import { join } from 'path' import cheerio from 'cheerio' -import { nextServer } from 'next-test-utils' +import { nextServer, waitFor } from 'next-test-utils' import { fetchViaHTTP, findPort, @@ -140,6 +140,7 @@ describe('Required Server Files', () => { expect($('#slug').text()).toBe('first') expect(data.hello).toBe('world') + await waitFor(2000) const html2 = await renderViaHTTP(appPort, '/fallback/first') const $2 = cheerio.load(html2) const data2 = JSON.parse($2('#props').text()) diff --git a/test/production/required-server-files-i18n.test.ts b/test/production/required-server-files-i18n.test.ts index 832442d2e4f5..43303acf2485 100644 --- a/test/production/required-server-files-i18n.test.ts +++ b/test/production/required-server-files-i18n.test.ts @@ -11,6 +11,7 @@ import { initNextServerScript, killApp, renderViaHTTP, + waitFor, } from 'next-test-utils' describe('should set-up next', () => { @@ -133,6 +134,7 @@ describe('should set-up next', () => { 's-maxage=1, stale-while-revalidate' ) + await waitFor(2000) await next.patchFile('standalone/data.txt', 'hide') const res2 = await fetchViaHTTP(appPort, '/gsp', undefined, { @@ -211,6 +213,7 @@ describe('should set-up next', () => { expect($('#slug').text()).toBe('first') expect(data.hello).toBe('world') + await waitFor(2000) const html2 = await renderViaHTTP(appPort, '/fallback/first') const $2 = cheerio.load(html2) const data2 = JSON.parse($2('#props').text()) From 6f8a3266ba50c5c319a9c72e91eea8e2a239421c Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Wed, 2 Mar 2022 14:43:31 -0800 Subject: [PATCH 5/5] update test --- test/e2e/prerender.test.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/e2e/prerender.test.ts b/test/e2e/prerender.test.ts index b6c6f8bf6427..0eeb864ed47b 100644 --- a/test/e2e/prerender.test.ts +++ b/test/e2e/prerender.test.ts @@ -164,6 +164,11 @@ describe('Prerender', () => { initialRevalidateSeconds: 1, srcRoute: '/blocking-fallback-some/[slug]', }, + '/blocking-fallback/test-errors-1': { + dataRoute: `/_next/data/${next.buildId}/blocking-fallback/test-errors-1.json`, + initialRevalidateSeconds: 1, + srcRoute: '/blocking-fallback/[slug]', + }, '/blog': { dataRoute: `/_next/data/${next.buildId}/blog.json`, initialRevalidateSeconds: 10,