feat(v6): update cloudflare default image service#13267
feat(v6): update cloudflare default image service#13267Princesseuh wants to merge 12 commits intov6from
Conversation
✅ Deploy Preview for astro-docs-2 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
|
Just noting that this will need an entry in the Cloudflare upgrade guide here too, where the change is clearly noted and with any upgrade guidance! https://v6.docs.astro.build/en/guides/integrations-guide/cloudflare/#upgrading-to-v13-and-astro-6 Normally when a default setting has changed, we say things like "inspect your deployed project for anything funky" in case this affects them. And if possible, we like to give advice on what to do to "revert to the previous behavior" (since presumably that was already working), which in this case seems to be explicitly setting Edited to add: Will note that this won't go in the main v6 upgrade guide since it's just for Cloudflare, but you can see what other "Changed default" entries typically look like there for inspiration! https://v6.docs.astro.build/en/guides/upgrade-to/v6/#changed-defaults |
| - **`cloudflare-binding`:** Uses the [Cloudflare Images binding](https://developers.cloudflare.com/images/transform-images/bindings/) for image transformation. The binding is automatically provisioned when you deploy. | ||
| - **`passthrough`:** Uses the existing [`noop`](/en/guides/images/#configure-no-op-passthrough-service) service. | ||
| - **`compile`:** Uses Astro's default service (Sharp), but only on pre-rendered routes at build time. For pages rendered on demand, all `astro:assets` features are disabled. | ||
| - **`compile`:** Transforms images at build time for pre-rendered routes. Uses passthrough for on-demand rendered pages. |
There was a problem hiding this comment.
This should still say what it uses, like all the other options, since this option is to define which image service is being used.
And, is the previous sentence now incorrect, that all astro:assets features are disabled for pages rendered on demand? That seemed a very clear way of stating it, so if it hasn't changed, I'm wondering why the wording has? Or do you want to emphasize the passthrough runtime behaviour? If so, then I'd write passthrough in code, something like:
| - **`compile`:** Transforms images at build time for pre-rendered routes. Uses passthrough for on-demand rendered pages. | |
| - **`compile`:** Uses XXXXXXX to transform images at build time for prerendered routes. The noop `passthrough` option is configured for on-demand rendered pages. |
(Also, note that we're standardizing docs around prerendered (no hyphen) as we get around to them, so might as well update that here!)
There was a problem hiding this comment.
The reason it just says locally instead of explaining exactly what it uses to transform is because its using sharp but through miniflare so its not exactly the same as it was before when we just said it uses sharp
There was a problem hiding this comment.
But, it's still using something, right? And, I don't see the above saying "local" anywhere (sorry, can see the text was updated which isn't visible looking at this conversation view!), which certainly could be incorporated for more explanation! If this is about configuring which image service is used, I do think the option should be described in terms of what (service) is or isn't being used, something along the lines of:
"Uses a combination of internal dependencies to transform images locally at build time for prerendered pages"
Co-authored-by: Sarah Rainsberger <5098874+sarah11918@users.noreply.github.com>
Co-authored-by: Sarah Rainsberger <5098874+sarah11918@users.noreply.github.com>
Co-authored-by: Sarah Rainsberger <5098874+sarah11918@users.noreply.github.com>
| <p> | ||
| **Type:** `'passthrough' | 'cloudflare' | 'cloudflare-binding' | 'compile' | 'custom'`<br /> | ||
| **Default:** `'compile'` | ||
| **Type:** `'passthrough' | 'cloudflare' | 'cloudflare-binding' | 'compile' | 'custom' | { build: 'compile', runtime?: 'cloudflare-binding' | 'passthrough' }`<br /> |
There was a problem hiding this comment.
| **Type:** `'passthrough' | 'cloudflare' | 'cloudflare-binding' | 'compile' | 'custom' | { build: 'compile', runtime?: 'cloudflare-binding' | 'passthrough' }`<br /> | |
| **Type:** `'passthrough' | 'cloudflare' | 'cloudflare-binding' | 'compile' | 'custom' | 'compile-and-run'`<br /> |
So, I woke up this morning thinking, "If there really is only one reasonable configuration option for this thing, can we just give it a simple name (while using the implementation under the hood)?
This is a dummy name, but wouldn't it make sense to just do that?
There was a problem hiding this comment.
The original idea was it opens the door to more in future, and acts as an invitation to anyone who wants to add more like custom to runtime
There was a problem hiding this comment.
In that scenario, wouldn't you want the type to be something like ImageService | { build: ImageService, runtime: ImageService } rather than only allowing one?
There was a problem hiding this comment.
I'll not resolve this thread in case there's any response to Matt's comment here, but noting for the record that I do not intend for my suggestion here to be taken!
There was a problem hiding this comment.
Yes probably the ideal is { build: ImageService, dev: ImageService, runtime: ImageService }
| }), | ||
| }); | ||
| ``` | ||
|
|
There was a problem hiding this comment.
We're missing closing ``` here!
| }); | ||
| ``` | ||
|
|
||
| ### `sessionKVBindingName` |
There was a problem hiding this comment.
OK, the link checker is claiming this section doesn't exist, but clearly it does... it might be confused by the missing/open ``` so I'll try to add them in myself
Co-authored-by: Yan <61414485+yanthomasdev@users.noreply.github.com>
sarah11918
left a comment
There was a problem hiding this comment.
Alright, I think we got there! Thanks for everyone's patience and help!
Description (required)
withastro/astro#15435 changes the default to use the bindings instead of compile because compile's dev behavior is not intuitive anymore (yet)