Only cache successful fetches (#48033)

### What? Change the caching logic for fetch-cache to only cache successful responses. ### Why? Currently fetch-cache will cache any response, without checking the http status code. But situations like 500 and 304 and others should not be cached, because we want to re-fetch from the origin. ### How? Add an extra check before deciding to call `incrementalCache.set()` --------- Co-authored-by: JJ Kasper <jj@jjsweb.site>
vercel · Apr 6, 2023 · 76ad58b · 76ad58b
1 parent 95e46f7
commit 76ad58b
Show file tree

Hide file tree

Showing 4 changed files with 9 additions and 16 deletions.
diff --git a/packages/next/src/server/lib/incremental-cache/index.ts b/packages/next/src/server/lib/incremental-cache/index.ts
@@ -155,7 +155,7 @@ export class IncrementalCache {
   ): Promise<string> {
     // this should be bumped anytime a fix is made to cache entries
     // that should bust the cache
-    const MAIN_KEY_PREFIX = 'v1'
+    const MAIN_KEY_PREFIX = 'v2'
 
     let cacheKey: string
     const bodyChunks: string[] = []

diff --git a/packages/next/src/server/lib/patch-fetch.ts b/packages/next/src/server/lib/patch-fetch.ts
@@ -225,6 +225,7 @@ export function patchFetch({
         const doOriginalFetch = async () => {
           return originFetch(input, init).then(async (res) => {
             if (
+              res.ok &&
               staticGenerationStore.incrementalCache &&
               cacheKey &&
               typeof revalidate === 'number' &&

diff --git a/test/e2e/app-dir/app-static/app-static.test.ts b/test/e2e/app-dir/app-static/app-static.test.ts
@@ -168,19 +168,16 @@ createNextDescribe(
       })
     }
 
-    it('should include statusCode in cache', async () => {
+    it('should not cache non-ok statusCode', async () => {
       const $ = await next.render$('/variable-revalidate/status-code')
       const origData = JSON.parse($('#page-data').text())
 
       expect(origData.status).toBe(404)
 
-      await check(async () => {
-        const new$ = await next.render$('/variable-revalidate/status-code')
-        const newData = JSON.parse(new$('#page-data').text())
-        expect(newData.status).toBe(origData.status)
-        expect(newData.text).not.toBe(origData.text)
-        return 'success'
-      }, 'success')
+      const new$ = await next.render$('/variable-revalidate/status-code')
+      const newData = JSON.parse(new$('#page-data').text())
+      expect(newData.status).toBe(origData.status)
+      expect(newData.text).not.toBe(origData.text)
     })
 
     if (isNextStart) {
@@ -313,8 +310,6 @@ createNextDescribe(
           'variable-revalidate/revalidate-360.html',
           'variable-revalidate/revalidate-360.rsc',
           'variable-revalidate/revalidate-360/page.js',
-          'variable-revalidate/status-code.html',
-          'variable-revalidate/status-code.rsc',
           'variable-revalidate/status-code/page.js',
         ])
       })
@@ -530,11 +525,6 @@ createNextDescribe(
             initialRevalidateSeconds: 10,
             srcRoute: '/variable-revalidate/revalidate-360',
           },
-          '/variable-revalidate/status-code': {
-            dataRoute: '/variable-revalidate/status-code.rsc',
-            initialRevalidateSeconds: 3,
-            srcRoute: '/variable-revalidate/status-code',
-          },
         })
         expect(curManifest.dynamicRoutes).toEqual({
           '/blog/[author]/[slug]': {

diff --git a/test/e2e/app-dir/app-static/app/variable-revalidate/status-code/page.js b/test/e2e/app-dir/app-static/app/variable-revalidate/status-code/page.js
@@ -1,3 +1,5 @@
+export const revalidate = 0
+
 export default async function Page() {
   const data = await fetch(
     'https://next-data-api-endpoint.vercel.app/api/random?status=404',