Skip to content

fix(node): make patchGlobalRequest idempotent - #255

Merged
pi0 merged 3 commits into
mainfrom
fix/249-patch-global-request
Jul 16, 2026
Merged

pi0 merged 3 commits into
mainfrom
fix/249-patch-global-request

Conversation

@pi0x

@pi0x pi0x commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Problem

patchGlobalRequest() is meant to install a srvx Request subclass as globalThis.Request so new Request(req) works in Node.js. It is intended to be safe to call more than once, but a second call was not idempotent:

  • The assignment to globalThis.Request is guarded by !(globalThis.Request as any)._srvx, so a second call correctly skips re-installing the global.
  • However, the function still builds and returns a brand-new PatchedRequest class on every call.

As a result, patchGlobalRequest() === globalThis.Request was false on the second (and every subsequent) call — the returned class was a fresh subclass that was never installed as the global. Any caller relying on the return value being the actual installed Request (e.g. instanceof checks, or capturing the class) could end up with a stale, uninstalled class.

Fix

Make the function idempotent: if the global is already patched (_srvx marker present), return the installed globalThis.Request immediately instead of constructing a new class. This also removes the now-redundant assignment guard.

Repeated calls now return the exact class installed as globalThis.Request.

Test

Added a regression test asserting that repeated patchGlobalRequest() calls return the same class and that it is identical to globalThis.Request (with the original global restored afterwards).

Part of #249

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved request handling so repeated initialization no longer replaces the existing request implementation.
    • Ensured consistent behavior when request support is initialized multiple times.
  • Tests

    • Added regression coverage verifying repeated initialization reuses the same request implementation.

A second call to `patchGlobalRequest()` skipped re-assigning
`globalThis.Request` (guarded by the `_srvx` marker) but still built and
returned a brand-new `PatchedRequest` class, so
`patchGlobalRequest() === globalThis.Request` was false on repeated calls.

Return the already-installed global when it is already patched, so repeated
calls are idempotent and return the exact class installed as
`globalThis.Request`.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@pi0x
pi0x requested a review from pi0 as a code owner July 16, 2026 18:12
@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

patchGlobalRequest() now returns an already-installed patched Request constructor on repeated calls, assigns the patched constructor during initial installation, and adds a node regression test covering constructor and global identity preservation.

Changes

Global Request patching

Layer / File(s) Summary
Idempotent Request installation and regression coverage
src/adapters/_node/request.ts, test/node-request-formdata.test.ts
patchGlobalRequest() detects previously patched constructors, installs PatchedRequest on the initial path, and tests that repeated calls preserve the same constructor and global value.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • h3js/srvx#155: Introduced the explicit global Request patching mechanism that this change makes idempotent.

Suggested reviewers: pi0

Poem

I’m a bunny guarding Request’s door,
No fresh subclass hops in anymore.
The first patch stays, the next calls agree,
Tests watch the global faithfully.
Carrots for stable constructors! 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: making patchGlobalRequest idempotent in the Node adapter.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/249-patch-global-request

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pkg-pr-new

pkg-pr-new Bot commented Jul 16, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/srvx@255

commit: 6a56a9b

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/adapters/_node/request.ts (1)

323-328: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Use optional chaining for safer global access.

If globalThis.Request happens to be undefined (e.g., in environments without a native or polyfilled Request), accessing ._srvx directly will throw a TypeError. Using optional chaining prevents this and is also consistent with how R?._srvx is safely checked in getNativeRequest().

♻️ Proposed refactor
   // Idempotent: if the global is already patched, return the installed class
   // so `patchGlobalRequest() === globalThis.Request` holds on repeated calls.
-  if ((globalThis.Request as any)._srvx) {
+  if ((globalThis.Request as any)?._srvx) {
     return globalThis.Request as unknown as typeof Request;
   }
🤖 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/adapters/_node/request.ts` around lines 323 - 328, Update the idempotency
check in patchGlobalRequest() to use optional chaining when reading the _srvx
marker from globalThis.Request, so the check safely handles environments where
Request is undefined while preserving the existing return behavior for an
already patched class.
🤖 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.

Nitpick comments:
In `@src/adapters/_node/request.ts`:
- Around line 323-328: Update the idempotency check in patchGlobalRequest() to
use optional chaining when reading the _srvx marker from globalThis.Request, so
the check safely handles environments where Request is undefined while
preserving the existing return behavior for an already patched class.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 08274582-9a62-4101-9bd0-9b4d71c4f178

📥 Commits

Reviewing files that changed from the base of the PR and between 58b157b and 61f3508.

📒 Files selected for processing (2)
  • src/adapters/_node/request.ts
  • test/node-request-formdata.test.ts

The idempotence regression test belonged in a global-Request patching
suite, not in the form-data test file. Remove it so the PR is just the
source fix.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@pi0
pi0 merged commit 9a88244 into main Jul 16, 2026
14 of 15 checks passed
@pi0
pi0 deleted the fix/249-patch-global-request branch July 16, 2026 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants