Uh oh!
There was an error while loading. Please reload this page.
perf(fetch): skip empty RequestInit work - #5701
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #5701 +/- ##
==========================================
- Coverage 93.47% 93.46% -0.01%
==========================================
Files 110 110 Lines 38846 38891 +45 ==========================================
+ Hits 36310 36350 +40 - Misses 2536 2541 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
39de48f to
652650eCompare…loor The Request constructor treated every init as non-empty because undici's priority member defaults to "auto". That forced the header clone/clear path on every fetch(url) / new Request(url). Idle keep-alive reuse was deferred with setTimeout(0), adding ~1ms of latency per sequential fetch. Use a ref'd setImmediate so validation still runs after poll without the timer floor or the unref'd-Immediate poll stall. Assisted by Cursor Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>
652650e to
d7f3facComparetsctx
commented
Aug 19, 2026
The correct fix here is to set the default value when defining the internal request, rather than in the converter. The default value is currently being set in the wrong place. The following diff should fix the issue. I do not think this test is necessary. diff --git a/lib/web/fetch/request.js b/lib/web/fetch/request.js
index dbe809c2..56945ec0 100644
--- a/lib/web/fetch/request.js+++ b/lib/web/fetch/request.js@@ -923,7 +923,7 @@ function makeRequest (init) {
serviceWorkers: init.serviceWorkers ?? 'all',
initiator: init.initiator ?? '',
destination: init.destination ?? '',
- priority: init.priority ?? null,+ priority: init.priority ?? 'auto',
origin: init.origin ?? 'client',
policyContainer: init.policyContainer ?? 'client',
referrer: init.referrer ?? 'client',
@@ -1129,8 +1129,7 @@ webidl.converters.RequestInit = webidl.dictionaryConverter([
{
key: 'priority',
converter: webidl.converters.DOMString,
- allowedValues: ['high', 'low', 'auto'],- defaultValue: () => 'auto'+ allowedValues: ['high', 'low', 'auto']
}
])
Separately, I am strongly opposed to replacing This would make the code less maintainable and more error-prone. Whenever a new property is added in a future specification update, the property checks would also need to be updated manually. If that update were overlooked, the new property would not be handled correctly, whereas the current implementation requires no such changes. Moreover, this optimization does not appear to provide any significant performance improvement. |
Move the "auto" default from the RequestInit WebIDL converter to makeRequest so an omitted init stays empty after conversion. Restores the Object.keys empty-init check. Assisted by Cursor Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>
anonrig
commented
Aug 21, 2026
Addressed in af14557. Moved the Kept a pair of Request-copy tests as a regression for the converter default making omitted init non-empty. Happy to drop them if you'd still rather not have them. |
Review feedback: the converter default move is sufficient; these Request-copy cases are not needed. Assisted by Cursor Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>
anonrig
commented
Aug 21, 2026
Dropped the extra Request-copy tests — thanks again. |
| } | ||
| }, 0) | ||
| socket[kIdleSocketValidationTimeout].unref?.() | ||
| }) |
There was a problem hiding this comment.
please move these to an independent pr
There was a problem hiding this comment.
Moved the idle-socket setImmediate change to #5707.
Keep this PR scoped to the RequestInit priority default fix. Assisted by Cursor Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>
anonrig
commented
Aug 21, 2026
@mcollina can you review |
This relates to...
prioritydefault was in the WebIDL converter, so every omitted init counted as non-emptyRationale
The Request constructor treated every
RequestInitas non-empty because undici'sprioritymember defaulted to"auto"in the WebIDL converter. Fetch's "If init is not empty" steps then always cloned/cleared/re-appended headers — including on the commonfetch(url)/new Request(url)path.Changes
priorityto"auto"on the inner request inmakeRequest, not in the RequestInit converter, so an omitted init stays empty after conversion andObject.keys(init)is correct.The idle-socket
setImmediatechange now lives in a follow-up PR (see review thread).Features
N/A
Bug Fixes
new Request(existingRequest)with omitted init no longer resetsreferrerPolicy/ related request state just becauseprioritydefaulted to"auto"in the converter.Breaking Changes and Deprecations
N/A. Copying a Request with an empty init now matches the spec more closely (does not run the "init is not empty" resets).
Benchmarks
Re-run 2026-08-21 on Node v22.14.0 (linux x64, Intel Xeon, 4 cores). Medians of 5 runs vs
main@181c293.new Request(url)new Request(url, { method })The
{ method }path still counts as non-empty, so the header clone/clear work still runs. It is a bit faster because the converter no longer injects a defaultpriority.Status