Skip to content

Conversation

@pilcrowonpaper
Copy link

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Nov 20, 2025

⚠️ No Changeset found

Latest commit: 4e4a7f9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

status: 508,
});
}
throw new Error("Failed to fetch image");
Copy link
Author

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

@sommeeeer
Copy link
Collaborator

Hello @pilcrowonpaper, good to see you here :) looks good on first glance, I'll do another review later!

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 20, 2025

Open in StackBlitz

npm i https://pkg.pr.new/@opennextjs/cloudflare@999

commit: 4e4a7f9

} else {
const cacheControlHeader = imageResponse.headers.get("Cache-Control");
if (cacheControlHeader !== null) {
immutable = cacheControlHeader.includes("immutable");
Copy link
Author

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

@pilcrowonpaper
Copy link
Author

It looks like the Playwright tests are failing because the test cases don't include the required w and q parameters.

const __IMAGES_CONTENT_DISPOSITION__ = JSON.stringify(
imagesManifest?.images?.contentDispositionType ?? "attachment"
);
const __IMAGES_MAX_REDIRECTS__ = JSON.stringify(imagesManifest?.images?.maximumRedirects ?? 3);
Copy link
Author

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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];
Copy link
Author

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];
Copy link
Author

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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, {
Copy link
Author

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.

) {
const imageUrl = url.searchParams.get("url") ?? "";
return await fetchImage(env.ASSETS, imageUrl, ctx);
return await handleImageRequest(url, request.headers, env);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe pass the request instead of url and headers to handleImageRequest ?

Copy link
Author

Choose a reason for hiding this comment

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

We already create a URL instance in the worker so this allows us to reuse it and avoid parsing the URL string twice

@pilcrowonpaper
Copy link
Author

@vicb
Should the behavior match Next.js 15 or 16 in general? Or should we check the version and change the behavior based on it? Not sure what the policy is on Next.js versions

};

let NEXT_IMAGE_REGEXP: RegExp;
export async function handleImageRequest(
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add JSDoc to this function

Comment on lines +254 to +258
const result: ErrorResult = {
ok: false,
message: '"url" parameter is required',
};
return result;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: That should be enough?

Suggested change
const result: ErrorResult = {
ok: false,
message: '"url" parameter is required',
};
return result;
return {
ok: false,
message: '"url" parameter is required',
};

Copy link
Author

Choose a reason for hiding this comment

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

Personally I don't like returning objects directly and prefer explicitly defining the type. I can change it if it bothers you

Copy link
Contributor

Choose a reason for hiding this comment

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

you can use satisfies if you really want to add a type.
But all "nit" comments are mostly suggestion, pick what you prefer.

url = urlQueryValue;

const pathname = getPathnameFromRelativeURL(url);
if (/\/_next\/image($|\/)/.test(decodeURIComponent(pathname))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about /_next/image?url

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I don't understand?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would not match as you have a $

Copy link
Author

Choose a reason for hiding this comment

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

It matches against the pathname so it should be fine, no? It matches against /_next/image, /_next/image/, and /_next/image/*

: {};

const __IMAGES_REMOTE_PATTERNS__ = JSON.stringify(imagesManifest?.images?.remotePatterns ?? []);
const __IMAGES_LOCAL_PATTERNS_DEFINED__ = JSON.stringify(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed given that __IMAGES_LOCAL_PATTERNS__ default to an empty list?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, Next.js treats undefined and an empty list differently

return result;
}
const widthQueryValue = widthQueryValues[0]!;
if (!/^[0-9]+$/.test(widthQueryValue)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
if (!/^[0-9]+$/.test(widthQueryValue)) {
if (!/^\d+$/.test(widthQueryValue)) {

Copy link
Author

Choose a reason for hiding this comment

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

For context, most of the parameter validation is based on Next.js' own code

if (!hasRemoteMatch(remotePatterns, parsedURL)) {
const result: ErrorResult = {
ok: false,
message: '"url" parameter is not allowed',
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe you can drop ok and rename "message" to "error"?

Copy link
Author

Choose a reason for hiding this comment

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

I personally don't like dealing with undefined and optional parameters so I prefer having with an ok property that is always defined, but let me know if you want me to change it

Comment on lines +422 to +426
url: url,
width: width,
quality: quality,
format: format,
static: staticAsset,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
url: url,
width: width,
quality: quality,
format: format,
static: staticAsset,
url,
width,
quality,
format,
static: staticAsset,

return new Response('"url" parameter is valid but upstream response is invalid', {
status: 400,
});
type ParseImageRequestURLResult = ParseImageRequestURLSuccessResult | ErrorResult;
Copy link
Contributor

Choose a reason for hiding this comment

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

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";
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: why have those been removed? (IIRC it comes from Next)

Copy link
Author

Choose a reason for hiding this comment

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

I've removed formats that aren't supported by Cloudflare:
https://developers.cloudflare.com/images/transform-images/

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the image binding is not used and we try to access one of this removed type?

Copy link
Author

Choose a reason for hiding this comment

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

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

let imageResponse: Response;
if (parseResult.url.startsWith("/")) {
if (env.ASSETS === undefined) {
console.error("env.ASSETS binding is not defined.");
Copy link
Contributor

Choose a reason for hiding this comment

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

use the logger rather than console.

Also we should fallback to the former behavior (serve unoptimized images) when the binding is not present)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops that's ASSET, nvm the comment about former behavior but still please switch to using the logger.

Copy link
Author

Choose a reason for hiding this comment

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

I've had some issues with wrangler not being able resolve @opennextjs/aws imports but I'll look into it again

const imageTransformationResult = await imageSource
.transform({
width: parseResult.width,
fit: "scale-down",
Copy link
Contributor

@vicb vicb Nov 21, 2025

Choose a reason for hiding this comment

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

question: Does not Next always use scale-down?

Copy link
Author

Choose a reason for hiding this comment

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

Unsure. There's so no documentation on the default value
https://developers.cloudflare.com/images/transform-images/transform-via-url/#fit

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the typo in my initial message, the question was about Next

Copy link
Author

Choose a reason for hiding this comment

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

I'll need to double check, but I just assumed so because you mentioned that the images shouldn't be scaled more than the original size. (For context, scale-down will only scale down and never scale up images.)

Copy link
Contributor

@vicb vicb left a comment

Choose a reason for hiding this comment

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

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants