refactor(node)!: avoid patching global request by default #155
Conversation
pi0
changed the title
📝 WalkthroughWalkthroughThis pull request upgrades multiple development and runtime dependencies, refactors Node.js request handling to use the native Request constructor directly instead of global patching, exports a new optional patchGlobalRequest utility function, and updates test fixtures to use internal request properties. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~13 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/adapters/_node/request.ts (2)
203-206: Consider preventing accidental re-patching.The function checks if the current
globalThis.Requesthas the_srvxmarker to avoid re-patching. However, this check happens after creating a newPatchedRequestclass. If called multiple times, it creates multiple class instances (though only one gets assigned globally).Consider documenting that calling this function multiple times is safe, or return early if already patched.
🔎 Optional optimization
export function patchGlobalRequest(): typeof Request { + // Return early if already patched + if ((globalThis.Request as any)._srvx) { + return globalThis.Request; + } + const NativeRequest = ((globalThis as any)[ Symbol.for("srvx.nativeRequest") ] ??= globalThis.Request) as typeof globalThis.Request; const PatchedRequest = class Request extends NativeRequest { static _srvx = true; // ... rest of implementation }; - if (!(globalThis.Request as any)._srvx) { - globalThis.Request = PatchedRequest as unknown as typeof globalThis.Request; - } + + globalThis.Request = PatchedRequest as unknown as typeof globalThis.Request; return PatchedRequest; }
179-181: Document the Symbol.for usage for native Request storage.The code uses
Symbol.for("srvx.nativeRequest")to store the original native Request. This is a global symbol that could theoretically conflict with other libraries using the same symbol name.Consider documenting this design choice, or use a more unique symbol name.
test/_fixture.ts (1)
131-132: Document the_requestfallback pattern for users.The code now uses
new Request((req as any)._request || req)instead of directly passingreq. This pattern is mentioned in the PR description as an alternative to callingpatchGlobalRequest().Consider documenting this pattern in the library's migration guide or API documentation, as users upgrading will need to know about this change.
Also, consider removing the commented-out line 131 to keep the code clean.
🔎 Cleanup: Remove commented code
case "/req-new-req": { - // const clone = new Request(req); const clone = new Request((req as any)._request || req); return Response.json({ method: clone.method, pathname: new URL(clone.url).pathname, headers: Object.fromEntries(clone.headers), }); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
package.jsonsrc/adapters/_node/request.tssrc/adapters/node.tstest/_fixture.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/adapters/_node/request.ts (1)
src/adapters/node.ts (1)
patchGlobalRequest(29-29)
⏰ 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: Cloudflare Pages
🔇 Additional comments (4)
src/adapters/node.ts (1)
29-29: LGTM! Export addition aligns with PR objectives.The addition of
patchGlobalRequestto the public exports makes the global patching functionality opt-in, which aligns with the PR's goal of avoiding automatic global patching.package.json (1)
62-93: All specified dependency versions have been verified to exist on npm and are publicly available.Verification confirms:
@cloudflare/workers-types@4.20251230.0is available (today's release)@types/node@25.0.3(major version bump) existsvitest@4.0.16and@vitest/coverage-v8@4.0.16(major version 4) are available- All other updated versions in the dependency list are present on the npm registry
No version availability issues detected.
src/adapters/_node/request.ts (2)
147-155: Verify undici's duplex requirement is correctly handled.The code sets
duplex: "half"when a body is present, which is required by undici (Node.js's fetch implementation) when streaming body content. This is the correct approach for Node.js compatibility. The @ts-expect-error annotation is also appropriate sinceduplexis undici-specific and not part of the standard RequestInit type.
19-19: No action needed for this change; existing code patterns are compatible.The shift to native
globalThis.Requestis safe because actual usage patterns don't rely on automatic patching. NodeRequest is instantiated directly asnew NodeRequest({ req, res })rather thannew Request(nodeRequest). The test fixtures already use the recommended alternative patternnew Request(req._request || req), andpatchGlobalRequest()remains available for explicit use if needed. No code paths that would break from this change were found in the codebase.
| constructor( | ||
| input: string | URL | globalThis.Request, | ||
| options?: RequestInit, | ||
| ) { | ||
| if (typeof input === "object" && "_request" in input) { | ||
| input = (input as any)._request; | ||
| } | ||
| super(input, options); | ||
| } |
There was a problem hiding this comment.
Add null check when unwrapping _request property.
The constructor checks typeof input === "object" but doesn't guard against null, which is also an object in JavaScript. This could cause a runtime error if null is passed.
🔎 Proposed fix
constructor(
input: string | URL | globalThis.Request,
options?: RequestInit,
) {
- if (typeof input === "object" && "_request" in input) {
+ if (input && typeof input === "object" && "_request" in input) {
input = (input as any)._request;
}
super(input, options);
}📝 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.
| constructor( | |
| input: string | URL | globalThis.Request, | |
| options?: RequestInit, | |
| ) { | |
| if (typeof input === "object" && "_request" in input) { | |
| input = (input as any)._request; | |
| } | |
| super(input, options); | |
| } | |
| constructor( | |
| input: string | URL | globalThis.Request, | |
| options?: RequestInit, | |
| ) { | |
| if (input && typeof input === "object" && "_request" in input) { | |
| input = (input as any)._request; | |
| } | |
| super(input, options); | |
| } |
🤖 Prompt for AI Agents
In src/adapters/_node/request.ts around lines 193 to 201, the constructor
unwraps input._request but only checks typeof input === "object" which fails for
null; update the guard to check input !== null (e.g. input !== null && typeof
input === "object" && "_request" in input) before accessing _request, and ensure
the unwrap uses the narrowed type to avoid potential runtime errors when input
is null.
Requestconstructor in Node.js (undici) has incompatible private#accessors that breaksnew Request(req)(when req comes from srvx)srvx used to have a global patch to workaround this issue, patching globally is not the best idea and can cause implicit issues even though we fixed many of those in past iteration,s more are possible... (even outside srvx by just importing it!)
This PR avoids global patch but instead exposes it as
patchGlobalRequest()utility fromsrvx/node.Alternatively, users can simply use
new Request(req._request || req)as solution!Summary by CodeRabbit
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.