Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: refactor image optimization to separate external/internal urls #61172

Merged
merged 5 commits into from
Jan 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 56 additions & 36 deletions packages/next/src/server/image-optimizer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 : ''
Expand Down Expand Up @@ -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
Expand All @@ -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') || ''
Expand Down Expand Up @@ -516,40 +524,34 @@ 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

if (isAbsolute) {
const upstreamRes = await fetch(href)

if (!upstreamRes.ok) {
Log.error('upstream image response failed for', href, upstreamRes.status)
throw new ImageError(
upstreamRes.status,
'"url" parameter is valid but upstream 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 {
): Promise<ImageUpstream> {
try {
const mocked = createRequestResponseMocks({
url: href,
Expand All @@ -569,12 +571,10 @@ export async function imageOptimizer(
)
}

upstreamBuffer = Buffer.concat(mocked.res.buffers)
upstreamType =
detectContentType(upstreamBuffer) ||
mocked.res.getHeader('Content-Type')
const buffer = Buffer.concat(mocked.res.buffers)
const contentType = mocked.res.getHeader('Content-Type')
const cacheControl = mocked.res.getHeader('Cache-Control')
maxAge = cacheControl ? getMaxAge(cacheControl) : 0
return { buffer, contentType, cacheControl }
} catch (err) {
Log.error('upstream image response failed for', href, err)
throw new ImageError(
Expand All @@ -584,9 +584,29 @@ export async function imageOptimizer(
}
}

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're only using cacheControl from imageUpstream to retrieve the max age, would it be worth refactoring to have fetchImageUpstream take care of that for us? And then have the ImageUpstream type return maxAge rather than cacheControl (unless we do need cacheControl for some reason that I'm not seeing in this diff).

Copy link
Member Author

@styfle styfle Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two functions: fetchExternalImage and fetchInternalImage so both of them would need to call getMaxAge().

My PR only requires a single getMaxAge().

I think the inputs here make sense because it allows you to pass in the result of the upstream (buffer from the body plus relevant headers) without needing to create the shape of a NodeResponse or WebResponse.

Maybe I should rename ImageUpstream to ImageUpstreamResponse to make it a little more clear?

const upstreamType =
detectContentType(upstreamBuffer) ||
imageUpstream.contentType?.toLowerCase().trim()

if (upstreamType) {
if (
upstreamType.startsWith('image/svg') &&
!nextConfig.images.dangerouslyAllowSVG
Expand Down
34 changes: 23 additions & 11 deletions packages/next/src/server/next-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -544,20 +544,15 @@ 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) => {
const handleInternalReq = async (
newReq: IncomingMessage,
newRes: ServerResponse
) => {
if (newReq.url === req.url) {
throw new Error(
`Invariant attempted to optimize _next/image itself`
)
throw new Error(`Invariant attempted to optimize _next/image itself`)
}

const protocol = this.serverOptions.experimentalHttpsServer
Expand Down Expand Up @@ -591,6 +586,23 @@ export default class NextNodeServer extends BaseServer {
}
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
)
}
}
Expand Down
Loading