-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
chore(open-next): apply various improvements #8304
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
base: main
Are you sure you want to change the base?
chore(open-next): apply various improvements #8304
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@vicb it would be great if you could have a look and validate the open-next changes if you get a chance 🙂 |
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.
Pull Request Overview
This PR upgrades the @opennextjs/cloudflare package from version 1.6.4 to 1.11.0 and implements enhanced caching and image optimization features for the Cloudflare deployment.
Key changes:
- Upgraded
@opennextjs/cloudflareto version 1.11.0, which includes updated dependencies like@opennextjs/awsand new packages (rclone.js,@types/rclone.js,adm-zip) - Migrated from KV-based caching to R2-based incremental caching with regional cache support and Durable Objects queue
- Added custom Cloudflare image loader to leverage Cloudflare's image transformation service
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Updated package versions including @opennextjs/cloudflare (1.6.4→1.11.0), @opennextjs/aws (3.7.4→3.8.5), and added new dependencies (rclone.js, @types/rclone.js, adm-zip) |
| apps/site/package.json | Upgraded @opennextjs/cloudflare dependency specification to ^1.11.0 |
| apps/site/wrangler.jsonc | Replaced KV namespace with R2 bucket configuration and added Durable Objects bindings for queue handling |
| apps/site/open-next.config.ts | Updated configuration to use R2-based incremental cache with regional caching, DO queue, and cache interception |
| apps/site/next.config.mjs | Added conditional image loader configuration to use Cloudflare's custom image loader when deploying to Cloudflare |
| apps/site/cloudflare-image-loader.ts | New file implementing custom image loader for Cloudflare's image transformation service |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8304 +/- ##
==========================================
+ Coverage 76.72% 76.76% +0.03%
==========================================
Files 118 118
Lines 9805 9816 +11
Branches 335 335
==========================================
+ Hits 7523 7535 +12
+ Misses 2280 2279 -1
Partials 2 2 ☔ View full report in Codecov by Sentry. |
93cdecb to
28ccc13
Compare
|
Lighthouse Results
|
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.
Can this go in a cloudflare/ subdirectory (cloudflare/images.ts or something)?
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.
Part of me really does not like this function. something about how it's designed 😅
I bert this can be simplified/cleaned up. + I'd rather not use the frigging process.env.NODE_ENV here and actually use the constants we have defined on next.constants. Please also document what this function is doing, why it works and Cloudflare external links. We need to know this exists and howit works.
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.
This function is taken straight from the open-next documentation: https://opennext.js.org/cloudflare/howtos/image#use-a-custom-loader
I think it might be beneficial (since there are no like custom requirements or anything like that) to keep it as close as possible as the version in the open-next docs? That way if anything on the open-next site changes an updated loader version can just be copy-pasted here instead or having a more bespoke version that we need to more directly maintain? 🤔
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.
Can this go in a cloudflare/ subdirectory (cloudflare/images.ts or something)?
Moved 👍
| import type { ImageLoaderProps } from 'next/image'; | ||
|
|
||
| const normalizeSrc = (src: string) => { | ||
| return src.startsWith('/') ? src.slice(1) : src; | ||
| }; | ||
|
|
||
| export default function cloudflareLoader({ | ||
| src, | ||
| width, | ||
| quality, | ||
| }: ImageLoaderProps) { | ||
| if (process.env.NODE_ENV === 'development') { | ||
| // Serve the original image when using `next dev` | ||
| return src; | ||
| } | ||
| const params = [`width=${width}`]; | ||
| if (quality) { | ||
| params.push(`quality=${quality}`); | ||
| } | ||
| const paramsString = params.join(','); | ||
| return `/cdn-cgi/image/${paramsString}/${normalizeSrc(src)}`; | ||
| } |
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.
A few notes with this implementation:
-
is
normalizeSrcneeded? Won't the leading/be normalized by the browser? If not, shouldn't there not be a slash / always be a slash passed here? This should only load images, inconsistencies should be resolved at their root, not here? -
I think this can be simplified to:
| import type { ImageLoaderProps } from 'next/image'; | |
| const normalizeSrc = (src: string) => { | |
| return src.startsWith('/') ? src.slice(1) : src; | |
| }; | |
| export default function cloudflareLoader({ | |
| src, | |
| width, | |
| quality, | |
| }: ImageLoaderProps) { | |
| if (process.env.NODE_ENV === 'development') { | |
| // Serve the original image when using `next dev` | |
| return src; | |
| } | |
| const params = [`width=${width}`]; | |
| if (quality) { | |
| params.push(`quality=${quality}`); | |
| } | |
| const paramsString = params.join(','); | |
| return `/cdn-cgi/image/${paramsString}/${normalizeSrc(src)}`; | |
| } | |
| import type { ImageLoaderProps } from 'next/image'; | |
| // This can also be a `.replace(...)`? | |
| const normalizeSrc = (src: string) => src.startsWith('/') ? src : `/${src}`; | |
| // For tree-shaking, do this outside | |
| export default process.env.NODE_ENV === 'development' ? | |
| ({ src }) => src : | |
| ({ | |
| src, | |
| width, | |
| quality, | |
| }: ImageLoaderProps) => `/cdn-cgi/image/width=${width}${quality ? `quality=${quality}` : ''}${normalizeSrc(src)}`; |
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'm happy to go with your version, however as I mentioned there might be benefits in keeping this in sync with the open-next documentation? 🤔
(PS: if you want you can also try to upstream these changes in the open-next docs (cc. @vicb))
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.
Imo, the initial implementation is a lot more readable than this (though normalizeSrc may be better as a just a .replace but I'm either or on it)
apps/site/next.config.mjs
Outdated
| }, | ||
| ], | ||
| }, | ||
| images: |
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.
As a follow-up, I think we should make an apps/site/vercel folder and apps/site/cloudflare folder (well, it'll already exist, see my other comment) for the specific logic. Then, the config file itself will be simple, and call things like getCloudflareImageConfiguration() and getVercelImageConfiguration() or something?
wdyt @nodejs/nodejs-website
apps/site/next.config.mjs
Outdated
| // adapter should set it itself when it invokes the Next.js build | ||
| // process, once it does that remove the manual `OPEN_NEXT_CLOUDFLARE` | ||
| // definition in the package.json script. | ||
| process.env.OPEN_NEXT_CLOUDFLARE |
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.
Can we not use direct process.env? Thanks! We use next.constants.mjs for all relevant constants.
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.
Also both these could be extracted into their own constants instead of a big ternary, easier to read.
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.
Can we not use direct process.env? Thanks! We use next.constants.mjs for all relevant constants.
Done 👍
Also both these could be extracted into their own constants instead of a big ternary, easier to read.
I hope this is what you had in mind: 302ba1b
|
|
||
| const cloudflareConfig = defineCloudflareConfig({ incrementalCache }); | ||
| const cloudflareConfig = defineCloudflareConfig({ | ||
| incrementalCache: withRegionalCache(r2IncrementalCache, { |
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.
Can you add inline comments reference @see to relevant docs? And also inline comment what these settings do? 🙇
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.
The PR description mentions
replaced the kv incremental cache with the more performance r2 + regional cache implementation
The actual issue with KV is that it is eventually consistent which might lead to unexpected behavior. This is why we recommend R2 instead - R2 is strongly consistent.
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 think you misunderstood, I'm asking for comments to be added on the code, so that it stays there for reference :)
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 got that, I added the comment mostly for Dario to update the PR description and add comments here
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.
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.
Changes look good to me when comments are addressed, thanks Dario!
A few notes:
- Cloudflare images need to be enabled/configured for the account
- R2 batch upload had been implemented recently to speed up cache asset upload. There are a few environment variables to set for this to work. I'll write the doc for it this week. Before that R2 upload could be slow.
|
Thanks Victor for the updates. Will keep an eye on your docs. Also, where can we enabled Cloudflare Images? |
https://developers.cloudflare.com/images/get-started/#enable-transformations-on-your-zone I already did that on the testing/staging account |
opennextjs/docs#189 about R2 batching was just merged to the docs. The 3 environments variables should be added the build time time env vars (provided you use Cloudflare CD). |
28ccc13 to
65b4b4b
Compare
| R2_ACCESS_KEY_ID: ${{ env.R2_ACCESS_KEY_ID }} | ||
| R2_SECRET_ACCESS_KEY: ${{ secrets.R2_SECRET_ACCESS_KEY }} |
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 these two to the repository, I've created a token specific for the account that only has access to the site's incremental cache r2 bucket, I hope this works for you, if it doesn't I'm happy to change things 🙂
Added 🫡 |
Description
This PR adds various open-next improvements to the site (mainly for better performance)
The changes are:
@opennextjs/cloudflareto the latest version (1.11.0)Validation
I've validated these changes locally by running
pnpm cloudflare:previewCheck List
pnpm formatto ensure the code follows the style guide.pnpm testto check if all tests are passing.pnpm buildto check if the website builds without errors.