Skip to content
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

fix: use ipx for static builds instead of auto detect #1224

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pi0
Copy link
Member

@pi0 pi0 commented Jan 30, 2024

Context: We discovered it with @atinux investigating broken nuxt UI docs since it was using vertical image optimization and not picked from.. Another story probably to solve for another day...


When building Nuxt in static mode (nuxt generate) we assume everything is at build-time. Auto detected providers (vercel, amplify) only benefit for production deploys with a server running where as for a static deploy, we can save (dynamic optimization) costs and reduce issue surface by optimizing at build time.

This PR improves detectProvider internal utility to preserve auto-detected status and later use in nitro hook to switch back to ipx-static if needed (so user input is still respected)

@pi0 pi0 requested review from atinux and danielroe January 30, 2024 12:21
Copy link

cloudflare-workers-and-pages bot commented Jan 30, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 14ee03f
Status: ✅  Deploy successful!
Preview URL: https://3ca02dd9.nuxt-image.pages.dev
Branch Preview URL: https://fix-auto-static.nuxt-image.pages.dev

View logs

@codecov-commenter
Copy link

codecov-commenter commented Jan 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8701991) 68.20% compared to head (14ee03f) 68.22%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1224      +/-   ##
==========================================
+ Coverage   68.20%   68.22%   +0.01%     
==========================================
  Files          76       76              
  Lines        4299     4308       +9     
  Branches      397      398       +1     
==========================================
+ Hits         2932     2939       +7     
- Misses       1339     1340       +1     
- Partials       28       29       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

danielroe
danielroe previously approved these changes Feb 1, 2024
@danielroe danielroe self-requested a review February 1, 2024 12:08
@danielroe
Copy link
Member

Actually, thinking about this I'm not 100% sure this is the right choice. Even with statically generated builds, we don't know that all images will be optimised at build time. (For example, they may be loading them in from an API.)

I think it's safer to have users opt-in to this behaviour.

@pi0
Copy link
Member Author

pi0 commented Feb 1, 2024

Vercel default config does not allow external URLs by default and we can detect this at build time using config if we want to allow this also automatically.

Build-time cost, is always less than on demand and we can use build-time caching and for staticly build-websites by default, we optimize only build-time images (again unless configured to allow external domain)

@danielroe
Copy link
Member

The point is that we can optimise all images that are known at generate time, and for plenty of websites that is perfect 👌

But for websites that have any kind of dynamic image which might rely on the current behaviour, this would be a breaking change. That's why I'm suggesting it should be opt-in.

@pi0 pi0 marked this pull request as draft February 1, 2024 15:08
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.

4 participants