refactor(fetch): align request and response semantics with web#11
Conversation
📝 WalkthroughWalkthroughIntroduces RequestInit and ResponseInit types and refactors Request/Response constructors to accept positional body/init arguments; removes Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 2
🧹 Nitpick comments (2)
lib/src/fetch/request.dart (1)
39-54: Consider reducing redundant header cloning inRequest.json.Headers are cloned multiple times in this flow:
_coerceInitclonesinit?.headers(line 103)nextInit.headers?.clone()(line 41)RequestInitconstructor clones again (line 48-51 → constructor at line 11)Requestconstructor clones again (line 22)While this ensures complete isolation, it creates unnecessary allocations. Consider passing headers directly where cloning has already occurred, or using an internal constructor that skips redundant clones.
♻️ Potential optimization
factory Request.json(Uri url, Object? body, [RequestInit? init]) { final nextInit = _coerceInit(init, body: json.encode(body)); - final nextHeaders = nextInit.headers?.clone() ?? Headers(); + final nextHeaders = nextInit.headers ?? Headers(); if (!nextHeaders.has('content-type')) { nextHeaders.set('content-type', 'application/json; charset=utf-8'); } - return Request( - url, - RequestInit( - method: nextInit.method, - headers: nextHeaders, - body: nextInit.body, - ), + return Request._internal( + url: url, + method: _normalizeMethod(nextInit.method ?? 'POST'), + headers: nextHeaders, + bodyData: BodyData.fromInit(nextInit.body), ); }Note: This would require ensuring
_applyDefaultBodyHeadersis called, so the full optimization may need additional adjustments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/fetch/request.dart` around lines 39 - 54, Request.json is performing redundant Headers clones (via _coerceInit, nextInit.headers?.clone(), RequestInit constructor, and Request constructor); to fix, avoid duplicate allocations by passing the already-cloned headers from _coerceInit through to the Request/RequestInit constructors without re-cloning: either use the headers instance returned by _coerceInit (nextInit.headers) directly instead of calling .clone() in Request.json, or add an internal/private constructor (e.g., RequestInit._internal or Request._internal) that accepts headers without cloning and call that from Request.json; ensure _applyDefaultBodyHeaders still runs against the final headers instance so behavior is unchanged.lib/src/fetch/response.dart (1)
27-30: Use empty string for defaultstatusTextto achieve Fetch API parity.The code explicitly claims to be "aligned with Fetch
ResponseInit", but currently defaultsstatusTextto the RFC reason phrase (e.g.,'OK','No Content') instead of the empty string that the Fetch API specifies. This makes constructor-created responses observably different from the web API.Suggested fix
}) : status = _validateStatus(init?.status ?? HttpStatus.ok), - statusText = - init?.statusText ?? - HttpStatus.reasonPhrase(init?.status ?? HttpStatus.ok), + statusText = init?.statusText ?? '', headers = init?.headers?.clone() ?? Headers(), bodyData = BodyData.fromInit(body) {Note: The test in
test/request_response_test.dartthat expectsstatusTextto be'No Content'will need to be updated to expect an empty string instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/fetch/response.dart` around lines 27 - 30, The Response constructor currently defaults statusText to the RFC reason phrase via HttpStatus.reasonPhrase in the initializer (see the statusText initializer in response.dart), but to match Fetch API parity change the default to an empty string by using init?.statusText ?? '' instead of HttpStatus.reasonPhrase(...); update any tests (e.g., in test/request_response_test.dart) that expect 'No Content' to expect an empty string and ensure _validateStatus(status) behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/src/fetch/response.dart`:
- Around line 80-85: The Response.redirect() call currently constructs a
redirected Response by invoking Response._create with url: location and
redirected: true; change it to match the Fetch Standard by creating the Response
with no response URL and not marked as redirected — call Response._create with
url: null (or omit the url) and redirected: false, while still passing the new
ResponseInit (e.g., ResponseInit(status: status, headers: nextHeaders)); update
the Response.redirect() implementation rather than changing Response._create so
the redirect helper returns a non-redirected Response with a null url.
- Around line 22-33: Response._create currently allows a non-empty body with
null-body statuses; update Response._create to validate that if
_validateStatus(init?.status ?? HttpStatus.ok) yields a null-body status (204,
205, 304) then BodyData.hasBody (from BodyData.fromInit(body) /
bodyData.hasBody) must be false and otherwise reject construction (throw/raise
an error). Modify the constructor to compute status and bodyData as now, then
check bodyData.hasBody against the status and throw a descriptive exception when
a body is present for null-body statuses; keep existing calls to _validateStatus
and _applyDefaultBodyHeaders. Add a regression test that constructs Response
with a body and ResponseInit(status: 204/205/304) and asserts that construction
fails.
---
Nitpick comments:
In `@lib/src/fetch/request.dart`:
- Around line 39-54: Request.json is performing redundant Headers clones (via
_coerceInit, nextInit.headers?.clone(), RequestInit constructor, and Request
constructor); to fix, avoid duplicate allocations by passing the already-cloned
headers from _coerceInit through to the Request/RequestInit constructors without
re-cloning: either use the headers instance returned by _coerceInit
(nextInit.headers) directly instead of calling .clone() in Request.json, or add
an internal/private constructor (e.g., RequestInit._internal or
Request._internal) that accepts headers without cloning and call that from
Request.json; ensure _applyDefaultBodyHeaders still runs against the final
headers instance so behavior is unchanged.
In `@lib/src/fetch/response.dart`:
- Around line 27-30: The Response constructor currently defaults statusText to
the RFC reason phrase via HttpStatus.reasonPhrase in the initializer (see the
statusText initializer in response.dart), but to match Fetch API parity change
the default to an empty string by using init?.statusText ?? '' instead of
HttpStatus.reasonPhrase(...); update any tests (e.g., in
test/request_response_test.dart) that expect 'No Content' to expect an empty
string and ensure _validateStatus(status) behavior remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4bc2c021-fe50-4056-b94b-c9692e06b9ef
📒 Files selected for processing (12)
CHANGELOG.mdREADME.mdexample/main.dartlib/src/core/http_status.dartlib/src/core/mime_type.dartlib/src/fetch/body.dartlib/src/fetch/form_data.dartlib/src/fetch/headers.dartlib/src/fetch/request.dartlib/src/fetch/response.darttest/public_api_surface_test.darttest/request_response_test.dart
| Response._create( | ||
| Object? body, | ||
| int status = HttpStatus.ok, | ||
| String? statusText, | ||
| Headers? headers, | ||
| this.url, | ||
| this.redirected = false, | ||
| }) : status = _validateStatus(status), | ||
| statusText = statusText ?? HttpStatus.reasonPhrase(status), | ||
| headers = headers?.clone() ?? Headers(), | ||
| ResponseInit? init, { | ||
| required this.url, | ||
| required this.redirected, | ||
| }) : status = _validateStatus(init?.status ?? HttpStatus.ok), | ||
| statusText = | ||
| init?.statusText ?? | ||
| HttpStatus.reasonPhrase(init?.status ?? HttpStatus.ok), | ||
| headers = init?.headers?.clone() ?? Headers(), | ||
| bodyData = BodyData.fromInit(body) { | ||
| _applyDefaultBodyHeaders(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
According to the Fetch Response() constructor semantics, should a Response with a non-null body and status 204, 205, or 304 be rejected?
💡 Result:
Yes. The Fetch Standard defines 204, 205, and 304 as “null body status” codes [1]. In the new Response(body, init) constructor steps, if body is non-null and the response’s status is a null body status, the constructor must throw a TypeError (i.e., reject construction) [2].
Sources:
[1] Fetch Standard — “A null body status is … 204, 205, or 304” (fetch.spec.whatwg.org)
[2] Fetch Standard — Response constructor: “If body is non-null … if status is a null body status … throw a TypeError” (fetch.spec.whatwg.org)
Citations:
🏁 Script executed:
# First, check the full implementation of Response._create and related code
head -n 100 lib/src/fetch/response.dart | cat -nRepository: medz/ht
Length of output: 3390
🏁 Script executed:
# Check if BodyData.fromInit handles null body properly
fd BodyData --type f -x grep -l "class BodyData" {}Repository: medz/ht
Length of output: 33
🏁 Script executed:
# Search for any existing tests around null-body statuses
rg -n "204|205|304" test/ -A 2 -B 2Repository: medz/ht
Length of output: 729
🏁 Script executed:
# Check for existing error handling in Response constructor
rg -n "ArgumentError|TypeError" lib/src/fetch/response.dart -B 2 -A 2Repository: medz/ht
Length of output: 275
🏁 Script executed:
# Find and examine BodyData class
fd . lib/src/fetch -name "*.dart" -type f | xargs grep -l "class BodyData"Repository: medz/ht
Length of output: 283
🏁 Script executed:
# Read the BodyData class to understand fromInit behavior
cat -n lib/src/fetch/body.dart | head -n 80Repository: medz/ht
Length of output: 2671
🏁 Script executed:
# Search for any existing tests that might cover Response with body + null-body status
rg -n "Response.*204|Response.*205|Response.*304" test/ -B 2 -A 3Repository: medz/ht
Length of output: 33
🏁 Script executed:
# Check what _validateStatus does
rg -n "_validateStatus" lib/src/fetch/response.dart -A 5Repository: medz/ht
Length of output: 511
🏁 Script executed:
# Find BodyData.fromInit implementation
rg -n "fromInit" lib/src/fetch/ -A 15Repository: medz/ht
Length of output: 2947
🏁 Script executed:
# Check if there are any tests attempting invalid Response construction
rg -n "ResponseInit.*status.*204\|ResponseInit.*status.*205\|ResponseInit.*status.*304" test/Repository: medz/ht
Length of output: 33
🏁 Script executed:
# Look at the complete BodyData class to find hasBody or similar
cat lib/src/fetch/body.dart | grep -n "hasBody\|_present\|fromInit" -A 3 -B 1Repository: medz/ht
Length of output: 1265
Add validation to reject bodies for 204/205/304 responses in Response._create.
The current implementation allows Response('x', ResponseInit(status: 204)) and similar invalid combinations. The Fetch Standard specifies that responses with status 204, 205, or 304 (null-body statuses) must not include a body; construction should be rejected. The shared _create constructor is the correct place to enforce this using the existing BodyData.hasBody property.
Suggested fix
}) : status = _validateStatus(init?.status ?? HttpStatus.ok),
statusText =
init?.statusText ??
HttpStatus.reasonPhrase(init?.status ?? HttpStatus.ok),
headers = init?.headers?.clone() ?? Headers(),
bodyData = BodyData.fromInit(body) {
+ if (bodyData.hasBody && const <int>{204, 205, 304}.contains(status)) {
+ throw ArgumentError.value(
+ status,
+ 'status',
+ 'Responses with status $status must not include a body',
+ );
+ }
_applyDefaultBodyHeaders();
}Please add a regression test for this case alongside the fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/src/fetch/response.dart` around lines 22 - 33, Response._create currently
allows a non-empty body with null-body statuses; update Response._create to
validate that if _validateStatus(init?.status ?? HttpStatus.ok) yields a
null-body status (204, 205, 304) then BodyData.hasBody (from
BodyData.fromInit(body) / bodyData.hasBody) must be false and otherwise reject
construction (throw/raise an error). Modify the constructor to compute status
and bodyData as now, then check bodyData.hasBody against the status and throw a
descriptive exception when a body is present for null-body statuses; keep
existing calls to _validateStatus and _applyDefaultBodyHeaders. Add a regression
test that constructs Response with a body and ResponseInit(status: 204/205/304)
and asserts that construction fails.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
lib/src/fetch/request.dart (3)
108-114: Consider reducing header cloning in the factory path.In the factory constructor path (e.g.,
Request.text), headers may be cloned up to three times:
- In
RequestInitconstructor (line 11)- In
_coerceInit(line 111)- In
Requestconstructor (line 24)While correct, this is inefficient. Consider having
_coerceInitpass headers through without cloning, relying on the final clone inRequest.♻️ Suggested optimization
static RequestInit _coerceInit(RequestInit? init, {required Object? body}) { return RequestInit( method: init?.method ?? 'POST', - headers: init?.headers?.clone(), + headers: init?.headers, body: body, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/fetch/request.dart` around lines 108 - 114, The factory-path is cloning headers multiple times; modify _coerceInit so it does not clone headers (i.e., pass init?.headers through directly into the returned RequestInit) and rely on the final clone inside the Request constructor to produce an owned headers object; update the _coerceInit implementation to avoid a headers.clone() call while preserving null-safety and existing behavior for RequestInit, leaving RequestInit and Request constructors unchanged.
20-26: Minor: Headers may be cloned twice.When a
RequestInitis passed with headers, they're cloned inRequestInit's constructor (line 11) and again here (line 24). This is safe but slightly inefficient. Consider accepting already-cloned headers fromRequestInitwithout re-cloning.♻️ Suggested optimization
- Request(Uri url, [RequestInit? init]) - : this._create( - url: url, - method: init?.method ?? 'GET', - headers: init?.headers?.clone() ?? Headers(), - bodyData: BodyData.fromInit(init?.body), - ); + Request(Uri url, [RequestInit? init]) + : this._create( + url: url, + method: init?.method ?? 'GET', + headers: init?.headers ?? Headers(), + bodyData: BodyData.fromInit(init?.body), + );Since
RequestInitalready clones headers on construction, re-cloning is unnecessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/fetch/request.dart` around lines 20 - 26, The Request constructor is cloning headers redundantly; update Request(Uri url, [RequestInit? init]) to pass init?.headers directly to the internal _create when present (instead of calling .clone() again) so already-cloned headers from RequestInit are reused; specifically modify the Request constructor call that currently does headers: init?.headers?.clone() ?? Headers() to accept init?.headers ?? Headers(), leaving Headers() fallback and leaving RequestInit and Headers.clone() behavior unchanged.
49-62: Minor redundancy inRequest.json.At line 58,
nextInit.method ?? 'POST'is redundant because_coerceInitalready defaults method to'POST'(line 110). The null-coalescing fallback will never trigger.♻️ Simplified version
return Request._create( url: url, - method: nextInit.method ?? 'POST', + method: nextInit.method!, headers: nextHeaders, bodyData: BodyData.fromInit(nextInit.body), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/fetch/request.dart` around lines 49 - 62, The Request.json factory redundantly uses nextInit.method ?? 'POST' even though _coerceInit already defaults method to 'POST'; remove the unreachable null-coalescing fallback and pass nextInit.method directly to Request._create (i.e., change the method argument in Request.json to use nextInit.method) so the defaulting is centralized in _coerceInit; update any relevant tests if they relied on the previous fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/src/fetch/request.dart`:
- Around line 108-114: The factory-path is cloning headers multiple times;
modify _coerceInit so it does not clone headers (i.e., pass init?.headers
through directly into the returned RequestInit) and rely on the final clone
inside the Request constructor to produce an owned headers object; update the
_coerceInit implementation to avoid a headers.clone() call while preserving
null-safety and existing behavior for RequestInit, leaving RequestInit and
Request constructors unchanged.
- Around line 20-26: The Request constructor is cloning headers redundantly;
update Request(Uri url, [RequestInit? init]) to pass init?.headers directly to
the internal _create when present (instead of calling .clone() again) so
already-cloned headers from RequestInit are reused; specifically modify the
Request constructor call that currently does headers: init?.headers?.clone() ??
Headers() to accept init?.headers ?? Headers(), leaving Headers() fallback and
leaving RequestInit and Headers.clone() behavior unchanged.
- Around line 49-62: The Request.json factory redundantly uses nextInit.method
?? 'POST' even though _coerceInit already defaults method to 'POST'; remove the
unreachable null-coalescing fallback and pass nextInit.method directly to
Request._create (i.e., change the method argument in Request.json to use
nextInit.method) so the defaulting is centralized in _coerceInit; update any
relevant tests if they relied on the previous fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 701a202a-0cc1-48d3-a60d-2daaad8c4f75
📒 Files selected for processing (3)
lib/src/fetch/request.dartlib/src/fetch/response.darttest/request_response_test.dart
1 participant
Summary by CodeRabbit
Breaking Changes
New Features
Documentation
Tests