-
Notifications
You must be signed in to change notification settings - Fork 130
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: images support via /_next/image
#357
feat: images support via /_next/image
#357
Conversation
🦋 Changeset detectedLatest commit: d1aa353 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
🧪 Prereleases are available for testing 🧪 @cloudflare/next-on-pagesYou can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/next-on-pages/runs/5446881342/npm-package-next-on-pages-357 @cloudflare/eslint-plugin-next-on-pagesYou can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/next-on-pages/runs/5446881342/npm-package-eslint-plugin-next-on-pages-357 |
63b1d0b
to
605f67f
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.
Awesome stuff, thanks a lot for the changes 😃
// NOTE: Pages also doesn't seem to support calling the `/cdn-cgi/image` endpoint either. | ||
// const cdnCgiImageUrl = buildCdnCgiImageUrl(requestUrl, imageUrl, options); | ||
// const cdnCgiResp = await fetch(cdnCgiImageUrl, { headers: request.headers }); | ||
// if (cdnCgiResp.status === 200) return formatResp(cdnCgiResp, imageUrl, config); |
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 had a chat with @GregBrimble and if I understood correctly it doesn't seem like this is going to be supported in the future
so I think maybe we should drop this entirely (alongside buildCdnCgiImageUrl
) as it is never going to be applied anyways
Greg?
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 gone ahead and removed it for now. tbh, I wouldn't really be a fan of falling back to that endpoint even if it were possible.
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.
Awesome! thanks a lot! awesome fix for the Image component 😄
* feat: images support via `/_next/image` * undo deps change * cant spell * Update packages/next-on-pages/templates/_worker.js/utils/images.ts Co-authored-by: Dario Piotrowicz <[email protected]> * apply suggestions * comment about default quality * remove code for `/cdn-cgi/image` --------- Co-authored-by: Dario Piotrowicz <[email protected]>
I think something new is breaking that, I have a next: 13.5.4 site running in Cloudflare, but I have no idea why my images aren't loading anymore. I noticed that the demo https://image-resizing-test.pages.dev/ also has some images that don't load correctly. Something I noticed is an error message when trying to load the image url directly.
|
The images on that demo load correctly - the third one is to a domain that wasn't allowed in the next.config.js, while the domain for the second one was allowed in the next.config.js. Can you please check that the domain you are trying to load images for was allowed in your next.config.js? |
This PR does the following:
/_next/image
endpoint.At the moment, it is not possible to use image resizing from within Cloudflare Pages 🙁. Therefore, this PR currently just falls back to the actual image URL after processing the request. The code to send the request to image resizing is commented out but can be easily enabled by uncommenting it once image resizing support is in Pages.
I think it probably makes sense to merge this PR with it just falling back to the image URL. In the future, once image resizing is added, we can open another PR to uncomment the relevant parts and add e2e tests for them. It may take months for image resizing support to be added though, so just falling back will have to do in the meantime.
Note that even though there is no image resizing in Pages yet, we should still go through all the image resizing steps so that requests are handled properly.
https://image-resizing-test.pages.dev/
fixes #94