From 2373320fc859af3424a8ab7e211b8292e3aafa67 Mon Sep 17 00:00:00 2001 From: Steven Date: Wed, 30 Jun 2021 17:26:20 -0400 Subject: [PATCH] Add upstream `max-age` to optimized image (#26739) This solves the main use case from Issue #19914. Previously, we would set the `Cache-Control` header to a constant and rely on the server cache. This would mean the browser would always request the image and the server could response with 304 Not Modified to omit the response body. This PR changes the behavior such that the `max-age` will propagate from the upstream server to the Next.js Image Optimization Server and allow browser caching. ("upstream" meaning external server or just an internal route to an image) This PR does not change the `max-age` for static imports which will remain `public, max-age=315360000, immutable`. #### Pros: - Fewer HTTP requests after initial browser visit - User configurable `max-age` via the upstream image `Cache-Control` header #### Cons: - ~~Might be annoying for `next dev` when modifying a source image~~ (solved: use `max-age=0` for dev) - Might cause browser to cache longer than expected (up to 2x longer than the server cache if requested in the last second before expiration) ## Bug - [x] Related issues linked using `fixes #number` --- packages/next/server/image-optimizer.ts | 120 ++++++++++++++---- packages/next/server/next-server.ts | 3 +- .../image-optimizer/test/index.test.js | 108 ++++++++-------- 3 files changed, 146 insertions(+), 85 deletions(-) diff --git a/packages/next/server/image-optimizer.ts b/packages/next/server/image-optimizer.ts index 5def834f5993e..30a175203e7c2 100644 --- a/packages/next/server/image-optimizer.ts +++ b/packages/next/server/image-optimizer.ts @@ -22,7 +22,7 @@ const PNG = 'image/png' const JPEG = 'image/jpeg' const GIF = 'image/gif' const SVG = 'image/svg+xml' -const CACHE_VERSION = 2 +const CACHE_VERSION = 3 const MODERN_TYPES = [/* AVIF, */ WEBP] const ANIMATABLE_TYPES = [WEBP, PNG, GIF] const VECTOR_TYPES = [SVG] @@ -35,7 +35,8 @@ export async function imageOptimizer( res: ServerResponse, parsedUrl: UrlWithParsedQuery, nextConfig: NextConfig, - distDir: string + distDir: string, + isDev = false ) { const imageData: ImageConfig = nextConfig.images || imageConfigDefault const { deviceSizes = [], imageSizes = [], domains = [], loader } = imageData @@ -158,24 +159,24 @@ export async function imageOptimizer( if (await fileExists(hashDir, 'directory')) { const files = await promises.readdir(hashDir) for (let file of files) { - const [prefix, etag, extension] = file.split('.') - const expireAt = Number(prefix) + const [maxAgeStr, expireAtSt, etag, extension] = file.split('.') + const maxAge = Number(maxAgeStr) + const expireAt = Number(expireAtSt) const contentType = getContentType(extension) const fsPath = join(hashDir, file) if (now < expireAt) { - res.setHeader( - 'Cache-Control', - isStatic - ? 'public, max-age=315360000, immutable' - : 'public, max-age=0, must-revalidate' + const result = setResponseHeaders( + req, + res, + etag, + maxAge, + contentType, + isStatic, + isDev ) - if (sendEtagResponse(req, res, etag)) { - return { finished: true } + if (!result.finished) { + createReadStream(fsPath).pipe(res) } - if (contentType) { - res.setHeader('Content-Type', contentType) - } - createReadStream(fsPath).pipe(res) return { finished: true } } else { await promises.unlink(fsPath) @@ -271,8 +272,22 @@ export async function imageOptimizer( const animate = ANIMATABLE_TYPES.includes(upstreamType) && isAnimated(upstreamBuffer) if (vector || animate) { - await writeToCacheDir(hashDir, upstreamType, expireAt, upstreamBuffer) - sendResponse(req, res, upstreamType, upstreamBuffer, isStatic) + await writeToCacheDir( + hashDir, + upstreamType, + maxAge, + expireAt, + upstreamBuffer + ) + sendResponse( + req, + res, + maxAge, + upstreamType, + upstreamBuffer, + isStatic, + isDev + ) return { finished: true } } @@ -342,13 +357,35 @@ export async function imageOptimizer( } if (optimizedBuffer) { - await writeToCacheDir(hashDir, contentType, expireAt, optimizedBuffer) - sendResponse(req, res, contentType, optimizedBuffer, isStatic) + await writeToCacheDir( + hashDir, + contentType, + maxAge, + expireAt, + optimizedBuffer + ) + sendResponse( + req, + res, + maxAge, + contentType, + optimizedBuffer, + isStatic, + isDev + ) } else { throw new Error('Unable to optimize buffer') } } catch (error) { - sendResponse(req, res, upstreamType, upstreamBuffer, isStatic) + sendResponse( + req, + res, + maxAge, + upstreamType, + upstreamBuffer, + isStatic, + isDev + ) } return { finished: true } @@ -362,37 +399,64 @@ export async function imageOptimizer( async function writeToCacheDir( dir: string, contentType: string, + maxAge: number, expireAt: number, buffer: Buffer ) { await promises.mkdir(dir, { recursive: true }) const extension = getExtension(contentType) const etag = getHash([buffer]) - const filename = join(dir, `${expireAt}.${etag}.${extension}`) + const filename = join(dir, `${maxAge}.${expireAt}.${etag}.${extension}`) await promises.writeFile(filename, buffer) } -function sendResponse( +function setResponseHeaders( req: IncomingMessage, res: ServerResponse, + etag: string, + maxAge: number, contentType: string | null, - buffer: Buffer, - isStatic: boolean + isStatic: boolean, + isDev: boolean ) { - const etag = getHash([buffer]) res.setHeader( 'Cache-Control', isStatic ? 'public, max-age=315360000, immutable' - : 'public, max-age=0, must-revalidate' + : `public, max-age=${isDev ? 0 : maxAge}, must-revalidate` ) if (sendEtagResponse(req, res, etag)) { - return + // already called res.end() so we're finished + return { finished: true } } if (contentType) { res.setHeader('Content-Type', contentType) } - res.end(buffer) + return { finished: false } +} + +function sendResponse( + req: IncomingMessage, + res: ServerResponse, + maxAge: number, + contentType: string | null, + buffer: Buffer, + isStatic: boolean, + isDev: boolean +) { + const etag = getHash([buffer]) + const result = setResponseHeaders( + req, + res, + etag, + maxAge, + contentType, + isStatic, + isDev + ) + if (!result.finished) { + res.end(buffer) + } } function getSupportedMimeType(options: string[], accept = ''): string { diff --git a/packages/next/server/next-server.ts b/packages/next/server/next-server.ts index 32ef98432e60c..4485694598587 100644 --- a/packages/next/server/next-server.ts +++ b/packages/next/server/next-server.ts @@ -789,7 +789,8 @@ export default class Server { res, parsedUrl, server.nextConfig, - server.distDir + server.distDir, + this.renderOpts.dev ), }, { diff --git a/test/integration/image-optimizer/test/index.test.js b/test/integration/image-optimizer/test/index.test.js index e553e0a31dc19..161301d3d28b3 100644 --- a/test/integration/image-optimizer/test/index.test.js +++ b/test/integration/image-optimizer/test/index.test.js @@ -13,7 +13,6 @@ import { } from 'next-test-utils' import isAnimated from 'next/dist/compiled/is-animated' import { join } from 'path' -import { createHash } from 'crypto' jest.setTimeout(1000 * 60 * 2) @@ -56,8 +55,8 @@ function runTests({ w, isDev, domains }) { const res = await fetchViaHTTP(appPort, '/_next/image', query, {}) expect(res.status).toBe(200) expect(res.headers.get('content-type')).toContain('image/gif') - expect(res.headers.get('cache-control')).toBe( - 'public, max-age=0, must-revalidate' + expect(res.headers.get('Cache-Control')).toBe( + `public, max-age=${isDev ? 0 : 60}, must-revalidate` ) expect(res.headers.get('etag')).toBeTruthy() expect(isAnimated(await res.buffer())).toBe(true) @@ -68,8 +67,8 @@ function runTests({ w, isDev, domains }) { const res = await fetchViaHTTP(appPort, '/_next/image', query, {}) expect(res.status).toBe(200) expect(res.headers.get('content-type')).toContain('image/png') - expect(res.headers.get('cache-control')).toBe( - 'public, max-age=0, must-revalidate' + expect(res.headers.get('Cache-Control')).toBe( + `public, max-age=${isDev ? 0 : 60}, must-revalidate` ) expect(res.headers.get('etag')).toBeTruthy() expect(isAnimated(await res.buffer())).toBe(true) @@ -80,8 +79,8 @@ function runTests({ w, isDev, domains }) { const res = await fetchViaHTTP(appPort, '/_next/image', query, {}) expect(res.status).toBe(200) expect(res.headers.get('content-type')).toContain('image/webp') - expect(res.headers.get('cache-control')).toBe( - 'public, max-age=0, must-revalidate' + expect(res.headers.get('Cache-Control')).toBe( + `public, max-age=${isDev ? 0 : 60}, must-revalidate` ) expect(res.headers.get('etag')).toBeTruthy() expect(isAnimated(await res.buffer())).toBe(true) @@ -93,8 +92,8 @@ function runTests({ w, isDev, domains }) { const res = await fetchViaHTTP(appPort, '/_next/image', query, opts) 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=0, must-revalidate' + expect(res.headers.get('Cache-Control')).toBe( + `public, max-age=${isDev ? 0 : 60}, must-revalidate` ) expect(res.headers.get('etag')).toBeTruthy() const actual = await res.text() @@ -111,8 +110,8 @@ function runTests({ w, isDev, domains }) { const res = await fetchViaHTTP(appPort, '/_next/image', query, opts) 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=0, must-revalidate' + expect(res.headers.get('Cache-Control')).toBe( + `public, max-age=${isDev ? 0 : 60}, must-revalidate` ) expect(res.headers.get('etag')).toBeTruthy() const actual = await res.text() @@ -131,8 +130,8 @@ function runTests({ w, isDev, domains }) { const res = await fetchViaHTTP(appPort, '/_next/image', query, opts) expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toContain('image/jpeg') - expect(res.headers.get('cache-control')).toBe( - 'public, max-age=0, must-revalidate' + expect(res.headers.get('Cache-Control')).toBe( + `public, max-age=${isDev ? 0 : 60}, must-revalidate` ) expect(res.headers.get('etag')).toBeTruthy() }) @@ -145,8 +144,8 @@ function runTests({ w, isDev, domains }) { const res = await fetchViaHTTP(appPort, '/_next/image', query, opts) expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toContain('image/png') - expect(res.headers.get('cache-control')).toBe( - 'public, max-age=0, must-revalidate' + expect(res.headers.get('Cache-Control')).toBe( + `public, max-age=${isDev ? 0 : 60}, must-revalidate` ) expect(res.headers.get('etag')).toBeTruthy() }) @@ -242,8 +241,8 @@ function runTests({ w, isDev, domains }) { const res = await fetchViaHTTP(appPort, '/_next/image', query, opts) expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toBe('image/webp') - expect(res.headers.get('cache-control')).toBe( - 'public, max-age=0, must-revalidate' + expect(res.headers.get('Cache-Control')).toBe( + `public, max-age=${isDev ? 0 : 60}, must-revalidate` ) expect(res.headers.get('etag')).toBeTruthy() await expectWidth(res, w) @@ -255,8 +254,8 @@ function runTests({ w, isDev, domains }) { const res = await fetchViaHTTP(appPort, '/_next/image', query, opts) expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toBe('image/png') - expect(res.headers.get('cache-control')).toBe( - 'public, max-age=0, must-revalidate' + expect(res.headers.get('Cache-Control')).toBe( + `public, max-age=${isDev ? 0 : 60}, must-revalidate` ) expect(res.headers.get('etag')).toBeTruthy() await expectWidth(res, w) @@ -268,8 +267,8 @@ function runTests({ w, isDev, domains }) { const res = await fetchViaHTTP(appPort, '/_next/image', query, opts) expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toBe('image/png') - expect(res.headers.get('cache-control')).toBe( - 'public, max-age=0, must-revalidate' + expect(res.headers.get('Cache-Control')).toBe( + `public, max-age=${isDev ? 0 : 60}, must-revalidate` ) expect(res.headers.get('etag')).toBeTruthy() await expectWidth(res, w) @@ -281,8 +280,8 @@ function runTests({ w, isDev, domains }) { const res = await fetchViaHTTP(appPort, '/_next/image', query, opts) expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toBe('image/gif') - expect(res.headers.get('cache-control')).toBe( - 'public, max-age=0, must-revalidate' + expect(res.headers.get('Cache-Control')).toBe( + `public, max-age=${isDev ? 0 : 60}, must-revalidate` ) expect(res.headers.get('etag')).toBeTruthy() // FIXME: await expectWidth(res, w) @@ -294,8 +293,8 @@ function runTests({ w, isDev, domains }) { const res = await fetchViaHTTP(appPort, '/_next/image', query, opts) expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toBe('image/tiff') - expect(res.headers.get('cache-control')).toBe( - 'public, max-age=0, must-revalidate' + expect(res.headers.get('Cache-Control')).toBe( + `public, max-age=${isDev ? 0 : 60}, must-revalidate` ) expect(res.headers.get('etag')).toBeTruthy() // FIXME: await expectWidth(res, w) @@ -309,8 +308,8 @@ function runTests({ w, isDev, domains }) { const res = await fetchViaHTTP(appPort, '/_next/image', query, opts) expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toBe('image/webp') - expect(res.headers.get('cache-control')).toBe( - 'public, max-age=0, must-revalidate' + expect(res.headers.get('Cache-Control')).toBe( + `public, max-age=${isDev ? 0 : 60}, must-revalidate` ) expect(res.headers.get('etag')).toBeTruthy() await expectWidth(res, w) @@ -324,8 +323,8 @@ function runTests({ w, isDev, domains }) { const res = await fetchViaHTTP(appPort, '/_next/image', query, opts) expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toBe('image/webp') - expect(res.headers.get('cache-control')).toBe( - 'public, max-age=0, must-revalidate' + expect(res.headers.get('Cache-Control')).toBe( + `public, max-age=${isDev ? 0 : 60}, must-revalidate` ) expect(res.headers.get('etag')).toBeTruthy() await expectWidth(res, w) @@ -344,8 +343,8 @@ function runTests({ w, isDev, domains }) { const res = await fetchViaHTTP(appPort, '/_next/image', query, opts) expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toBe('image/webp') - expect(res.headers.get('cache-control')).toBe( - 'public, max-age=0, must-revalidate' + expect(res.headers.get('Cache-Control')).toBe( + `public, max-age=${isDev ? 0 : 60}, must-revalidate` ) expect(res.headers.get('etag')).toBeTruthy() await expectWidth(res, w) @@ -447,7 +446,7 @@ function runTests({ w, isDev, domains }) { expect(res1.status).toBe(200) expect(res1.headers.get('Content-Type')).toBe('image/webp') expect(res1.headers.get('Cache-Control')).toBe( - 'public, max-age=0, must-revalidate' + `public, max-age=${isDev ? 0 : 60}, must-revalidate` ) const etag = res1.headers.get('Etag') expect(etag).toBeTruthy() @@ -458,6 +457,9 @@ function runTests({ w, isDev, domains }) { expect(res2.status).toBe(304) 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 : 60}, must-revalidate` + ) expect((await res2.buffer()).length).toBe(0) const query3 = { url: '/test.jpg', w, q: 25 } @@ -465,7 +467,7 @@ function runTests({ w, isDev, domains }) { expect(res3.status).toBe(200) expect(res3.headers.get('Content-Type')).toBe('image/webp') expect(res3.headers.get('Cache-Control')).toBe( - 'public, max-age=0, must-revalidate' + `public, max-age=${isDev ? 0 : 60}, must-revalidate` ) expect(res3.headers.get('Etag')).toBeTruthy() expect(res3.headers.get('Etag')).not.toBe(etag) @@ -481,8 +483,8 @@ function runTests({ w, isDev, domains }) { const res = await fetchViaHTTP(appPort, '/_next/image', query, opts) expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toBe('image/bmp') - expect(res.headers.get('cache-control')).toBe( - 'public, max-age=0, must-revalidate' + expect(res.headers.get('Cache-Control')).toBe( + `public, max-age=${isDev ? 0 : 60}, must-revalidate` ) expect(res.headers.get('etag')).toBeTruthy() @@ -496,8 +498,8 @@ function runTests({ w, isDev, domains }) { const res = await fetchViaHTTP(appPort, '/_next/image', query, opts) expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toBe('image/webp') - expect(res.headers.get('cache-control')).toBe( - 'public, max-age=0, must-revalidate' + expect(res.headers.get('Cache-Control')).toBe( + `public, max-age=${isDev ? 0 : 60}, must-revalidate` ) expect(res.headers.get('etag')).toBeTruthy() await expectWidth(res, 400) @@ -511,8 +513,8 @@ function runTests({ w, isDev, domains }) { const res = await fetchViaHTTP(appPort, '/_next/image', query, opts) expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toBe('image/png') - expect(res.headers.get('cache-control')).toBe( - 'public, max-age=0, must-revalidate' + expect(res.headers.get('Cache-Control')).toBe( + `public, max-age=${isDev ? 0 : 60}, must-revalidate` ) const png = await res.buffer() @@ -535,7 +537,7 @@ function runTests({ w, isDev, domains }) { const res1 = await fetchViaHTTP(appPort, '/_next/image', query, opts) expect(res1.status).toBe(200) - expect(res1.headers.get('cache-control')).toBe( + expect(res1.headers.get('Cache-Control')).toBe( 'public, max-age=315360000, immutable' ) await expectWidth(res1, w) @@ -543,7 +545,7 @@ function runTests({ w, isDev, domains }) { // Ensure subsequent request also has immutable header const res2 = await fetchViaHTTP(appPort, '/_next/image', query, opts) expect(res2.status).toBe(200) - expect(res2.headers.get('cache-control')).toBe( + expect(res2.headers.get('Cache-Control')).toBe( 'public, max-age=315360000, immutable' ) await expectWidth(res2, w) @@ -559,6 +561,7 @@ function runTests({ w, isDev, domains }) { }) it('should handle concurrent requests', async () => { + await fs.remove(imagesDir) const query = { url: '/test.png', w, q: 80 } const opts = { headers: { accept: 'image/webp,*/*' } } const [res1, res2] = await Promise.all([ @@ -572,19 +575,8 @@ function runTests({ w, isDev, domains }) { await expectWidth(res1, w) await expectWidth(res2, w) - // There should be only one image created in the cache directory. - const hashItems = [2, '/test.png', w, 80, 'image/webp'] - const hash = createHash('sha256') - for (let item of hashItems) { - if (typeof item === 'number') hash.update(String(item)) - else { - hash.update(item) - } - } - const hashDir = hash.digest('base64').replace(/\//g, '-') - const dir = join(imagesDir, hashDir) - const files = await fs.readdir(dir) - expect(files.length).toBe(1) + const json1 = await fsToJson(imagesDir) + expect(Object.keys(json1).length).toBe(1) }) } @@ -720,6 +712,7 @@ describe('Image Optimizer', () => { const domains = [ 'localhost', 'example.com', + 'assets.vercel.com', 'image-optimization-test.vercel.app', ] @@ -861,10 +854,10 @@ describe('Image Optimizer', () => { }, }` nextConfig.replace('{ /* replaceme */ }', newConfig) + await nextBuild(appDir) appPort = await findPort() - app = await launchApp(appDir, appPort) + app = await nextStart(appDir, appPort) }) - afterAll(async () => { await killApp(app) nextConfig.restore() @@ -877,6 +870,9 @@ describe('Image Optimizer', () => { const res = await fetchViaHTTP(appPort, '/_next/image', query, opts) expect(res.status).toBe(200) expect(res.headers.get('Content-Type')).toBe('image/webp') + expect(res.headers.get('Cache-Control')).toBe( + `public, max-age=31536000, must-revalidate` + ) await expectWidth(res, 64) }) })