fix(vite): support concurrent dev servers sharing a plugin instance - #4496
fix(vite): support concurrent dev servers sharing a plugin instance#4496danielroe wants to merge 2 commits into
Conversation
|
@danielroe is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesVite runner lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
commit: |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/build/vite/env.ts (1)
131-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInternal helper placement / file size.
_createEnvRunneris unexported but is placed between exportedreleaseEnvRunnerand exportedgetEnvRunner. As per coding guidelines, "Place non-exported/internal helpers at the end of the file." Given this block adds ~90 lines of runner-lifecycle logic to an already large file, also consider splitting it into its own module per "Split logic across files; avoid long single-file modules (>200 LoC). Use_*prefix for internal files."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/build/vite/env.ts` around lines 131 - 217, The internal helper _createEnvRunner is located between exported functions and contributes to an oversized module. Move _createEnvRunner and its runner-lifecycle logic to the end of the file, or extract it into a dedicated internal _*-prefixed module, then update references from initEnvRunner and acquireEnvRunner while preserving their existing behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/build/vite/env.ts`:
- Around line 185-195: Handle the promise returned by _loadRunner in the
manager.onClose retry branch by awaiting it where supported or attaching a
rejection handler that logs the reload failure through ctx.nitro!.logger.
Preserve the existing retry limit and successful restart flow.
- Around line 150-180: Update acquireEnvRunner so a rejection of the newly
created entry.promise removes that entry from ctx._serverEnvRunners before
propagating the error. Ensure cleanup only removes the same cached entry,
preserving newer entries if one was created concurrently, and leave successful
reference-counting and releaseEnvRunner behavior unchanged.
In `@test/unit/vite-concurrent-servers.test.ts`:
- Around line 32-35: Update the concurrent server creation at
test/unit/vite-concurrent-servers.test.ts lines 32-35 to capture successfully
initialized servers, close all captured servers in a catch before rethrowing,
and preserve the existing failure assertion. Apply the same failure-safe
creation helper at lines 50-53 so partial initialization cannot leak servers.
- Around line 23-25: Update the `hello` function to validate that
`server.environments.nitro` exists before casting or calling `dispatchFetch`;
throw an explicit error identifying the missing Nitro environment, then preserve
the existing fetch flow when it is available.
---
Nitpick comments:
In `@src/build/vite/env.ts`:
- Around line 131-217: The internal helper _createEnvRunner is located between
exported functions and contributes to an oversized module. Move _createEnvRunner
and its runner-lifecycle logic to the end of the file, or extract it into a
dedicated internal _*-prefixed module, then update references from initEnvRunner
and acquireEnvRunner while preserving their existing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 50e3d532-bbe2-4fa9-981b-696db52307ab
📒 Files selected for processing (5)
src/build/vite/dev.tssrc/build/vite/env.tssrc/build/vite/plugin.tssrc/build/vite/types.tstest/unit/vite-concurrent-servers.test.ts
🔗 Linked issue
❓ Type of change
📚 Description
I've hit this a couple of times (first when adopting nitro/vite and now in nuxt/nuxt#35891 where I'm making it the default option) when trying to run multiple vitest projects (which share a vite config, and therefore share a nitro plugin):
a minimal repro is two servers sharing one
nitro()plugin instance:there are two separate issues:
the second server skips setup entirely (because
ctx._initializedis set, so it tries to create an environment before the first server has finishedboth servers share a runner, so closing one server throws
503 Runner is unavailablefor the othersimply awaiting
initEnvRunnerfixes the first one but not the secondlet me know if you have any better ideas on how to fix it 🙏
📝 Checklist