fix: strip domain from ipx edge functions path (#2098) #2099
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for netlify-plugin-nextjs-nx-monorepo-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-static-root-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-export-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for nextjs-plugin-custom-routes-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-next-auth-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for next-plugin-canary ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for next-plugin-edge-middleware ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for next-i18next-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
packages/runtime/src/helpers/edge.ts
Outdated
| /** | ||
| * Convert the images path to strip the origin (until domain-level Edge functions are supported) | ||
| */ | ||
| const sanitizeEdgePath = (imagesPath: string) => new URL(imagesPath, process.env.URL || 'http://n').pathname |
There was a problem hiding this comment.
Nice using new URL. This fix looks good.
| manifest.functions.push({ | ||
| function: 'ipx', | ||
| name: 'next/image handler', | ||
| path: nextConfig.images.path || '/_next/image', | ||
| path: nextConfig.images.path ? sanitizeEdgePath(nextConfig.images.path) : '/_next/image', |
There was a problem hiding this comment.
It'd be good to either have a unit test checking that it gets sanitized when needed or we could update a demo site with a full images path in a Next config to validate the fix.
|
it's been awhile since @nickytonline 's review 2 weeks ago - but we have enterprise customers actively awaiting this to be shipped:
Can we wrap this up soon? |
|
@nickytonline I added a few unit tests here, can you have a look? thanks! |
LekoArts
approved these changes
Jun 12, 2023
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
When folks put in a full URL into the
images.pathconfiguration while using Edge functions for image format content negotiation, the Edge function never runs, as full URLs are not supported for Edge function paths (in Netlify).Strictly speaking in your
next.config.js:The PR changes the Edge function to capture
/_next/imageinstead of (previously erroring)https://example.com/_next/image. It does not affect the generated markup.See #2098 for more info.
Potential issues:
loaderother thandefault(the code assumes that opting into edge function images means they’re using thedefaultloader)Test plan
Would appreciate some additional guidance here! I did attempt to put together a sample repo but encountered some errors when attempting to
npm install github:netlify/next-runtime#zl/next-images-path-edge.Relevant links (GitHub issues, Notion docs, etc.) or a picture of cute animal
Fixes #2098
Standard checks:
🧪 Once merged, make sure to update the version if needed and that it was published correctly.