Add image optimization with Cloudflare Images #999
Conversation
|
|
Hello @pilcrowonpaper, good to see you here :) looks good on first glance, I'll do another review later! |
commit: |
| } else { | ||
| const cacheControlHeader = imageResponse.headers.get("Cache-Control"); | ||
| if (cacheControlHeader !== null) { | ||
| immutable = cacheControlHeader.includes("immutable"); |
There was a problem hiding this comment.
Next.js does something more complicated and bases the optimized response on the upstream Cache-Control header. I didn't want to do anything complex for now so it just checks if the upstream image is a static content
|
It looks like the Playwright tests are failing because the test cases don't include the required |
| const __IMAGES_CONTENT_DISPOSITION__ = JSON.stringify( | ||
| imagesManifest?.images?.contentDispositionType ?? "attachment" | ||
| ); | ||
| const __IMAGES_MAX_REDIRECTS__ = JSON.stringify(imagesManifest?.images?.maximumRedirects ?? 3); |
There was a problem hiding this comment.
This was changed to 3 from unlimited in Next.js 16
There was a problem hiding this comment.
We could use unlimited then (and maybe add your comment as a code comment on this file/line)
|
|
||
| const defaultDeviceSizes = [640, 750, 828, 1080, 1200, 1920, 2048, 3840]; | ||
| const defaultImageSizes = [32, 48, 64, 96, 128, 256, 384]; | ||
| const defaultQualities = [75]; |
There was a problem hiding this comment.
Before Next.js 16, the default behavior was to allow values from 1-100
| } | ||
|
|
||
| const defaultDeviceSizes = [640, 750, 828, 1080, 1200, 1920, 2048, 3840]; | ||
| const defaultImageSizes = [32, 48, 64, 96, 128, 256, 384]; |
There was a problem hiding this comment.
Before Next.js 16, 16 was also included
There was a problem hiding this comment.
I would use Next 16 (and add comments on the lines).
nit: maybe move those constants up (i.e. before they are used)
| async function fetchImage(url: string, count: number): Promise<FetchImageResult> { | ||
| let response: Response; | ||
| try { | ||
| response = await fetch(url, { |
There was a problem hiding this comment.
I haven't added support for dangerouslyAllowLocalIP, which checks if a domain resolves to a private IP.
There was a problem hiding this comment.
Could you please add a note in the JSDoc?
|
@vicb |
| : {}; | ||
|
|
||
| const __IMAGES_REMOTE_PATTERNS__ = JSON.stringify(imagesManifest?.images?.remotePatterns ?? []); | ||
| const __IMAGES_LOCAL_PATTERNS_DEFINED__ = JSON.stringify( |
There was a problem hiding this comment.
Is this needed given that __IMAGES_LOCAL_PATTERNS__ default to an empty list?
There was a problem hiding this comment.
Yes, Next.js treats undefined and an empty list differently
There was a problem hiding this comment.
How so?
If the behavior is different then we could pass the value differently to avoid having to defines.
There was a problem hiding this comment.
I updated the variable name so we don't have an awkward "is defined" variable
| return new Response('"url" parameter is valid but upstream response is invalid', { | ||
| status: 400, | ||
| }); | ||
| type ParseImageRequestURLResult = ParseImageRequestURLSuccessResult | ErrorResult; |
There was a problem hiding this comment.
nit: move the type above the function where they are used
| const WEBP = "image/webp"; | ||
| const PNG = "image/png"; | ||
| const JPEG = "image/jpeg"; | ||
| const JXL = "image/jxl"; |
There was a problem hiding this comment.
Question: why have those been removed? (IIRC it comes from Next)
There was a problem hiding this comment.
I've removed formats that aren't supported by Cloudflare:
https://developers.cloudflare.com/images/transform-images/
There was a problem hiding this comment.
What happens if the image binding is not used and we try to access one of this removed type?
There was a problem hiding this comment.
That part I was unsure. I personally think we should only serve images that are supported by the optimization process, even when the binding isn't defined. It'd be weird if your images stopped being served when you turn on image optimization by defining the binding. It will be a breaking change though
There was a problem hiding this comment.
Let's not do this in this PR to keep the current behavior.
We can decide to changer at a later point.
vicb
left a comment
vicb
left a comment
There was a problem hiding this comment.
Awesome @pilcrowonpaper !
I have done a frist round of review.
Could you please add JSDoc comments to added functions.
Maybe there could be separate functions to i.e. validate each parameter?
| try { | ||
| response = await fetch(url, { | ||
| signal: AbortSignal.timeout(timeout), | ||
| redirect: "manual", |
There was a problem hiding this comment.
nit: only use "manual" when redirect is not max?
There was a problem hiding this comment.
You commented that Next 16 do not limit the number of redirects.
In that case we should not do manual redirects but just follow redirects.
Do that make sense?
There was a problem hiding this comment.
No, I meant the opposite. Next.js 16 does limit redirects, while it didn't in Next.js 15
| response.headers.set("Vary", "Accept"); | ||
| response.headers.set("Content-Type", contentType); | ||
| response.headers.set("Content-Disposition", __IMAGES_CONTENT_DISPOSITION__); | ||
| response.headers.set("Content-Security-Policy", __IMAGES_CONTENT_SECURITY_POLICY__); | ||
| if (imageResponseFlags.immutable) { | ||
| response.headers.set("Cache-Control", "public, max-age=315360000, immutable"); |
There was a problem hiding this comment.
nit: passing this in the Response ctor (options.headers) might be less verbose
|
It looks like we can't add tests for the |
| : {}; | ||
|
|
||
| const __IMAGES_REMOTE_PATTERNS__ = JSON.stringify(imagesManifest?.images?.remotePatterns ?? []); | ||
| const __IMAGES_ALLOW_ALL_LOCAL_PATHS__ = JSON.stringify( |
There was a problem hiding this comment.
Can't we completely remove that?
i.e. set __IMAGES_LOCAL_PATTERNS__ to [{pathname: '**'}] when the local pattern is undefined?
3 participants
No description provided.