-
Notifications
You must be signed in to change notification settings - Fork 270
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
Conversation
@danielroe OK, I've changed it to now detect at build time instead. |
const normalizableProviders: Partial<Record<string, () => ImageProviderName>> = { | ||
netlify: () => { | ||
return process.env.NETLIFY_LFS_ORIGIN_URL ? 'netlifyLargeMedia' : 'netlifyImageCdn' | ||
} | ||
} |
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.
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.
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.
Seems good to me, and it's internal so we can always refactor in future.
Docs for Netlify Image CDN implementation
✅ Live Preview ready!
|
Codecov ReportAttention:
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. |
@@ -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' |
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.
Thank you so much ❤️ |
Co-authored-by: Kristen Lavavej <35638702+klavavej@users.noreply.github.com>
Very nice change, but it would have been better if this had been marked as a breaking change. Netlify deployments now seem to use Context: my app is built using |
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 thenetlify
provider. To avoid breaking existing sites that use Large Media, I have done this by renaming the old provider tonetlifyLargeMedia
, creating a new provider callednetlifyImageCdn
, and then replacing thenetlify
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.