From f8fe70dd50113ef3dbf46c6292aa49cc37f9b395 Mon Sep 17 00:00:00 2001 From: Steven Date: Fri, 26 Jan 2024 11:20:20 -0500 Subject: [PATCH] chore: refactor image optimization to separate external/internal urls (#61172) This PR is refactoring only and doesn't change the logic, therefore no tests were added. The improvement here is readability: - separate functions to fetch external vs internal image (in the future, we could improve perf of internal image routing) - types of the arguments are more specific (rather than passing in complete NextConfig) > Note: Its easier to review with whitespace hidden: https://github.com/vercel/next.js/pull/61172/files?w=1 --- packages/next/src/server/image-optimizer.ts | 134 +++++++++++--------- packages/next/src/server/next-server.ts | 92 ++++++++------ 2 files changed, 129 insertions(+), 97 deletions(-) diff --git a/packages/next/src/server/image-optimizer.ts b/packages/next/src/server/image-optimizer.ts index 6e69481e36930..7ad6f90be46e0 100644 --- a/packages/next/src/server/image-optimizer.ts +++ b/packages/next/src/server/image-optimizer.ts @@ -72,6 +72,12 @@ export interface ImageParamsResult { minimumCacheTTL: number } +interface ImageUpstream { + buffer: Buffer + contentType: string | null | undefined + cacheControl: string | null | undefined +} + function getSupportedMimeType(options: string[], accept = ''): string { const mimeType = mediaType(accept, options) return accept.includes(mimeType) ? mimeType : '' @@ -373,7 +379,9 @@ export class ImageError extends Error { } } -function parseCacheControl(str: string | null): Map { +function parseCacheControl( + str: string | null | undefined +): Map { const map = new Map() if (!str) { return map @@ -389,7 +397,7 @@ function parseCacheControl(str: string | null): Map { return map } -export function getMaxAge(str: string | null): number { +export function getMaxAge(str: string | null | undefined): number { const map = parseCacheControl(str) if (map) { let age = map.get('s-maxage') || map.get('max-age') || '' @@ -516,77 +524,89 @@ export async function optimizeImage({ return optimizedBuffer } -export async function imageOptimizer( +export async function fetchExternalImage(href: string): Promise { + const res = await fetch(href) + + if (!res.ok) { + Log.error('upstream image response failed for', href, res.status) + throw new ImageError( + res.status, + '"url" parameter is valid but upstream response is invalid' + ) + } + + const buffer = Buffer.from(await res.arrayBuffer()) + const contentType = res.headers.get('Content-Type') + const cacheControl = res.headers.get('Cache-Control') + + return { buffer, contentType, cacheControl } +} + +export async function fetchInternalImage( + href: string, _req: IncomingMessage, _res: ServerResponse, - paramsResult: ImageParamsResult, - nextConfig: NextConfigComplete, - isDev: boolean | undefined, handleRequest: ( newReq: IncomingMessage, newRes: ServerResponse, newParsedUrl?: NextUrlWithParsedQuery ) => Promise -): Promise<{ buffer: Buffer; contentType: string; maxAge: number }> { - let upstreamBuffer: Buffer - let upstreamType: string | null | undefined - let maxAge: number - const { isAbsolute, href, width, mimeType, quality } = paramsResult +): Promise { + try { + const mocked = createRequestResponseMocks({ + url: href, + method: _req.method || 'GET', + headers: _req.headers, + socket: _req.socket, + }) - if (isAbsolute) { - const upstreamRes = await fetch(href) + await handleRequest(mocked.req, mocked.res, nodeUrl.parse(href, true)) + await mocked.res.hasStreamed - if (!upstreamRes.ok) { - Log.error('upstream image response failed for', href, upstreamRes.status) + if (!mocked.res.statusCode) { + Log.error('image response failed for', href, mocked.res.statusCode) throw new ImageError( - upstreamRes.status, - '"url" parameter is valid but upstream response is invalid' + mocked.res.statusCode, + '"url" parameter is valid but internal response is invalid' ) } - upstreamBuffer = Buffer.from(await upstreamRes.arrayBuffer()) - upstreamType = - detectContentType(upstreamBuffer) || - upstreamRes.headers.get('Content-Type') - maxAge = getMaxAge(upstreamRes.headers.get('Cache-Control')) - } else { - try { - const mocked = createRequestResponseMocks({ - url: href, - method: _req.method || 'GET', - headers: _req.headers, - socket: _req.socket, - }) - - await handleRequest(mocked.req, mocked.res, nodeUrl.parse(href, true)) - await mocked.res.hasStreamed - - if (!mocked.res.statusCode) { - Log.error('image response failed for', href, mocked.res.statusCode) - throw new ImageError( - mocked.res.statusCode, - '"url" parameter is valid but internal response is invalid' - ) - } - - upstreamBuffer = Buffer.concat(mocked.res.buffers) - upstreamType = - detectContentType(upstreamBuffer) || - mocked.res.getHeader('Content-Type') - const cacheControl = mocked.res.getHeader('Cache-Control') - maxAge = cacheControl ? getMaxAge(cacheControl) : 0 - } catch (err) { - Log.error('upstream image response failed for', href, err) - throw new ImageError( - 500, - '"url" parameter is valid but upstream response is invalid' - ) - } + const buffer = Buffer.concat(mocked.res.buffers) + const contentType = mocked.res.getHeader('Content-Type') + const cacheControl = mocked.res.getHeader('Cache-Control') + return { buffer, contentType, cacheControl } + } catch (err) { + Log.error('upstream image response failed for', href, err) + throw new ImageError( + 500, + '"url" parameter is valid but upstream response is invalid' + ) } +} - if (upstreamType) { - upstreamType = upstreamType.toLowerCase().trim() +export async function imageOptimizer( + imageUpstream: ImageUpstream, + paramsResult: Pick< + ImageParamsResult, + 'href' | 'width' | 'quality' | 'mimeType' + >, + nextConfig: { + output: NextConfigComplete['output'] + images: Pick< + NextConfigComplete['images'], + 'dangerouslyAllowSVG' | 'minimumCacheTTL' + > + }, + isDev: boolean | undefined +): Promise<{ buffer: Buffer; contentType: string; maxAge: number }> { + const { href, quality, width, mimeType } = paramsResult + const upstreamBuffer = imageUpstream.buffer + const maxAge = getMaxAge(imageUpstream.cacheControl) + const upstreamType = + detectContentType(upstreamBuffer) || + imageUpstream.contentType?.toLowerCase().trim() + if (upstreamType) { if ( upstreamType.startsWith('image/svg') && !nextConfig.images.dangerouslyAllowSVG diff --git a/packages/next/src/server/next-server.ts b/packages/next/src/server/next-server.ts index b7da975e85a56..799f3701333c0 100644 --- a/packages/next/src/server/next-server.ts +++ b/packages/next/src/server/next-server.ts @@ -544,53 +544,65 @@ export default class NextNodeServer extends BaseServer { 'invariant: imageOptimizer should not be called in minimal mode' ) } else { - const { imageOptimizer } = + const { imageOptimizer, fetchExternalImage, fetchInternalImage } = require('./image-optimizer') as typeof import('./image-optimizer') - return imageOptimizer( - req.originalRequest, - res.originalResponse, - paramsResult, - this.nextConfig, - this.renderOpts.dev, - async (newReq, newRes) => { - if (newReq.url === req.url) { - throw new Error( - `Invariant attempted to optimize _next/image itself` - ) - } + const handleInternalReq = async ( + newReq: IncomingMessage, + newRes: ServerResponse + ) => { + if (newReq.url === req.url) { + throw new Error(`Invariant attempted to optimize _next/image itself`) + } - const protocol = this.serverOptions.experimentalHttpsServer - ? 'https' - : 'http' - - const invokeRes = await invokeRequest( - `${protocol}://${this.fetchHostname || 'localhost'}:${this.port}${ - newReq.url || '' - }`, - { - method: newReq.method || 'GET', - headers: newReq.headers, - signal: signalFromNodeResponse(res.originalResponse), - } - ) - const filteredResHeaders = filterReqHeaders( - toNodeOutgoingHttpHeaders(invokeRes.headers), - ipcForbiddenHeaders - ) + const protocol = this.serverOptions.experimentalHttpsServer + ? 'https' + : 'http' - for (const key of Object.keys(filteredResHeaders)) { - newRes.setHeader(key, filteredResHeaders[key] || '') + const invokeRes = await invokeRequest( + `${protocol}://${this.fetchHostname || 'localhost'}:${this.port}${ + newReq.url || '' + }`, + { + method: newReq.method || 'GET', + headers: newReq.headers, + signal: signalFromNodeResponse(res.originalResponse), } - newRes.statusCode = invokeRes.status || 200 + ) + const filteredResHeaders = filterReqHeaders( + toNodeOutgoingHttpHeaders(invokeRes.headers), + ipcForbiddenHeaders + ) - if (invokeRes.body) { - await pipeToNodeResponse(invokeRes.body, newRes) - } else { - res.send() - } - return + for (const key of Object.keys(filteredResHeaders)) { + newRes.setHeader(key, filteredResHeaders[key] || '') } + newRes.statusCode = invokeRes.status || 200 + + if (invokeRes.body) { + await pipeToNodeResponse(invokeRes.body, newRes) + } else { + res.send() + } + return + } + + const { isAbsolute, href } = paramsResult + + const imageUpstream = isAbsolute + ? await fetchExternalImage(href) + : await fetchInternalImage( + href, + req.originalRequest, + res.originalResponse, + handleInternalReq + ) + + return imageOptimizer( + imageUpstream, + paramsResult, + this.nextConfig, + this.renderOpts.dev ) } }