-
-
Notifications
You must be signed in to change notification settings - Fork 45
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
feat(cloudflare): support "compile-time" only image config for astro:assets #58
feat(cloudflare): support "compile-time" only image config for astro:assets #58
Conversation
🦋 Changeset detectedLatest commit: 7050491 The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 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 |
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @alexanderniebuhr and the rest of your teammates on Graphite |
!preview cf-assets-compile |
Snapshots have been released for the following packages:
Publish Log
Build Log
|
ddd6880
to
00f577f
Compare
!preview cf-assets-compile |
Snapshots have been released for the following packages:
Publish Log
Build Log
|
00f577f
to
c9ceb14
Compare
!preview cf-assets-compile |
Snapshots have been released for the following packages:
Publish Log
Build Log
|
15394fe
to
1118d2e
Compare
!preview cf-assets-compile |
Snapshots have been released for the following packages:
Publish Log
Build Log
|
1118d2e
to
5eaf9c8
Compare
!preview cf-assets-compile |
Snapshots have been released for the following packages:
Publish Log
Build Log
|
58cc3f3
to
99cf387
Compare
5eaf9c8
to
86909f9
Compare
99cf387
to
4587fea
Compare
86909f9
to
3ef4af5
Compare
Hi! @alexanderniebuhr, I am developing a website (is SSR hybrid, basically all is static except an API) that need this use of case so if you need I can test the packages. For know I am install @astrojs/cloudflare@experimental--cf-assets-compile and I am looking how can I activate the "compile-time" optimization. EDIT: I saw that it the "compile" option and I am testing it. ▶ /_astro/me.ee1ae41e_ZcCgWD.webp (reused cache entry) (+1ms) (1/31)
▶ /_astro/50241495487_e05f382b64_o_1hx8eF.webp (reused cache entry) (+1.12s) (2/31)
▶ /_astro/50241495487_e05f382b64_o_Z2duqmk.webp (reused cache entry) (+1.11s) (3/31)
▶ /_astro/50241495487_e05f382b64_o_1QWdIk.webp (reused cache entry) (+1.11s) (4/31)
▶ /_astro/50241495487_e05f382b64_o_1BJQAq.png (reused cache entry) (+1.13s) (5/31)
▶ /_astro/50241495487_e05f382b64_o_1hktIV.png (reused cache entry) (+1.05s) (6/31)
▶ /_astro/50241495487_e05f382b64_o_hA00E.png (reused cache entry) (+750ms) (7/31)
▶ /_astro/42003419460_f9109b0fba_o_Z128QwJ.webp (reused cache entry) (+682ms) (8/31)
▶ /_astro/42003419460_f9109b0fba_o_Z2i4Hkw.webp (reused cache entry) (+680ms) (9/31)
▶ /_astro/42003419460_f9109b0fba_o_1KMJx0.webp (reused cache entry) (+674ms) (10/31)
▶ /_astro/42003419460_f9109b0fba_o_Z2jRW5I.png (reused cache entry) (+684ms) (11/31)
▶ /_astro/42003419460_f9109b0fba_o_1MkzM2.png (reused cache entry) (+629ms) (12/31)
▶ /_astro/42003419460_f9109b0fba_o_L0SPC.png (reused cache entry) (+417ms) (13/31)
▶ /_astro/52298257522_fe30274bb7_o_Z1V7LNy.webp (reused cache entry) (+0.99s) (14/31)
▶ /_astro/52298257522_fe30274bb7_o_ZlXcAC.webp (reused cache entry) (+0.98s) (15/31)
▶ /_astro/52298257522_fe30274bb7_o_Z1lHGjT.webp (reused cache entry) (+0.96s) (16/31)
▶ /_astro/52945559280_54640422ce_o_ZGKrG2.webp (reused cache entry) (+1.93s) (17/31)
▶ /_astro/52945559280_54640422ce_o_HTWcN.webp (reused cache entry) (+1.96s) (18/31)
▶ /_astro/53175451734_805a2501ae_o_1eA9N8.webp (reused cache entry) (+0.84s) (19/31)
▶ /_astro/53175451734_805a2501ae_o_Z2groMR.webp (reused cache entry) (+0.84s) (20/31)
▶ /_astro/53175451734_805a2501ae_o_1O0fhM.webp (reused cache entry) (+0.83s) (21/31)
▶ /_astro/52945559280_54640422ce_o_Z4keik.webp (reused cache entry) (+1.95s) (22/31)
▶ /_astro/52298257522_fe30274bb7_o_Z1AU3rN.png (reused cache entry) (+2.95s) (23/31)
▶ /_astro/53175451734_805a2501ae_o_1yMS9S.png (reused cache entry) (+0.87s) (24/31)
▶ /_astro/52945559280_54640422ce_o_ZmxIkh.png (reused cache entry) (+2.07s) (25/31)
▶ /_astro/53175451734_805a2501ae_o_1envio.png (reused cache entry) (+0.87s) (26/31)
▶ /_astro/52298257522_fe30274bb7_o_Z1Vkqji.png (reused cache entry) (+2.96s) (27/31)
▶ /_astro/52945559280_54640422ce_o_ZQrguR.png (reused cache entry) (+2.03s) (28/31)
▶ /_astro/53175451734_805a2501ae_o_eD1z7.png (reused cache entry) (+0.89s) (29/31)
▶ /_astro/52298257522_fe30274bb7_o_297dLm.png (reused cache entry) (+2.91s) (30/31)
▶ /_astro/52945559280_54640422ce_o_Z1DGs10.png (reused cache entry) (+1.82s) (31/31) EDIT 3: I think all work well in cloudflare, but I couldn't deploy because I exceed the 26MB limit for free tier rodrigotomees.932bdc84-0961-41ea-aa00-acddbd6e77a2.log EDIT 4: Compiled and deployed successfully using Image instead of Picture The deployment is here: rodrigotome.es |
@RodrigoTomeES thank you for testing this. I'll just have to add a test case for this PR and it is ready to merge. Probably later this week. :re the fallback format for Picture component, I remember that it should been fixed in this PR. Maybe double check and if not open a new Issue in the main repo with your findings :) |
I have an Astro site hosted on Cloudflare Pages, and would like to enable SSR for certain pages. However, once I switch to the Cloudflare adapter, all image optimizations stop working, even for prerendered routes. Am I reading correctly that this PR will fix this, so that build-time image optimizations will work? If so, is there any news on this? I'm happy to help with testing here |
@mikkelsvartveit That's correct. This should work with |
583e8b6
to
d30a95e
Compare
!preview cf-assets-compile |
Snapshots have been released for the following packages:
Publish Log
Build Log
|
Yes, this is still blocked. The issue is quite complicated, it's caused by some very precise code that was added to workaround another bundling issue. It's a bug fix that cause another bug, the worse kind of bug fix. If anyone want to check it out, the top top upstream issue causing this, is this one: rollup/rollup#4708, which we worked around by doing this: https://github.com/withastro/astro/blob/218ea0781b362e256f4a6c0b688c6757f88b6461/packages/astro/src/assets/vite-plugin-assets.ts#L38C1-L50, but it causes, well, the issue this PR is encountering. Essentially, the image service doesn't get DCE'd because of the manual chunking option. The ultimate fix would probably have been to remove the TLA in Astro 4.0, but unfortunately I was so busy with the dev toolbar and other stuff that I forgot about it. (It's a breaking change, unfortunately. A sync function would become async.) |
4160ec2
to
8d8ed84
Compare
@@ -22,7 +22,7 @@ export type { DirectoryRuntime } from './entrypoints/server.directory.js'; | |||
export type Options = { | |||
mode?: 'directory' | 'advanced'; | |||
functionPerRoute?: boolean; | |||
imageService?: 'passthrough' | 'cloudflare'; | |||
imageService?: 'passthrough' | 'cloudflare' | 'compile'; |
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 needs a docs pr
!preview cf-assets-compile |
Snapshots have been released for the following packages:
Publish Log
Build Log
|
@RodrigoTomeES @mikkelsvartveit If you would like to help, this is ready to test: I'll write a docs pr tomorrow and most likely merge this feature Monday if we don't find any issue with the workaround. |
8d8ed84
to
e43e6c5
Compare
Very exciting PR |
e43e6c5
to
e36b60c
Compare
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.
Only asking you to verify how the warning will actually render. Please test/check the GitHub syntax for that, since it's not usually an inline thing.
But, the content is fine!
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.
Only one small nit!
e36b60c
to
7050491
Compare
!preview cf-assets-compile |
Snapshots have been released for the following packages:
Publish Log
Build Log
|
Fantastic work, thanks guys :) |
Current state:
Given the explanation and looking at which packageManagers work, it has to be some kind of
transitive dependency
resolving issue, but I'm very confused why that would break bundlingChanges
astro:assets
image service bundle regression astro#9059