fix(dev): use runtime-agnostic websocket adapter for dev runner#4376
Open
productdevbook wants to merge 1 commit into
Open
fix(dev): use runtime-agnostic websocket adapter for dev runner#4376productdevbook wants to merge 1 commit into
productdevbook wants to merge 1 commit into
Conversation
The dev runtime entry hardcoded `crossws/adapters/node`, which throws `[crossws] Using Node.js adapter in an incompatible environment` when the dev worker runs under Bun (e.g. `bun --bun vite dev`). Expose the websocket hooks via the `websocket` AppEntry field so env-runner attaches the correct crossws adapter per runtime (node/bun/deno) through `crossws/server`, instead of selecting the Node.js adapter unconditionally. Closes nitrojs#3939 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@productdevbook is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
📝 WalkthroughWalkthroughThe Nitro dev entry ( ChangesNitro Dev WebSocket Refactor and Tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/presets/_nitro/runtime/nitro-dev.ts`:
- Around line 24-25: The multi-line comments on lines 24-25 in the nitro-dev.ts
file that explain the crossws adapter behavior are restating what the code does
inline, which violates the repo's style guidelines for src/**/*.{ts,js} files
that specify not to add comments explaining what lines do unless explicitly
prompted. Remove these comment lines entirely to align with the coding
standards.
In `@test/presets/nitro-dev.test.ts`:
- Around line 30-42: The websocket is not being closed in the failure paths when
a timeout or error occurs. In the setTimeout callback that rejects with
"websocket timeout" and in the ws.addEventListener("error") handler, ensure you
call ws.close() before calling reject. This guarantees that the socket is
properly cleaned up in both error and timeout scenarios, preventing dangling
connections and test flakiness.
🪄 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
Run ID: c857a932-4f46-495d-b375-6351cf973c41
📒 Files selected for processing (4)
src/presets/_nitro/runtime/nitro-dev.tstest/fixture/nitro.config.tstest/fixture/server/routes/ws.tstest/presets/nitro-dev.test.ts
Comment on lines
+24
to
+25
| // Let the dev runner attach the runtime-appropriate crossws adapter | ||
| // (node/bun/deno) via `crossws/server` instead of hardcoding the Node.js one. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove line-explaining comments in runtime entry.
These comments restate behavior inline and should be removed to align with repo style for src/**/*.{ts,js}.
As per coding guidelines, src/**/*.{ts,js} says: "Do not add comments explaining what the line does unless prompted."
🤖 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/presets/_nitro/runtime/nitro-dev.ts` around lines 24 - 25, The multi-line
comments on lines 24-25 in the nitro-dev.ts file that explain the crossws
adapter behavior are restating what the code does inline, which violates the
repo's style guidelines for src/**/*.{ts,js} files that specify not to add
comments explaining what lines do unless explicitly prompted. Remove these
comment lines entirely to align with the coding standards.
Source: Coding guidelines
Comment on lines
+30
to
+42
| await new Promise<void>((resolve, reject) => { | ||
| const timer = setTimeout(() => reject(new Error("websocket timeout")), 5000); | ||
| ws.addEventListener("error", (error) => reject(error as any)); | ||
| ws.addEventListener("message", (event) => { | ||
| messages.push(String(event.data)); | ||
| if (messages.length === 2) { | ||
| clearTimeout(timer); | ||
| ws.close(); | ||
| resolve(); | ||
| } | ||
| }); | ||
| ws.addEventListener("open", () => ws.send("ping")); | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Close socket on timeout/error to avoid dangling test resources.
Failure paths currently reject without guaranteed socket cleanup, which can leave open handles and make the suite flaky.
Suggested fix
await new Promise<void>((resolve, reject) => {
- const timer = setTimeout(() => reject(new Error("websocket timeout")), 5000);
- ws.addEventListener("error", (error) => reject(error as any));
+ const fail = (error: unknown) => {
+ clearTimeout(timer);
+ ws.close();
+ reject(error);
+ };
+ const timer = setTimeout(() => fail(new Error("websocket timeout")), 5000);
+ ws.addEventListener("error", (error) => fail(error as any), { once: true });
ws.addEventListener("message", (event) => {
messages.push(String(event.data));
if (messages.length === 2) {
clearTimeout(timer);
ws.close();
resolve();
}
});
ws.addEventListener("open", () => ws.send("ping"));
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await new Promise<void>((resolve, reject) => { | |
| const timer = setTimeout(() => reject(new Error("websocket timeout")), 5000); | |
| ws.addEventListener("error", (error) => reject(error as any)); | |
| ws.addEventListener("message", (event) => { | |
| messages.push(String(event.data)); | |
| if (messages.length === 2) { | |
| clearTimeout(timer); | |
| ws.close(); | |
| resolve(); | |
| } | |
| }); | |
| ws.addEventListener("open", () => ws.send("ping")); | |
| }); | |
| await new Promise<void>((resolve, reject) => { | |
| const fail = (error: unknown) => { | |
| clearTimeout(timer); | |
| ws.close(); | |
| reject(error); | |
| }; | |
| const timer = setTimeout(() => fail(new Error("websocket timeout")), 5000); | |
| ws.addEventListener("error", (error) => fail(error as any), { once: true }); | |
| ws.addEventListener("message", (event) => { | |
| messages.push(String(event.data)); | |
| if (messages.length === 2) { | |
| clearTimeout(timer); | |
| ws.close(); | |
| resolve(); | |
| } | |
| }); | |
| ws.addEventListener("open", () => ws.send("ping")); | |
| }); |
🤖 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 `@test/presets/nitro-dev.test.ts` around lines 30 - 42, The websocket is not
being closed in the failure paths when a timeout or error occurs. In the
setTimeout callback that rejects with "websocket timeout" and in the
ws.addEventListener("error") handler, ensure you call ws.close() before calling
reject. This guarantees that the socket is properly cleaned up in both error and
timeout scenarios, preventing dangling connections and test flakiness.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Problem
Closes #3939
The dev runtime entry (
src/presets/_nitro/runtime/nitro-dev.ts) hardcodedcrossws/adapters/nodefor WebSocket support. When the dev worker runs under Bun (e.g.bun --bun vite devwithpreset: "bun"), the Node.js adapter throws:Fix
Instead of selecting the adapter manually, expose the WebSocket hooks via the
websocketAppEntry field.env-runneralready attaches the runtime-appropriate crossws adapter per runner (node-worker,bun-process,deno-process) throughcrossws/server, so the correct adapter is chosen automatically:Tests
nitro-devpreset suite (echo handler intest/fixture, realWebSocketconnection in dev mode), enabledfeatures.websocketon the fixture.bun-processdev runner (Bun 1.4.0): fails before the fix (reproduces Websocket does not work in vite dev mode with bun runtime #3939), passes after.nitro-dev/bun/nodesuites pass on both rollup and rolldown builders;typecheck,lint,fmtclean.🤖 Generated with Claude Code