fix: improve misleading fetch() error for non-string URL inputs#28766
fix: improve misleading fetch() error for non-string URL inputs#28766jw409 wants to merge 1 commit intooven-sh:mainfrom
Conversation
When fetch() receives a non-string value (undefined, number, etc.) that cannot be parsed as a URL, the error now says "URL could not be parsed" instead of the misleading "URL must not be a blank string". When fetch.preconnect() receives a URL that parses but has no hostname, the error now says "URL must have a hostname" instead of "URL must not be a blank string". Fixes oven-sh#21361
WalkthroughModified error handling in fetch.zig to provide more accurate error messages. Added Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/webcore/fetch.zig`:
- Around line 342-349: Extract the inline string used when url_was_from_input is
false into a top-of-file constant (alongside existing error messages like
fetch_error_blank_url) and replace the inline literal in the conditional that
builds the TypeError (the branch that calls ctx.toTypeError(.INVALID_URL, "...",
.{})) so that the code uses the new constant instead of the inline string; keep
the same identifier naming style as other error constants and update references
in the url_str.isEmpty handling block and any other occurrences if present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7fa69933-d199-4ce2-9244-ae5053647be9
📒 Files selected for processing (1)
src/bun.js/webcore/fetch.zig
| if (url_str.isEmpty()) { | ||
| is_error = true; | ||
| const err = ctx.toTypeError(.INVALID_URL, fetch_error_blank_url, .{}); | ||
| const err = if (url_was_from_input) | ||
| ctx.toTypeError(.INVALID_URL, fetch_error_blank_url, .{}) | ||
| else | ||
| ctx.toTypeError(.INVALID_URL, "fetch() URL could not be parsed; received a non-string value that is not a URL object.", .{}); | ||
| return JSPromise.dangerouslyCreateRejectedPromiseValueWithoutNotifyingVM(globalThis, err); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting the inline error message to a constant for consistency.
The conditional error logic correctly addresses the PR objectives. However, the new error message on line 347 is defined inline rather than as a constant like the other error messages at the top of the file (lines 1-4).
🔧 Suggested refactor for consistency
Add a constant at the top of the file alongside other error messages:
pub const fetch_error_no_args = "fetch() expects a string but received no arguments.";
pub const fetch_error_blank_url = "fetch() URL must not be a blank string.";
pub const fetch_error_no_hostname = "fetch() URL must have a hostname.";
+pub const fetch_error_non_string_url = "fetch() URL could not be parsed; received a non-string value that is not a URL object.";
pub const fetch_error_unexpected_body = "fetch() request with GET/HEAD/OPTIONS method cannot have body.";Then use it in the conditional:
const err = if (url_was_from_input)
ctx.toTypeError(.INVALID_URL, fetch_error_blank_url, .{})
else
- ctx.toTypeError(.INVALID_URL, "fetch() URL could not be parsed; received a non-string value that is not a URL object.", .{});
+ ctx.toTypeError(.INVALID_URL, fetch_error_non_string_url, .{});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/webcore/fetch.zig` around lines 342 - 349, Extract the inline
string used when url_was_from_input is false into a top-of-file constant
(alongside existing error messages like fetch_error_blank_url) and replace the
inline literal in the conditional that builds the TypeError (the branch that
calls ctx.toTypeError(.INVALID_URL, "...", .{})) so that the code uses the new
constant instead of the inline string; keep the same identifier naming style as
other error constants and update references in the url_str.isEmpty handling
block and any other occurrences if present.
Summary
Fixes #21361
When
fetch()receives a non-string, non-URL value likeundefined,123, or an object whosetoString()returns a relative path, Bun currently reports:This is misleading because the user did provide a value — it just wasn't a valid URL.
Changes
All changes are in
src/bun.js/webcore/fetch.zig:fetchImpl: Track whether the empty URL string came from actual user input (e.g.fetch("")) vs a failed type conversion (e.g.fetch(undefined)). When the input was a non-string/non-URL value that couldn't be converted, the error now reads:The original "must not be a blank string" message is preserved for
fetch("")where it is accurate.Bun__fetchPreconnect_: When a URL parses successfully but has no hostname (e.g. a path like/some_paththat made it past the empty check), the error now reads:Previously this also used the "blank string" message, which was incorrect — the string wasn't blank, it just lacked a hostname.
Before / After
fetch(undefined)URL must not be a blank stringURL could not be parsed; received a non-string value that is not a URL objectfetch(123)URL must not be a blank stringURL could not be parsed; received a non-string value that is not a URL objectfetch("")URL must not be a blank stringURL must not be a blank string(unchanged)preconnect("/path")URL must not be a blank stringURL must have a hostnameTest plan
fetch(undefined)no longer reports "blank string"fetch(123)no longer reports "blank string"fetch("")still reports "blank string" (correct case)fetch({toString() { return '/path' }})no longer reports "blank string"fetch.preconnect('/path')reports "must have a hostname"fetch('https://example.com')is unaffected🇺🇸 Reid Wiseman — Commander
🇺🇸 Victor Glover — Pilot
🇺🇸 Christina Koch — Mission Specialist
🇨🇦 Jeremy Hansen — Mission Specialist
Artemis II. Open source for all — on this planet and beyond it.