-
Notifications
You must be signed in to change notification settings - Fork 81
Add image optimization with Cloudflare Images #999
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
base: main
Are you sure you want to change the base?
Add image optimization with Cloudflare Images #999
Conversation
|
| status: 508, | ||
| }); | ||
| } | ||
| throw new Error("Failed to fetch image"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is consistent with Next.js but not sure if a 500 response is the best if the URL is invalid
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was changed to 3 from unlimited in Next.js 16
|
|
||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before Next.js 16, 16 was also included
| async function fetchImage(url: string, count: number): Promise<FetchImageResult> { | ||
| let response: Response; | ||
| try { | ||
| response = await fetch(url, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't added support for dangerouslyAllowLocalIP, which checks if a domain resolves to a private IP.
No description provided.