Repository navigation
Conversation
Node's fileURLToPath accepts any object that duck-types as a WHATWG URL (truthy href and protocol, no legacy url.parse() auth/path fields) and reads protocol/hostname/pathname off it directly. Bun's implementation is the native Bun.fileURLToPath, which only accepts a real URL instance or a string and throws ERR_INVALID_ARG_TYPE for plain objects. Add a JS wrapper in internal/url that dispatches strings and URL instances to the native function unchanged and handles URL-like objects with Node's getPathFromURLPosix / getPathFromURLWin32 logic. node:url.fileURLToPath now exports the wrapper, and the duplicated toPathIfFileURL helpers in child_process and fs/glob are replaced with the shared one. Bun.fileURLToPath itself is unchanged.
|
Reproduced on CI: the diff is green on every lane that ran. Build #73151 and the re-roll #73170 both hit the same |
WalkthroughChangesThe shared internal URL module now handles URL-like object detection, file URL conversion, and domain conversion bindings. Node URL APIs, glob, and child process reuse these helpers, with new intrinsic declarations and URL-like object coverage. ChangesURL and File URL conversion
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 11:33 PM PT - Jul 14th, 2026
✅ @robobun, your commit a895bc3362154b89ae21736a51692093fc9411fc passed in 🧪 To try this PR locally: bunx bun-pr 34202That installs a local version of the PR into your bun-34202 --bun |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/js/internal/url.ts`:
- Line 13: Update both native error calls in src/js/internal/url.ts: at line 13,
pass process.platform directly to $ERR_INVALID_FILE_URL_HOST instead of a
preformatted message; at line 61, pass the intrinsic "file" argument directly to
$ERR_INVALID_URL_SCHEME. Preserve the surrounding validation behavior and let
each binding generate its own error text.
In `@test/js/node/url/url-fileurltopath.test.js`:
- Around line 227-229: Update the invalid-input tests around url.fileURLToPath
to use describe.each() with the existing values [null, undefined, 1, true, {},
[], () => {}], creating an independently reported case for each rejected input
while preserving the ERR_INVALID_ARG_TYPE assertion.
🪄 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: 52209779-2fa8-4c30-ac61-f68358df2b68
📒 Files selected for processing (6)
src/js/builtins.d.tssrc/js/internal/fs/glob.tssrc/js/internal/url.tssrc/js/node/child_process.tssrc/js/node/url.tstest/js/node/url/url-fileurltopath.test.js
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-15 and it conflicts with main. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
Reproduction
Node's
fileURLToPathaccepts any object that duck-types as a WHATWG URL viaisURL(truthyhrefandprotocol, no legacyurl.parse()auth/pathfields) and readsprotocol/hostname/pathnameoff it directly. Bun'snode:url.fileURLToPathis the nativeBun.fileURLToPath, which only accepts a realURLinstance or a string.This surfaces through any API that routes a path-or-URL through
toPathIfFileURL(e.g.child_processcwd,fs.globcwd): those already duck-typed the argument but then passed the plain object toBun.fileURLToPath, which rejected it.Fix
Add a
fileURLToPathwrapper ininternal/urlalongside Node'sisURLandtoPathIfFileURL. Strings and realURLinstances go to the nativeBun.fileURLToPathunchanged; URL-like objects go through Node'sgetPathFromURLPosix/getPathFromURLWin32logic (ported verbatim: hostname check, encoded-separator check,decodeURIComponent, drive-letter / UNC handling) so the observable behaviour, including which property is read when, matches Node exactly.node:urlnow exports this wrapper instead of the native function. The duplicatedtoPathIfFileURLhelpers inchild_processandinternal/fs/globare replaced with the shared one frominternal/url. ThedomainToASCII/domainToUnicodenative binding moves tointernal/urlso the Win32 UNC path can use it;node:urlre-exports them unchanged.Bun.fileURLToPathitself is untouched: it still only acceptsURL | string.Verification
bun bd test test/js/node/url/url-fileurltopath.test.jspasses (newURL-like objectblock covers POSIX/Windows/UNC paths, encoded-separator and host errors, legacy-Url rejection). Every case was diffed against Node v26.3.0 and matches byte-for-byte on result and errorcode/message.test/js/node/url/url-domain-ascii-unicode.test.jsandtest/js/bun/util/fileUrl.test.jspass unchanged.Related: #33373 rewrites the native
functionFileURLToPathfor percent-decoding andoptions.windows; this change is orthogonal and layered in JS, so strings andURLinstances continue to route through whichever native implementation is current.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/url/url-fileurltopath.test.js