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

feat: support Netlify Image CDN #1234

Merged
merged 21 commits into from
Feb 15, 2024
Merged

Conversation

ascorbic
Copy link
Contributor

@ascorbic ascorbic commented Feb 3, 2024

In September lase year, Netlify deprecated Large Media, which is currently the basis of the netlify image provider. The DX was always awkward and it was never widely used. Subsequently we launched Netlify Image CDN, which is a new product that is available automatcially on all sites, and is much easier to use. This PR adds support for the new image CDN, and makes it the default for the netlify provider. To avoid breaking existing sites that use Large Media, I have done this by renaming the old provider to netlifyLargeMedia, creating a new provider called netlifyImageCdn, and then replacing the netlify provider with one that conditionally exports the other provider, depending on whether Large Media is detected on the site. Users can also choose one of the providers manually.

This is a draft to get feedback on my approach. If it looks good I'll update the tests and docs too. Because the image CDN is automatically enabled on all sites, I would like to follow up with a PR to auto-enable this provider, similarly to the Vercel provider.

@ascorbic ascorbic marked this pull request as ready for review February 7, 2024 14:30
@ascorbic
Copy link
Contributor Author

ascorbic commented Feb 8, 2024

@danielroe OK, I've changed it to now detect at build time instead.

Comment on lines +149 to 153
const normalizableProviders: Partial<Record<string, () => ImageProviderName>> = {
netlify: () => {
return process.env.NETLIFY_LFS_ORIGIN_URL ? 'netlifyLargeMedia' : 'netlifyImageCdn'
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added this as a new way for providers to resolve at build time. It alows an alias or detected platform to provide logic to resolve to one or other defined provider types.

Copy link
Member

Choose a reason for hiding this comment

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

Seems good to me, and it's internal so we can always refactor in future.

Copy link

nuxt-studio bot commented Feb 9, 2024

Live Preview ready!

Name Edit Preview Latest Commit
Image Edit on Studio ↗︎ View Live Preview ce84424

@codecov-commenter
Copy link

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (8701991) 68.20% compared to head (a5d9212) 68.62%.
Report is 9 commits behind head on main.

Files Patch % Lines
src/provider.ts 85.71% 2 Missing ⚠️
docs/pages/index.vue 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1234      +/-   ##
==========================================
+ Coverage   68.20%   68.62%   +0.42%     
==========================================
  Files          76       78       +2     
  Lines        4299     4386      +87     
  Branches      397      402       +5     
==========================================
+ Hits         2932     3010      +78     
- Misses       1339     1348       +9     
  Partials       28       28              

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

@@ -136,7 +142,14 @@ export async function resolveProvider (_nuxt: any, key: string, input: InputProv

const autodetectableProviders: Partial<Record<ProviderName, ImageProviderName>> = {
vercel: 'vercel',
aws_amplify: 'awsAmplify'
aws_amplify: 'awsAmplify',
netlify: 'netlify'
Copy link
Member

Choose a reason for hiding this comment

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

Adding a comment that we may change the logic for these providers in a subsequent PR. I'm discussing with @pi0 whether it's a breaking change to enable runtime support when users are performing fully static builds (e.g. nuxt generate).

context: #1224

@danielroe
Copy link
Member

Thank you so much ❤️

@danielroe danielroe merged commit a872822 into nuxt:main Feb 15, 2024
2 checks passed
riddla pushed a commit to tricks-gmbh/nuxt-image that referenced this pull request Mar 1, 2024
Co-authored-by: Kristen Lavavej <35638702+klavavej@users.noreply.github.com>
@github-actions github-actions bot mentioned this pull request Mar 5, 2024
@Anoesj
Copy link

Anoesj commented Mar 20, 2024

Very nice change, but it would have been better if this had been marked as a breaking change. Netlify deployments now seem to use netlify as a provider by default, which is a change and broke all images in an app of mine hosted on Netlify after deployment, because I was assuming IPX to be used by default at all times. Changing provider to ipx manually in nuxt.config.ts fixed the issue.

Context: my app is built using NITRO_PRESET=netlify-static nuxt build with ssr: false in nuxt.config.ts to generate a preview of the app as a plain Vue app. The images are all "passthrough", so there is no image optimization going on at all, not by IPX either.

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.

None yet

5 participants