Add opt-in Cloudflare binding image optimization during build - #16194
Conversation
🦋 Changeset detectedLatest commit: e84ed87 The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
the
pkg: integration
There was a problem hiding this comment.
I'm not sure if this is correct. In theory the image binding should have sharp for prerender optimization. And the binding should be available when workerd is used for prerendering.
cc @OliverSpeir @Princesseuh who might have more context to share
|
Cloudflare-binding intentionally does not transform images on pre-rendered routes locally during build. The intention is they go to the image endpoint at request time as well, one benefit of this is faster build times |
|
It should use the cloudflare binding itself to optimize them during build time though, not sharp. So like during |
|
@Desel72 I'm sorry, but we are still discussing if this is actually an bug or the expected behavior on Discord. So we might end up not merging this, but no decision yet. |
|
Thank you for the context, @alexanderniebuhr! I think I misunderstood the design intent behind |
@Desel72 yeah after discussion we think this the the better way. I'll update the original issue so it makes more sense. Would you like to still work on that? If so, do you want to use this PR or should we close it and you can create a new one? |
|
I'd love to use this PR @alexanderniebuhr I will solve perfectly. |
9d75820 to
904d809
Compare
|
Hi @matthewp @alexanderniebuhr I've done. Welcome to your feedback. Thanks |
|
@Desel72 we are going to review the PR as soon as we have bandwidth, but please fix the failing lint check :) |
|
@alexanderniebuhr Thanks for your reviewing. I've solved. |
|
Hi @alexanderniebuhr is there any update on this PR? |
|
We are still reviewing this. Current state is that this should be put behind a option, so users can opt-in and opt-out and the changeset can't be a patch, since it's a breaking change. |
alexanderniebuhr
left a comment
alexanderniebuhr
left a comment
There was a problem hiding this comment.
Please make sure that this is a major change, which means we need to update the changeset as well as have an docs PR. The behavior should also put behind a flag so users can opt-in or opt-out. I suggest using the triplet configuration suggested here: #15662
Hi @alexanderniebuhr, how are you? Sorry for the late. I've solved this. Please review this. |
|
@alexanderniebuhr |
00b344d to
f415455
Compare
Daedalus-Icarus
changed the title
|
Hi @alexanderniebuhr, I addressed the requested changes from the Apr 16 review:
Local verification:
Could you please re-review when you have bandwidth? |
|
@alexanderniebuhr |
f415455 to
9aae56e
Compare
9aae56e to
a6de8b9
Compare
a6de8b9 to
ae0c20f
Compare
…ding
Adds a compound imageService config { build: 'cloudflare-binding', runtime?: ... }
that opts in to transforming static images with the Cloudflare IMAGES binding in
the workerd prerender environment at build time, writing the optimized bytes
directly to the output directory (falling back to Sharp when the binding fails).
The string shorthand 'cloudflare-binding' keeps runtime-only behavior.
Closes withastro#16035
Co-authored-by: Daedalus <6442298+Daedalus-Icarus@users.noreply.github.com>
ae0c20f to
f0a0af3
Compare
matthewp
left a comment
matthewp
left a comment
There was a problem hiding this comment.
I think this is now covered, you can provide an object to differentiate between build time and runtime image services: https://docs.astro.build/en/guides/integrations-guide/cloudflare/#imageservice
|
@matthewp this gives the ability to use Cloudflare |
adamchal
left a comment
adamchal
left a comment
There was a problem hiding this comment.
@Daedalus-Icarus I love this idea, but I have some scaling concerns.
| hash, | ||
| finalPath, | ||
| transform: transform as Record<string, any>, | ||
| imageData, |
There was a problem hiding this comment.
This base64 and JSON encodes every prerendered transform into a single response. All optimized variants (+~33% for base64) are held in memory in full twice: once on the workerd side to JSON.stringify, once on the Node side to parse.
The runtime image-transform-endpoint avoids this by streaming raw bytes one image at a time. I think the build path should adopt a similar shape. It would be more local HTTP requests, but would allow for scale.
There was a problem hiding this comment.
@adamchal you are right, we can optimize here. Let me push an updated approach.
I don't think the idea of this PR is covered. Let's discuss this https://discord.com/channels/830184174198718474/845430950191038464/1530469987502915615 |
…ng them
The build path base64-encoded every prerendered transform into a single JSON
response. Peak memory held the whole optimized image set several times over:
once as base64 in the entries array, once in the `JSON.stringify` result, once
as the encoded response body, then again on the Node side to parse. A site with
a few hundred variants could exceed the isolate memory limit, and a large enough
one hits V8's max string length in `JSON.stringify`.
Transform one image per request instead, mirroring the runtime
`image-transform-endpoint`: `/__astro_image_transform` streams the optimized
bytes straight from the IMAGES binding into the client output directory, with
bounded concurrency. Peak memory is now proportional to the concurrency limit
rather than to the image set.
This also fixes the binding never actually running. The worker resolved the
original through the ASSETS binding, but at that point in the build the
unoptimized original lives in Astro's prerender output, not in the client
directory ASSETS serves, so every transform failed with an empty input. The
failure was swallowed by a bare `catch {}` and silently fell back to Sharp, so
builds looked successful while the binding did nothing. The original is now
streamed up as the request body, and failures are logged instead of hidden.
Adds a test that builds with the binding config and fails if any image falls
back to the Node-side service.
ArmandPhilippot
left a comment
ArmandPhilippot
left a comment
There was a problem hiding this comment.
I have a nit for the changeset, otherwise docs LGTM on this side. Thanks.
I don't want to block the PR, but here is my opinion on the DX, from the perspective of someone who might be using the Cloudflare integration for the first time.
I was reviewing the docs PR and, although my lack of familiarity with Cloudflare might not be helping, I think the current behavior for the shorthand is confusing.
The object shape is fine and, yes, this sounds like an improvement for users! But, IMO, imageService: 'cloudflare-binding' doesn't meet the POLA.
IIUC:
- with
imageService: 'cloudflare-binding', 'cloudflare-binding' is only used at runtime (and falls back to something else at build-time... I don't think we explain that in docs) - with
imageService: { build: 'cloudflare-binding' }, 'cloudflare-binding' is used both at build-time and runtime - with
imageService: { build: 'compile', runtime: 'cloudflare-binding' }, 'cloudflare-binding' is runtime only
In this context, I would expect imageService: 'cloudflare-binding' to be syntactic sugar for imageService: { build: 'cloudflare-binding', runtime: 'cloudflare-binding' }. This is not the case and someone would have to write the long version to use cloudflare-binding everywhere.
(well, it seems they can use imageService: { build: 'cloudflare-binding') }, right, but this doesn't change my point about what the shorthand should do).
I understand that this would be a breaking change for the integration as we can't just swap the default to use an object matching the current behavior (e.g. { build: 'compile', runtime: 'cloudflare-binding' }). Someone could have set imageService: 'cloudflare-binding' explicitly.
But, shouldn't we choose to release a major in order to improve the DX here? (and so rework the current way the shorthand works, now that users can use an object to fine-tune the configuration)
|
@ArmandPhilippot I agree with you, but IIRC we decided against changing the default for now, so this stays a minor, and we can do the switch later if wanted in a major. I also think your summary is not quite right, but I have to validate that.
|
Co-authored-by: Armand Philippot <git@armand.philippot.eu>
|
@alexanderniebuhr Yeah, I understand. As I said, I won't block the PR, but I thought I had to express my concern because it's not easy to document, and so I expect it to be just as confusing for users... And speaking of confusion... 😄 You said yourself in the docs PR that "
|
|
I think we should go ahead with this one, and do any other change in follow ups. |
6 participants
Changes
imageServiceconfiguration for using the Cloudflare Images binding during build-time prerendering. The defaultimageService: 'cloudflare-binding'shorthand keeps the existing runtime-only behavior.workerdprerender worker to transform static images, writes successful transforms to the client output, and falls back to Sharp only when a transform is not returned by the binding.Testing
cloudflare-bindingconfig opts into build-time transforms.Docs