From 7909aadbd88cd67fa312ef6861f74b1c3fb98b11 Mon Sep 17 00:00:00 2001 From: Steven Date: Mon, 19 Jul 2021 18:38:03 -0400 Subject: [PATCH] Fix `minimumCacheTTL` so it doesn't affect browser caching (#27307) In a previous PR (#27200), we added `minimumCacheTTL` to configure the time-to-live for the cached image. However, this was setting the `max-age` header. This PR ensures that `minimumCacheTTL` doesn't affect browser caching, only the upstream header can affect browser caching. This is a bit safer in case the developer accidentally caches something that shouldn't be and the cache needs to be invalidated. Simply delete the `.next/cache/images` directory. - Related to #19914 - Related to #22319 --- docs/basic-features/image-optimization.md | 4 +- packages/next/server/image-optimizer.ts | 15 +- .../image-optimizer/test/get-max-age.test.js | 37 +++-- .../image-optimizer/test/index.test.js | 135 ++++++++++++------ 4 files changed, 114 insertions(+), 77 deletions(-) diff --git a/docs/basic-features/image-optimization.md b/docs/basic-features/image-optimization.md index cce62c11a68b8..e9da1121f65b6 100644 --- a/docs/basic-features/image-optimization.md +++ b/docs/basic-features/image-optimization.md @@ -176,7 +176,7 @@ module.exports = { ### Minimum Cache TTL -You can configure the time to live (TTL) in seconds for cached optimized images. This will configure the server's image cache as well as the `Cache-Control` header sent to the browser. This is a global setting that will affect all images. In most cases, its better to use a [Static Image Import](#image-Imports) which will handle hashing file contents and caching the file. You can also configure the `Cache-Control` header on an individual upstream image instead. +You can configure the time to live (TTL) in seconds for cached optimized images. In many cases, its better to use a [Static Image Import](#image-Imports) which will handle hashing file contents and caching the file forever. ```js module.exports = { @@ -186,6 +186,8 @@ module.exports = { } ``` +If you need to add a `Cache-Control` header for the browser (not recommended), you can configure [`headers`](/docs/api-reference/next.config.js/headers) on the upstream image e.g. `/some-asset.jpg` not `/_next/image` itself. + ### Disable Static Imports The default behavior allows you to import static files such as `import icon from './icon.png` and then pass that to the `src` property. diff --git a/packages/next/server/image-optimizer.ts b/packages/next/server/image-optimizer.ts index 7820d1a361a1c..f815892ea4ba8 100644 --- a/packages/next/server/image-optimizer.ts +++ b/packages/next/server/image-optimizer.ts @@ -212,10 +212,7 @@ export async function imageOptimizer( upstreamType = detectContentType(upstreamBuffer) || upstreamRes.headers.get('Content-Type') - maxAge = getMaxAge( - upstreamRes.headers.get('Cache-Control'), - minimumCacheTTL - ) + maxAge = getMaxAge(upstreamRes.headers.get('Cache-Control')) } else { try { const resBuffers: Buffer[] = [] @@ -272,7 +269,7 @@ export async function imageOptimizer( upstreamBuffer = Buffer.concat(resBuffers) upstreamType = detectContentType(upstreamBuffer) || mockRes.getHeader('Content-Type') - maxAge = getMaxAge(mockRes.getHeader('Cache-Control'), minimumCacheTTL) + maxAge = getMaxAge(mockRes.getHeader('Cache-Control')) } catch (err) { res.statusCode = 500 res.end('"url" parameter is valid but upstream response is invalid') @@ -280,7 +277,7 @@ export async function imageOptimizer( } } - const expireAt = maxAge * 1000 + now + const expireAt = Math.max(maxAge, minimumCacheTTL) * 1000 + now if (upstreamType) { const vector = VECTOR_TYPES.includes(upstreamType) @@ -540,7 +537,7 @@ export function detectContentType(buffer: Buffer) { return null } -export function getMaxAge(str: string | null, minimumCacheTTL: number): number { +export function getMaxAge(str: string | null): number { const map = parseCacheControl(str) if (map) { let age = map.get('s-maxage') || map.get('max-age') || '' @@ -549,8 +546,8 @@ export function getMaxAge(str: string | null, minimumCacheTTL: number): number { } const n = parseInt(age, 10) if (!isNaN(n)) { - return Math.max(n, minimumCacheTTL) + return n } } - return minimumCacheTTL + return 0 } diff --git a/test/integration/image-optimizer/test/get-max-age.test.js b/test/integration/image-optimizer/test/get-max-age.test.js index b4cd38371ea79..36df6a8468e29 100644 --- a/test/integration/image-optimizer/test/get-max-age.test.js +++ b/test/integration/image-optimizer/test/get-max-age.test.js @@ -2,43 +2,40 @@ import { getMaxAge } from '../../../../packages/next/dist/server/image-optimizer.js' describe('getMaxAge', () => { - it('should return default when no cache-control provided', () => { - expect(getMaxAge(undefined, 60)).toBe(60) + it('should return 0 when no cache-control provided', () => { + expect(getMaxAge(undefined)).toBe(0) }) - it('should return default when cache-control is null', () => { - expect(getMaxAge(null, 60)).toBe(60) + it('should return 0 when cache-control is null', () => { + expect(getMaxAge(null)).toBe(0) }) - it('should return default when cache-control is empty string', () => { - expect(getMaxAge('', 60)).toBe(60) + it('should return 0 when cache-control is empty string', () => { + expect(getMaxAge('')).toBe(0) }) - it('should return default when cache-control max-age is less than default', () => { - expect(getMaxAge('max-age=30', 60)).toBe(60) + it('should return 0 when cache-control max-age is not a number', () => { + expect(getMaxAge('max-age=foo')).toBe(0) }) - it('should return default when cache-control max-age is not a number', () => { - expect(getMaxAge('max-age=foo', 60)).toBe(60) - }) - it('should return default when cache-control is no-cache', () => { - expect(getMaxAge('no-cache', 60)).toBe(60) + it('should return 0 when cache-control is no-cache', () => { + expect(getMaxAge('no-cache')).toBe(0) }) it('should return cache-control max-age lowercase', () => { - expect(getMaxAge('max-age=9999', 60)).toBe(9999) + expect(getMaxAge('max-age=9999')).toBe(9999) }) it('should return cache-control MAX-AGE uppercase', () => { - expect(getMaxAge('MAX-AGE=9999', 60)).toBe(9999) + expect(getMaxAge('MAX-AGE=9999')).toBe(9999) }) it('should return cache-control s-maxage lowercase', () => { - expect(getMaxAge('s-maxage=9999', 60)).toBe(9999) + expect(getMaxAge('s-maxage=9999')).toBe(9999) }) it('should return cache-control S-MAXAGE', () => { - expect(getMaxAge('S-MAXAGE=9999', 60)).toBe(9999) + expect(getMaxAge('S-MAXAGE=9999')).toBe(9999) }) it('should return cache-control s-maxage with spaces', () => { - expect(getMaxAge('public, max-age=5555, s-maxage=9999', 60)).toBe(9999) + expect(getMaxAge('public, max-age=5555, s-maxage=9999')).toBe(9999) }) it('should return cache-control s-maxage without spaces', () => { - expect(getMaxAge('public,s-maxage=9999,max-age=5555', 60)).toBe(9999) + expect(getMaxAge('public,s-maxage=9999,max-age=5555')).toBe(9999) }) it('should return cache-control for a quoted value', () => { - expect(getMaxAge('public, s-maxage="9999", max-age="5555"', 60)).toBe(9999) + expect(getMaxAge('public, s-maxage="9999", max-age="5555"')).toBe(9999) }) }) diff --git a/test/integration/image-optimizer/test/index.test.js b/test/integration/image-optimizer/test/index.test.js index 8ad03a97c7a9d..6a8145b3272f3 100644 --- a/test/integration/image-optimizer/test/index.test.js +++ b/test/integration/image-optimizer/test/index.test.js @@ -33,7 +33,7 @@ async function fsToJson(dir, output = {}) { output[file] = {} await fsToJson(fsPath, output[file]) } else { - output[file] = 'file' + output[file] = stat.mtime.toISOString() } } return output @@ -45,7 +45,7 @@ async function expectWidth(res, w) { expect(d.width).toBe(w) } -function runTests({ w, isDev, domains = [], ttl = 60 }) { +function runTests({ w, isDev, domains = [], ttl }) { it('should return home page', async () => { const res = await fetchViaHTTP(appPort, '/', null, {}) expect(await res.text()).toMatch(/Image Optimizer Home/m) @@ -57,7 +57,7 @@ function runTests({ w, isDev, domains = [], ttl = 60 }) { expect(res.status).toBe(200) expect(res.headers.get('content-type')).toContain('image/gif') expect(res.headers.get('Cache-Control')).toBe( - `public, max-age=${isDev ? 0 : ttl}, must-revalidate` + `public, max-age=0, must-revalidate` ) expect(res.headers.get('Vary')).toBe('Accept') expect(res.headers.get('etag')).toBeTruthy() @@ -70,7 +70,7 @@ function runTests({ w, isDev, domains = [], ttl = 60 }) { expect(res.status).toBe(200) expect(res.headers.get('content-type')).toContain('image/png') expect(res.headers.get('Cache-Control')).toBe( - `public, max-age=${isDev ? 0 : ttl}, must-revalidate` + `public, max-age=0, must-revalidate` ) expect(res.headers.get('Vary')).toBe('Accept') expect(res.headers.get('etag')).toBeTruthy() @@ -83,7 +83,7 @@ function runTests({ w, isDev, domains = [], ttl = 60 }) { expect(res.status).toBe(200) expect(res.headers.get('content-type')).toContain('image/webp') expect(res.headers.get('Cache-Control')).toBe( - `public, max-age=${isDev ? 0 : ttl}, must-revalidate` + `public, max-age=0, must-revalidate` ) expect(res.headers.get('Vary')).toBe('Accept') expect(res.headers.get('etag')).toBeTruthy() @@ -97,7 +97,7 @@ function runTests({ w, isDev, domains = [], ttl = 60 }) { expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toContain('image/svg+xml') expect(res.headers.get('Cache-Control')).toBe( - `public, max-age=${isDev ? 0 : ttl}, must-revalidate` + `public, max-age=0, must-revalidate` ) // SVG is compressible so will have accept-encoding set from // compression @@ -118,7 +118,7 @@ function runTests({ w, isDev, domains = [], ttl = 60 }) { expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toContain('image/x-icon') expect(res.headers.get('Cache-Control')).toBe( - `public, max-age=${isDev ? 0 : ttl}, must-revalidate` + `public, max-age=0, must-revalidate` ) expect(res.headers.get('Vary')).toMatch(/^Accept(,|$)/) expect(res.headers.get('etag')).toBeTruthy() @@ -139,7 +139,7 @@ function runTests({ w, isDev, domains = [], ttl = 60 }) { expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toContain('image/jpeg') expect(res.headers.get('Cache-Control')).toBe( - `public, max-age=${isDev ? 0 : ttl}, must-revalidate` + `public, max-age=0, must-revalidate` ) expect(res.headers.get('Vary')).toBe('Accept') expect(res.headers.get('etag')).toBeTruthy() @@ -154,7 +154,7 @@ function runTests({ w, isDev, domains = [], ttl = 60 }) { expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toContain('image/png') expect(res.headers.get('Cache-Control')).toBe( - `public, max-age=${isDev ? 0 : ttl}, must-revalidate` + `public, max-age=0, must-revalidate` ) expect(res.headers.get('Vary')).toBe('Accept') expect(res.headers.get('etag')).toBeTruthy() @@ -252,7 +252,7 @@ function runTests({ w, isDev, domains = [], ttl = 60 }) { expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toBe('image/webp') expect(res.headers.get('Cache-Control')).toBe( - `public, max-age=${isDev ? 0 : ttl}, must-revalidate` + `public, max-age=0, must-revalidate` ) expect(res.headers.get('Vary')).toBe('Accept') expect(res.headers.get('etag')).toBeTruthy() @@ -266,7 +266,7 @@ function runTests({ w, isDev, domains = [], ttl = 60 }) { expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toBe('image/png') expect(res.headers.get('Cache-Control')).toBe( - `public, max-age=${isDev ? 0 : ttl}, must-revalidate` + `public, max-age=0, must-revalidate` ) expect(res.headers.get('Vary')).toBe('Accept') expect(res.headers.get('etag')).toBeTruthy() @@ -280,7 +280,7 @@ function runTests({ w, isDev, domains = [], ttl = 60 }) { expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toBe('image/png') expect(res.headers.get('Cache-Control')).toBe( - `public, max-age=${isDev ? 0 : ttl}, must-revalidate` + `public, max-age=0, must-revalidate` ) expect(res.headers.get('Vary')).toBe('Accept') expect(res.headers.get('etag')).toBeTruthy() @@ -294,7 +294,7 @@ function runTests({ w, isDev, domains = [], ttl = 60 }) { expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toBe('image/gif') expect(res.headers.get('Cache-Control')).toBe( - `public, max-age=${isDev ? 0 : ttl}, must-revalidate` + `public, max-age=0, must-revalidate` ) expect(res.headers.get('Vary')).toBe('Accept') expect(res.headers.get('etag')).toBeTruthy() @@ -308,7 +308,7 @@ function runTests({ w, isDev, domains = [], ttl = 60 }) { expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toBe('image/tiff') expect(res.headers.get('Cache-Control')).toBe( - `public, max-age=${isDev ? 0 : ttl}, must-revalidate` + `public, max-age=0, must-revalidate` ) expect(res.headers.get('Vary')).toBe('Accept') expect(res.headers.get('etag')).toBeTruthy() @@ -324,7 +324,7 @@ function runTests({ w, isDev, domains = [], ttl = 60 }) { expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toBe('image/webp') expect(res.headers.get('Cache-Control')).toBe( - `public, max-age=${isDev ? 0 : ttl}, must-revalidate` + `public, max-age=0, must-revalidate` ) expect(res.headers.get('Vary')).toBe('Accept') expect(res.headers.get('etag')).toBeTruthy() @@ -340,7 +340,7 @@ function runTests({ w, isDev, domains = [], ttl = 60 }) { expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toBe('image/webp') expect(res.headers.get('Cache-Control')).toBe( - `public, max-age=${isDev ? 0 : ttl}, must-revalidate` + `public, max-age=0, must-revalidate` ) expect(res.headers.get('Vary')).toBe('Accept') expect(res.headers.get('etag')).toBeTruthy() @@ -361,7 +361,7 @@ function runTests({ w, isDev, domains = [], ttl = 60 }) { expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toBe('image/webp') expect(res.headers.get('Cache-Control')).toBe( - `public, max-age=${isDev ? 0 : ttl}, must-revalidate` + `public, max-age=0, must-revalidate` ) expect(res.headers.get('Vary')).toBe('Accept') expect(res.headers.get('etag')).toBeTruthy() @@ -416,6 +416,17 @@ function runTests({ w, isDev, domains = [], ttl = 60 }) { expect(res2.headers.get('Content-Type')).toBe('image/webp') const json2 = await fsToJson(imagesDir) expect(json2).toStrictEqual(json1) + + if (ttl) { + // Wait until expired so we can confirm image is regenerated + await waitFor(ttl * 1000) + const res3 = await fetchViaHTTP(appPort, '/_next/image', query, opts) + expect(res3.status).toBe(200) + expect(res3.headers.get('Content-Type')).toBe('image/webp') + const json3 = await fsToJson(imagesDir) + expect(json3).not.toStrictEqual(json1) + expect(Object.keys(json3).length).toBe(1) + } }) it('should use cached image file when parameters are the same for svg', async () => { @@ -464,7 +475,7 @@ function runTests({ w, isDev, domains = [], ttl = 60 }) { expect(res1.status).toBe(200) expect(res1.headers.get('Content-Type')).toBe('image/webp') expect(res1.headers.get('Cache-Control')).toBe( - `public, max-age=${isDev ? 0 : ttl}, must-revalidate` + `public, max-age=0, must-revalidate` ) expect(res1.headers.get('Vary')).toBe('Accept') const etag = res1.headers.get('Etag') @@ -477,7 +488,7 @@ function runTests({ w, isDev, domains = [], ttl = 60 }) { expect(res2.headers.get('Content-Type')).toBeFalsy() expect(res2.headers.get('Etag')).toBe(etag) expect(res2.headers.get('Cache-Control')).toBe( - `public, max-age=${isDev ? 0 : ttl}, must-revalidate` + `public, max-age=0, must-revalidate` ) expect(res2.headers.get('Vary')).toBe('Accept') expect((await res2.buffer()).length).toBe(0) @@ -487,7 +498,7 @@ function runTests({ w, isDev, domains = [], ttl = 60 }) { expect(res3.status).toBe(200) expect(res3.headers.get('Content-Type')).toBe('image/webp') expect(res3.headers.get('Cache-Control')).toBe( - `public, max-age=${isDev ? 0 : ttl}, must-revalidate` + `public, max-age=0, must-revalidate` ) expect(res3.headers.get('Vary')).toBe('Accept') expect(res3.headers.get('Etag')).toBeTruthy() @@ -505,7 +516,7 @@ function runTests({ w, isDev, domains = [], ttl = 60 }) { expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toBe('image/bmp') expect(res.headers.get('Cache-Control')).toBe( - `public, max-age=${isDev ? 0 : ttl}, must-revalidate` + `public, max-age=0, must-revalidate` ) // bmp is compressible so will have accept-encoding set from // compression @@ -523,7 +534,7 @@ function runTests({ w, isDev, domains = [], ttl = 60 }) { expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toBe('image/webp') expect(res.headers.get('Cache-Control')).toBe( - `public, max-age=${isDev ? 0 : ttl}, must-revalidate` + `public, max-age=0, must-revalidate` ) expect(res.headers.get('Vary')).toBe('Accept') expect(res.headers.get('etag')).toBeTruthy() @@ -539,7 +550,7 @@ function runTests({ w, isDev, domains = [], ttl = 60 }) { expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toBe('image/png') expect(res.headers.get('Cache-Control')).toBe( - `public, max-age=${isDev ? 0 : ttl}, must-revalidate` + `public, max-age=0, must-revalidate` ) expect(res.headers.get('Vary')).toBe('Accept') @@ -808,28 +819,6 @@ describe('Image Optimizer', () => { runTests({ w: size, isDev: true, domains }) }) - describe('dev support for minimumCacheTTL', () => { - const size = 96 // defaults defined in server/config.ts - const ttl = 500 - beforeAll(async () => { - const json = JSON.stringify({ - images: { - minimumCacheTTL: ttl, - }, - }) - nextConfig.replace('{ /* replaceme */ }', json) - appPort = await findPort() - app = await launchApp(appDir, appPort) - }) - afterAll(async () => { - await killApp(app) - nextConfig.restore() - await fs.remove(imagesDir) - }) - - runTests({ w: size, isDev: true, ttl }) - }) - describe('Server support w/o next.config.js', () => { const size = 384 // defaults defined in server/config.ts beforeAll(async () => { @@ -868,9 +857,9 @@ describe('Image Optimizer', () => { runTests({ w: size, isDev: false, domains }) }) - describe('Server support for minimumCacheTTL', () => { + describe('Server support for minimumCacheTTL in next.config.js', () => { const size = 96 // defaults defined in server/config.ts - const ttl = 900000 + const ttl = 5 // super low ttl in seconds beforeAll(async () => { const json = JSON.stringify({ images: { @@ -891,6 +880,58 @@ describe('Image Optimizer', () => { runTests({ w: size, isDev: false, ttl }) }) + describe('Server support for headers in next.config.js', () => { + const size = 96 // defaults defined in server/config.ts + beforeAll(async () => { + nextConfig.replace( + '{ /* replaceme */ }', + `{ + async headers() { + return [ + { + source: '/test.png', + headers: [ + { + key: 'Cache-Control', + value: 'public, max-age=86400, must-revalidate', + }, + ], + }, + ] + }, + }` + ) + await nextBuild(appDir) + appPort = await findPort() + app = await nextStart(appDir, appPort) + }) + afterAll(async () => { + await killApp(app) + nextConfig.restore() + await fs.remove(imagesDir) + }) + + it('should set max-age header from upstream when matching next.config.js', async () => { + const query = { url: '/test.png', w: size, q: 75 } + const opts = { headers: { accept: 'image/webp' } } + const res = await fetchViaHTTP(appPort, '/_next/image', query, opts) + expect(res.status).toBe(200) + expect(res.headers.get('Cache-Control')).toBe( + `public, max-age=86400, must-revalidate` + ) + }) + + it('should not set max-age header when not matching next.config.js', async () => { + const query = { url: '/test.jpg', w: size, q: 75 } + const opts = { headers: { accept: 'image/webp' } } + const res = await fetchViaHTTP(appPort, '/_next/image', query, opts) + expect(res.status).toBe(200) + expect(res.headers.get('Cache-Control')).toBe( + `public, max-age=0, must-revalidate` + ) + }) + }) + describe('Serverless support with next.config.js', () => { const size = 256 beforeAll(async () => {