Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.4k
refactor(nuxt): move custom impl of vite warmup to vite config#33390
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 ourterms of service andprivacy 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?
Conversation
|
does it work? when I last checked it did not correctly warm up the server |
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/schema/src/config/vite.ts (1)
122-137:Consider extracting duplicate resolver logic.The resolver logic is identical for both
ssrFilesandclientFiles. Whilst this duplication is minor, extracting it to a helper function would improve maintainability.Example refactor:
constresolveWarmupFiles=async(val:any,get:(key:string)=>Promise<any>)=>{if(val){returnval}return(awaitget('vite.warmupEntry')) ?[awaitget('vite.entry')] :[]}// Then use:warmup:{ssrFiles:{$resolve:resolveWarmupFiles,},clientFiles:{$resolve:resolveWarmupFiles,},},packages/vite/src/vite.ts (1)
154-158:server.warmup hardcodes override schema defaults
- Explicit ssrFiles/clientFiles in
packages/vite/src/vite.ts(lines 154–158) bypass the schema resolver inpackages/schema/src/config/vite.ts(lines 127/135), which honoursnuxt.options.vite.warmupEntry.- Use a conditional based on
nuxt.options.vite.warmupEntryor remove the explicit arrays to defer to the schema’s logic.- Warmup timing logs were dropped—consider re-adding observability via a Vite plugin or hook.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/schema/src/config/vite.ts(1 hunks)packages/vite/src/utils/warmup.ts(0 hunks)packages/vite/src/vite.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/vite/src/utils/warmup.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/vite/src/vite.tspackages/schema/src/config/vite.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: code
🔇 Additional comments (1)
packages/schema/src/config/vite.ts (1)
127-127:No action needed:vite.warmupEntryis defined in packages/schema/src/types/config.ts (line 114) as an optional boolean.
@danielroe It seems so. the warmup request do get triggered through How would you verify it ? |
I test it by measuring the time after the dev server is ready and timing the initial browser load of
ideally you should see a significant difference between 1 & 2, and no difference between 2 & 3 (or a speedup). |
Well I see no differences between no warmup, this PR and the current implementation using nuxt.com |
| allow:[...newSet(allowDirs)], | ||
| }, | ||
| warmup:{ | ||
| ssrFiles:[entry], |
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.
we already do this:
nuxt/packages/vite/src/server.ts
Lines 114 to 116 in1918764
warmup:{ ssrFiles:[serverEntry], }, nuxt/packages/vite/src/client.ts
Lines 141 to 143 in926782d
warmup:{ clientFiles:[ctx.entry], },
see#27963
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.
yes but I removed it when testing it. I currently have some issues with WSL, will try again later
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 mean, I'm not sure this PR changes anything as we already use the vite native warmup (we just set it in a different file)
🔗 Linked issue
📚 Description
This PR moves our vite warmup implementation to
vite.server.warmup. There's a catch though, we lose logs about how long it took to warmup.