fix(node:url): Respect options parameter in url.format - #24402
Conversation
WalkthroughAdds a Changes
Pre-merge checks✅ Passed checks (4 passed)
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: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between 550522e and 1ead2f0c8aa7448a59eaec72256b12b186e729ed.
📒 Files selected for processing (2)
src/js/node/url.ts(5 hunks)test/js/node/url/url-format-whatwg.test.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/node/url/url-format-whatwg.test.js
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/node/url/url-format-whatwg.test.js
test/js/node/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Node.js module compatibility tests under test/js/node/, separated by module (e.g., test/js/node/assert/)
Files:
test/js/node/url/url-format-whatwg.test.js
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/node/url/url-format-whatwg.test.js
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/js/node/url/url-format-whatwg.test.js
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/js/node/url/url-format-whatwg.test.js
test/js/{bun,node}/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests by module under
/test/js/bun/and/test/js/node/
Files:
test/js/node/url/url-format-whatwg.test.js
src/js/node/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Place Node.js compatibility modules (e.g., node:fs, node:path) under node/
Files:
src/js/node/url.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use require() with string literals only (no dynamic requires)
Do not use ESM import syntax; write modules as CommonJS with export default { ... }
Export via export default {} for modules
Use .$call and .$apply; never use .call or .apply
Prefer JSC intrinsics/private $ APIs for performance (e.g., $Array.from, map.$set, $newArrayWithSize, $debug, $assert)
Validate callbacks with $isCallable and throw $ERR_INVALID_ARG_TYPE with the correct parameter name and expected type
Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
Files:
src/js/node/url.ts
🧠 Learnings (2)
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Follow existing patterns in similar V8 classes, add comprehensive Node.js parity tests, update all symbol files, and document any special behavior
Applied to files:
test/js/node/url/url-format-whatwg.test.js
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/node/**/*.{js,ts} : Place Node.js module compatibility tests under test/js/node/, separated by module (e.g., test/js/node/assert/)
Applied to files:
test/js/node/url/url-format-whatwg.test.js
🔇 Additional comments (7)
test/js/node/url/url-format-whatwg.test.js (1)
11-74: LGTM! Comprehensive test coverage for URL formatting options.The re-enabled tests thoroughly validate the options parameter behavior for auth, fragment, search, and unicode options, ensuring Node.js compatibility.
src/js/node/url.ts (6)
479-481: LGTM! Proper options validation.Correctly validates the options parameter type and uses the appropriate error type per Node.js conventions.
520-527: LGTM! Unicode hostname conversion with appropriate error handling.The try-catch gracefully falls back to the original hostname if unicode conversion fails, which is acceptable for robustness.
529-536: LGTM! Correct host construction with unicode and IPv6 support.The logic properly handles unicode-converted hostnames, IPv6 address formatting, and port appending.
544-547: LGTM! Correct search option handling.Properly excludes search string when the option is explicitly set to a falsy value.
557-564: LGTM! Correct slashes option handling.The logic correctly defaults to including slashes unless explicitly disabled, maintaining backward compatibility.
569-576: LGTM! Correct fragment option handling.Properly excludes or includes the fragment based on the option value, with correct # prefix handling.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/js/node/url.ts (2)
552-555: Apply consistent falsy value handling to options.search.Similar to the auth handling, this check only explicitly handles
false,0, and"", but notnullor other falsy values. For consistency, consider:- if (options.search === false || options.search === 0 || options.search === "") { + if (options.search !== undefined && !options.search) { search = ""; }
499-515: Handle all falsy values consistently for options.auth.The current implementation only treats
false,0, and""as explicit signals to clear auth. If a user passesoptions.auth = null, it will fall through to line 514 and use the reconstructed/existing auth instead of clearing it. For consistency, consider treating all falsy values (exceptundefined) the same way:let auth: string = ""; if (typeof options.auth === "string") { auth = options.auth; - } else if (options.auth === false || options.auth === 0 || options.auth === "") { + } else if (options.auth !== undefined && !options.auth) { auth = ""; } else { auth = authToUse || ""; }Apply the same pattern to the check at line 501:
- if ((authToUse === null || authToUse === undefined) && !(options.auth === false || options.auth === 0 || options.auth === "")) { + if ((authToUse === null || authToUse === undefined) && !(options.auth !== undefined && !options.auth)) { if (this.username || this.password) { authToUse = this.username + (this.password ? ":" + this.password : ""); } }Based on learnings
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between 1ead2f0c8aa7448a59eaec72256b12b186e729ed and 0fc8593f5edb259e47656a8659b5a8d69a8148b0.
📒 Files selected for processing (1)
src/js/node/url.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/js/node/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Place Node.js compatibility modules (e.g., node:fs, node:path) under node/
Files:
src/js/node/url.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use require() with string literals only (no dynamic requires)
Do not use ESM import syntax; write modules as CommonJS with export default { ... }
Export via export default {} for modules
Use .$call and .$apply; never use .call or .apply
Prefer JSC intrinsics/private $ APIs for performance (e.g., $Array.from, map.$set, $newArrayWithSize, $debug, $assert)
Validate callbacks with $isCallable and throw $ERR_INVALID_ARG_TYPE with the correct parameter name and expected type
Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
Files:
src/js/node/url.ts
🔇 Additional comments (3)
src/js/node/url.ts (3)
33-38: LGTM: URLFormatOptions type definition added.The
URLFormatOptionsinterface has been properly defined with all the properties used in the implementation (auth, unicode, search, fragment), addressing the previous review concern.
529-535: LGTM: Unicode hostname transformation.The unicode hostname handling correctly uses a local variable to avoid mutating the input, and appropriately handles conversion errors with a try-catch block.
537-544: LGTM: Host construction with IPv6 support.The host construction logic correctly handles IPv6 addresses by wrapping them in brackets and properly prepends auth credentials.
| if (options !== undefined && typeof options !== "object") { | ||
| throw $ERR_INVALID_ARG_TYPE("options", ["Object"], options); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider validating individual option properties.
While the current validation ensures options is an object, it doesn't validate the types of individual properties (e.g., that options.unicode is a boolean). If invalid property types could cause issues downstream, consider adding property-level validation.
🤖 Prompt for AI Agents
In src/js/node/url.ts around lines 486 to 488, the code only checks that options
is an object but not that its individual properties are of expected types; add
property-level validation after the existing object check to guard against bad
downstream inputs — specifically, for each supported option (e.g., unicode,
auth, base, etc.) verify its presence is the right type (for booleans use typeof
=== "boolean", for strings typeof === "string", for objects use typeof ===
"object" and null-checks or Array.isArray where appropriate), throw
$ERR_INVALID_ARG_TYPE with the option name and expected type when a property has
the wrong type, and ensure optional properties are only validated when defined
so existing behavior remains unchanged.
4755ee4 to
3663fc4
Compare
|
Hii there! Can I get a review on this PR, or is there anything that I should change? It fixes #24343 .Thanks for your time! |
|
Thanks for the PR. This behavior landed on main separately in #34660, which rewrote Verified on a build of current main (165dc9f): This PR is no longer needed, so closing it. |
2 participants
What does this PR do?
Fixes #24343 and #24233
Summary
This PR fixes
url.formatto correctly respect several key properties in theoptionsparameter (auth,search,fragment, andunicode), which were previously being ignored.The issue
url.formatfunction was ignoring the options object being passed as a second parameter.this.authis empty in WHATWG URL,this.usernameandthis.passwordobjects were not being used to create a new auth object, and hence their absence in the final URL.The fix
How did you verify your code works?