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
vercel · Feb 28, 2024 · f8fe70d · f8fe70d
1 parent 5a5f178
commit f8fe70d
Show file tree

Hide file tree

Showing 2 changed files with 129 additions and 97 deletions.
diff --git 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<string, string> {
+function parseCacheControl(
+  str: string | null | undefined
+): Map<string, string> {
   const map = new Map<string, string>()
   if (!str) {
     return map
@@ -389,7 +397,7 @@ function parseCacheControl(str: string | null): Map<string, string> {
   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<ImageUpstream> {
+  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<void>
-): 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<ImageUpstream> {
+  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
@@ -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
       )
     }
   }