feat(cloudflare-module, cloudflare-durable): pass cloudflare env and context in websocket context #3904
Conversation
|
@emretinaztepe is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughRefactors Cloudflare runtime handlers to extract context augmentation logic into a reusable helper function, consolidating per-request environment and context setup across WebSocket, Durable Object, and Pages handlers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/presets/cloudflare/runtime/cloudflare-durable.ts (1)
55-64: Logic looks correct for passing Cloudflare bindings to WebSocket handlers.The implementation properly initializes
request.contextwith Cloudflare bindings before delegating to the WebSocket upgrade handler. The use ofObject.definePropertywithenumerable: trueand the||=operators ensure safe initialization.However, this exact logic is duplicated in
src/presets/cloudflare/runtime/cloudflare-module.ts(lines 28-37). Consider extracting this initialization into a shared helper function in_module-handler.tsto reduce duplication:// In _module-handler.ts function initializeWebSocketContext( request: Request, env: any, context: any ): void { let wsContext = (request as any).context; if (!wsContext) { wsContext = {}; Object.defineProperty(request as any, "context", { enumerable: true, value: wsContext }); } wsContext.cloudflare ||= {}; wsContext.cloudflare.env ||= env; wsContext.cloudflare.context ||= context; }Then use in both files:
initializeWebSocketContext(request, env, context); return ws!.handleUpgrade(request, env, context);Note: The comment stating "crossws will forward
request.contexttopeer.context" may be inaccurate. According to crossws documentation, context should be forwarded viactx.props(ExecutionContext properties) rather thanrequest.context. Verify if the current approach actually works as intended or ifcontext.propsshould be used instead.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/presets/cloudflare/runtime/cloudflare-durable.tssrc/presets/cloudflare/runtime/cloudflare-module.ts
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{ts,js,tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,js,tsx,jsx}: Usepathefor cross-platform path operations instead of Node.jsnode:path
Use ESM and modern JavaScript
Do not add comments explaining what the line does unless prompted
Files:
src/presets/cloudflare/runtime/cloudflare-module.tssrc/presets/cloudflare/runtime/cloudflare-durable.ts
src/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use
unstoragefor storage abstraction
Files:
src/presets/cloudflare/runtime/cloudflare-module.tssrc/presets/cloudflare/runtime/cloudflare-durable.ts
🧠 Learnings (3)
📚 Learning: 2025-12-24T11:45:17.416Z
Learnt from: CR
Repo: nitrojs/nitro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-24T11:45:17.416Z
Learning: Applies to src/runtime/**/*.{ts,js} : In runtime code, prefer Web APIs (fetch, Request, Response, URL, etc.) over Node.js APIs
Applied to files:
src/presets/cloudflare/runtime/cloudflare-module.ts
📚 Learning: 2025-12-03T19:09:13.905Z
Learnt from: medz
Repo: nitrojs/nitro PR: 3834
File: src/presets/cloudflare/server-entry.ts:63-63
Timestamp: 2025-12-03T19:09:13.905Z
Learning: In the Nitro Cloudflare preset (src/presets/cloudflare/server-entry.ts), native errors from oxc-parser and Node.js readFile operations should be allowed to bubble up naturally without wrapping, as their native error messages are more user-friendly and provide better diagnostic information.
Applied to files:
src/presets/cloudflare/runtime/cloudflare-module.ts
📚 Learning: 2025-12-24T11:45:17.416Z
Learnt from: CR
Repo: nitrojs/nitro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-24T11:45:17.416Z
Learning: Applies to src/runtime/**/*.{ts,js} : Keep runtime code minimal and side-effect free to reduce bundle size
Applied to files:
src/presets/cloudflare/runtime/cloudflare-module.ts
🧬 Code graph analysis (2)
src/presets/cloudflare/runtime/cloudflare-module.ts (1)
src/runtime/internal/vite/node-runner.mjs (1)
env(174-174)
src/presets/cloudflare/runtime/cloudflare-durable.ts (1)
src/runtime/internal/vite/node-runner.mjs (1)
env(174-174)
🔇 Additional comments (1)
src/presets/cloudflare/runtime/cloudflare-module.ts (1)
28-37: LGTM! Implementation correctly initializes Cloudflare bindings for WebSocket context.The logic mirrors the implementation in
cloudflare-durable.tsand correctly ensuresenvandcontextare accessible within WebSocket handlers viapeer.context.Note: The code duplication concern and refactoring suggestion were already mentioned in the review of
cloudflare-durable.ts(lines 55-64).
pi0
changed the title
pi0
changed the title
|
Thanks. This looks like a nice enhancenment, however, it seems we can always inject env and context in shared fetch handler to request object so it works both in ws upgrades and nornmal paths (we seem missing it in v3) |
|
@pi0 Any plans to add it to the shared fetch handler soon? I am currently using a local pnpm patch to work around it and would love to remove it as soon as it is officially supported. |
|
I think you can refactor your PR to do this so we can merge/release it asap! |
|
@pi0 I believe it should be fixed for all handlers to be available for all Cloudflare Worker entrypoints such as email, scheduled, etc. What do you think? |
|
We could make an internal util like |
commit: |
|
@pi0 That makes sense, but I am afraid I am not familiar with the implementation of other presets. I will push a refactored version. Please feel free to modify it if it doesn't make sense. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/presets/cloudflare/runtime/_module-handler.ts:
- Around line 10-14: The function signature for augmentContext uses an
un-prefixed DurableObjectState which will fail TS compile; update the parameter
type to use the Cloudflare namespace by changing DurableObjectState to
CF.DurableObjectState in the augmentContext declaration so it matches the other
CF.* types (i.e., ensure the third param is typed as CF.ExecutionContext |
CF.DurableObjectState).
- Around line 61-62: There is a redundant augmentContext(request, env, context)
call before invoking hooks.fetch; remove that duplicate call (the one outside
fetchHandler) or make it conditional so augmentation only happens once: keep
augmentContext inside fetchHandler (since fetchHandler is also called directly
from cloudflare-durable.ts) and either delete the pre-fetchHandler call at line
61 or add a guard on context (e.g., context.__augmented) so hooks.fetch can rely
on an augmented context without causing double augmentation.
🧹 Nitpick comments (2)
src/presets/cloudflare/runtime/_module-handler.ts (1)
81-149: Consider consolidating env exposure for non-fetch handlers.The
scheduled,queue,tail, andtracehandlers all repeat(globalThis as any).__env__ = env;inline. Per the PR discussion, the goal was to call a shared utility from all entrypoints.Since these handlers lack a request object, you could extract a minimal helper (e.g.,
exposeEnv(env)) or extendaugmentContextto accept an optional request. This would reduce duplication and centralize the global env exposure pattern.src/presets/cloudflare/runtime/cloudflare-pages.ts (1)
63-77: Consider using a shared env exposure helper for consistency.The
scheduledhandler still has inline(globalThis as any).__env__ = env;(line 65), mirroring the pattern in_module-handler.ts. If you extract a shared helper for env exposure as suggested for the other handlers, this could be consolidated as well.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/presets/cloudflare/runtime/_module-handler.tssrc/presets/cloudflare/runtime/cloudflare-durable.tssrc/presets/cloudflare/runtime/cloudflare-pages.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/presets/cloudflare/runtime/cloudflare-durable.ts
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{ts,js,tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,js,tsx,jsx}: Usepathefor cross-platform path operations instead of Node.jsnode:path
Use ESM and modern JavaScript
Do not add comments explaining what the line does unless prompted
Files:
src/presets/cloudflare/runtime/_module-handler.tssrc/presets/cloudflare/runtime/cloudflare-pages.ts
src/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use
unstoragefor storage abstraction
Files:
src/presets/cloudflare/runtime/_module-handler.tssrc/presets/cloudflare/runtime/cloudflare-pages.ts
🧠 Learnings (3)
📚 Learning: 2025-12-24T11:45:17.435Z
Learnt from: CR
Repo: nitrojs/nitro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-24T11:45:17.435Z
Learning: Applies to src/runtime/**/*.{ts,js} : In runtime code, prefer Web APIs (fetch, Request, Response, URL, etc.) over Node.js APIs
Applied to files:
src/presets/cloudflare/runtime/_module-handler.tssrc/presets/cloudflare/runtime/cloudflare-pages.ts
📚 Learning: 2025-12-24T11:45:17.435Z
Learnt from: CR
Repo: nitrojs/nitro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-24T11:45:17.435Z
Learning: Applies to src/runtime/**/*.{ts,js} : Keep runtime code minimal and side-effect free to reduce bundle size
Applied to files:
src/presets/cloudflare/runtime/_module-handler.tssrc/presets/cloudflare/runtime/cloudflare-pages.ts
📚 Learning: 2025-12-03T19:09:13.905Z
Learnt from: medz
Repo: nitrojs/nitro PR: 3834
File: src/presets/cloudflare/server-entry.ts:63-63
Timestamp: 2025-12-03T19:09:13.905Z
Learning: In the Nitro Cloudflare preset (src/presets/cloudflare/server-entry.ts), native errors from oxc-parser and Node.js readFile operations should be allowed to bubble up naturally without wrapping, as their native error messages are more user-friendly and provide better diagnostic information.
Applied to files:
src/presets/cloudflare/runtime/_module-handler.tssrc/presets/cloudflare/runtime/cloudflare-pages.ts
🧬 Code graph analysis (2)
src/presets/cloudflare/runtime/_module-handler.ts (1)
src/types/srvx.ts (1)
ServerRequest(1-1)
src/presets/cloudflare/runtime/cloudflare-pages.ts (1)
src/presets/cloudflare/runtime/_module-handler.ts (1)
augmentContext(10-42)
🔇 Additional comments (2)
src/presets/cloudflare/runtime/_module-handler.ts (1)
15-41: LGTM!The augmentation logic correctly handles both srvx and crossws compatibility. The use of spread operators preserves existing values, and
Object.definePropertyappropriately creates the context object when needed.src/presets/cloudflare/runtime/cloudflare-pages.ts (1)
42-44: LGTM!The refactoring correctly delegates context augmentation to the shared helper. The
as anycast oncontextis acceptable given the type difference betweenEventContextandExecutionContext.
| export function augmentContext( | ||
| cfReq: Request | CF.Request, | ||
| env: unknown, | ||
| context: CF.ExecutionContext | DurableObjectState | ||
| ) { |
There was a problem hiding this comment.
Missing type namespace prefix for DurableObjectState.
DurableObjectState on line 13 is not prefixed with CF., but only CF namespace types are imported from @cloudflare/workers-types. This will cause a TypeScript compilation error.
🐛 Proposed fix
export function augmentContext(
cfReq: Request | CF.Request,
env: unknown,
- context: CF.ExecutionContext | DurableObjectState
+ context: CF.ExecutionContext | CF.DurableObjectState
) {🤖 Prompt for AI Agents
In @src/presets/cloudflare/runtime/_module-handler.ts around lines 10 - 14, The
function signature for augmentContext uses an un-prefixed DurableObjectState
which will fail TS compile; update the parameter type to use the Cloudflare
namespace by changing DurableObjectState to CF.DurableObjectState in the
augmentContext declaration so it matches the other CF.* types (i.e., ensure the
third param is typed as CF.ExecutionContext | CF.DurableObjectState).
| augmentContext(request as any, env, context); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/presets/cloudflare/runtime/_module-handler.ts | head -200Repository: nitrojs/nitro
Length of output: 5238
🏁 Script executed:
rg -n "fetchHandler\s*\(" src/presets/cloudflare/runtime/_module-handler.tsRepository: nitrojs/nitro
Length of output: 126
🏁 Script executed:
rg -n "augmentContext" src/presets/cloudflare/runtime/_module-handler.tsRepository: nitrojs/nitro
Length of output: 189
🏁 Script executed:
rg -n "fetchHandler" --type=ts src/presets/cloudflare/Repository: nitrojs/nitro
Length of output: 423
🏁 Script executed:
cat -n src/presets/cloudflare/runtime/cloudflare-durable.ts | sed -n '75,95p'Repository: nitrojs/nitro
Length of output: 758
Remove the redundant augmentContext call at line 61 or refactor to avoid duplication.
augmentContext is called at line 61 before fetchHandler is invoked at line 71, and then called again at line 161 inside fetchHandler. While augmentContext cannot be removed from fetchHandler (it's called directly from cloudflare-durable.ts line 85 without prior augmentation), the call at line 61 can be eliminated if hooks.fetch doesn't depend on augmented context. If hooks.fetch does require it, consider moving the single augmentContext call to be conditional or refactoring to ensure it's only called once before any handler logic runs.
🤖 Prompt for AI Agents
In @src/presets/cloudflare/runtime/_module-handler.ts around lines 61 - 62,
There is a redundant augmentContext(request, env, context) call before invoking
hooks.fetch; remove that duplicate call (the one outside fetchHandler) or make
it conditional so augmentation only happens once: keep augmentContext inside
fetchHandler (since fetchHandler is also called directly from
cloudflare-durable.ts) and either delete the pre-fetchHandler call at line 61 or
add a guard on context (e.g., context.__augmented) so hooks.fetch can rely on an
augmented context without causing double augmentation.
2 participants
Currently, there is no way to access or pass Cloudflare env and bindings for websockets.
This PR passes the Cloudflare environment and context to the CrossWS peer, making bindings accessible within WS handlers.