fix(node/net): return string family in server.address()#31465
fix(node/net): return string family in server.address()#31465Tango992 merged 2 commits intodenoland:mainfrom
family in server.address()#31465Conversation
WalkthroughAdds export getIPFamily(ip) and uses it to represent IP family as string literals ("IPv4"/"IPv6") in TCP internals and AddressInfo.family in ext/node/polyfills/internal_binding/tcp_wrap.ts. ServerImpl.address() now returns { port, address, family } in ext/node/polyfills/http.ts. Socket.remoteFamily getter in ext/node/polyfills/net.ts was changed to return the raw numeric family. DNS lookup handling in ext/node/polyfills/dns.ts and ext/node/polyfills/internal/dns/promises.ts accepts string family values and maps them to numeric 4/6. New tests for http, http2, https, dns, and net validate family behavior. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
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 |
6ba8a42 to
b7c45b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ext/node/polyfills/http.ts (1)
2236-2241: Verify edge case: isIP could return 0.If
isIP(addr)returns 0 (not a valid IP), line 2241 defaults to"IPv4", which would be incorrect. While unlikely given that#addr.hostnameis set to validated IPs inlisten(), consider defensive handling:const ipVersion = isIP(addr); if (ipVersion === 0) { // Handle invalid IP - throw error or log warning } return { port: this.#addr.port, address: addr, family: ipVersion === 6 ? "IPv6" : "IPv4", };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
ext/node/polyfills/dns.ts(1 hunks)ext/node/polyfills/http.ts(2 hunks)ext/node/polyfills/internal/dns/promises.ts(1 hunks)ext/node/polyfills/internal_binding/tcp_wrap.ts(5 hunks)ext/node/polyfills/net.ts(1 hunks)tests/unit_node/http2_test.ts(1 hunks)tests/unit_node/http_test.ts(1 hunks)tests/unit_node/https_test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit_node/http_test.ts
- ext/node/polyfills/internal_binding/tcp_wrap.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
ext/node/polyfills/internal/dns/promises.tsext/node/polyfills/net.tsext/node/polyfills/dns.tstests/unit_node/https_test.tstests/unit_node/http2_test.tsext/node/polyfills/http.ts
🧬 Code graph analysis (3)
ext/node/polyfills/internal/dns/promises.ts (1)
ext/node/polyfills/internal/validators.mjs (1)
validateOneOf(219-232)
ext/node/polyfills/dns.ts (1)
ext/node/polyfills/internal/validators.mjs (1)
validateOneOf(219-232)
ext/node/polyfills/http.ts (2)
ext/http/lib.rs (1)
addr(1734-1734)ext/node/polyfills/net.ts (1)
isIP(2756-2756)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: test release macos-aarch64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: lint debug linux-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: build libs
- GitHub Check: lint debug macos-x86_64
🔇 Additional comments (6)
ext/node/polyfills/dns.ts (1)
237-249: LGTM - String family handling implemented correctly.The switch statement properly maps string family values to their numeric equivalents while maintaining backward compatibility with numeric inputs. The fallback to
validateOneOfensures invalid values are caught.tests/unit_node/http2_test.ts (1)
466-502: LGTM - Comprehensive test coverage for both IPv4 and IPv6.The test properly verifies that
server.address()includes thefamilyproperty as a string for both IP versions, aligning with Node.js v18.4.0+ behavior.tests/unit_node/https_test.ts (1)
7-52: LGTM - HTTPS family property test mirrors HTTP/2 pattern.Consistent test implementation for HTTPS servers with proper TLS setup and verification of the string
familyproperty for both IPv4 and IPv6.ext/node/polyfills/internal/dns/promises.ts (1)
197-209: LGTM - Consistent with dns.ts implementation.String family handling is correctly implemented, maintaining consistency between the callback and promise-based DNS APIs.
ext/node/polyfills/http.ts (1)
71-71: LGTM - isIP import added for family determination.ext/node/polyfills/net.ts (1)
1534-1534: Verify that_getpeername().familyreturns a string.The test at
tests/unit_node/http2_test.ts:383expectsremoteFamilyto be the string"IPv4", but the AI summary for this change indicates the type shifts tonumber | undefined. Confirm the actual return type of_getpeername().familyfrom thetcp_wrap.tsimplementation—if it returns numeric values, the test will fail; if it returns strings, this comment is incorrect.
b7c45b3 to
fd3260a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit_node/dns_test.ts (1)
4-7: Good coverage for stringfamily-> numeric result mappingThe added alias for
lookupPromiseand the new test assertinglookup("localhost", { family: "IPv4" })yieldsfamily === 4align with the new DNS option behavior. This is a sensible, focused regression test; extending with an"IPv6"case would be nice-to-have but not required.Also applies to: 93-96
ext/node/polyfills/http.ts (1)
71-71: ServerImpl.address() now exposesfamilyand matches expected IP literalsImporting
isIPand using it inaddress()to derive"IPv4"vs"IPv6"fromthis.#addr.hostnamemakes the HTTP/HTTPS server addresses consistent withnet.Server.address()and the new tests. Givenlisten()normalizeshostto concrete IP literals, this should reliably return the correct family string.Also applies to: 2235-2242
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
ext/node/polyfills/dns.ts(1 hunks)ext/node/polyfills/http.ts(2 hunks)ext/node/polyfills/internal/dns/promises.ts(1 hunks)ext/node/polyfills/internal_binding/tcp_wrap.ts(5 hunks)ext/node/polyfills/net.ts(1 hunks)tests/unit_node/dns_test.ts(2 hunks)tests/unit_node/http2_test.ts(1 hunks)tests/unit_node/http_test.ts(1 hunks)tests/unit_node/https_test.ts(1 hunks)tests/unit_node/net_test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ext/node/polyfills/dns.ts
- tests/unit_node/http2_test.ts
- ext/node/polyfills/internal/dns/promises.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
tests/unit_node/net_test.tsext/node/polyfills/http.tstests/unit_node/https_test.tsext/node/polyfills/net.tstests/unit_node/dns_test.tsext/node/polyfills/internal_binding/tcp_wrap.tstests/unit_node/http_test.ts
🧬 Code graph analysis (1)
ext/node/polyfills/http.ts (1)
ext/node/polyfills/net.ts (1)
isIP(2756-2756)
🔇 Additional comments (5)
ext/node/polyfills/net.ts (1)
1532-1535: remoteFamily now correctly surfaces string family from underlying handle
remoteFamilysimply returningthis._getpeername().familylets it pick up the"IPv4"/"IPv6"string set in the TCP handle, matching the rest of this PR’s family handling. No issues spotted here.tests/unit_node/net_test.ts (1)
307-319: remoteFamily regression test is appropriate and minimalThis test asserts
socket.remoteFamilyis"IPv4"for a simple loopback connection, matching the updated implementation. The lifecycle (end socket, close server, await deferred) is clean and should be stable.tests/unit_node/https_test.ts (1)
4-52: HTTPS Server.address() test correctly validatesfamilyfor IPv4/IPv6The new test for HTTPS servers verifies
server.address()returns anAddressInfowith the expectedaddress,"IPv4"/"IPv6"family, and numericportfor both 127.0.0.1 and ::1. The setup/teardown per block is straightforward and mirrors the HTTP tests.tests/unit_node/http_test.ts (1)
2228-2262: HTTP Server.address() family test aligns with new behaviorThis test sensibly checks that
server.address()returns an object withaddress,"IPv4"/"IPv6"family, and numericportfor explicit IPv4 and IPv6 listens. It matches the updated polyfill’s contract and should catch regressions.ext/node/polyfills/internal_binding/tcp_wrap.ts (1)
60-62: TCP address/peerfamilynow consistently uses"IPv4"/"IPv6"stringsSwitching
AddressInfo.familyand the internal#remoteFamilyto strings and populating them viaisIP()in the constructor,getsockname(), and#connect()makes the low-level TCP handle align with the higher-level expectations ofnet.Server.address()andnet.Socket.remoteFamily. Resetting#remoteFamilyin_onClose()keeps the cached state coherent. This looks correct and consistent with the rest of the PR.Also applies to: 98-98, 146-148, 283-285, 380-382, 489-491
fd3260a to
7fe7995
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ext/node/polyfills/http.ts (1)
234-244: Consider extracting the IP version to family string mapping.The logic
ipVersion === 6 ? "IPv6" : ipVersion === 4 ? "IPv4" : undefinedis duplicated in tcp_wrap.ts (lines 147-151, 288-292, 389-393). Consider extracting this to a helper function in internal/net.ts.// In ext/node/polyfills/internal/net.ts export function getIPFamily(address: string): string | undefined { const ipVersion = isIP(address); return ipVersion === 6 ? "IPv6" : ipVersion === 4 ? "IPv4" : undefined; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
ext/node/polyfills/dns.ts(1 hunks)ext/node/polyfills/http.ts(2 hunks)ext/node/polyfills/internal/dns/promises.ts(1 hunks)ext/node/polyfills/internal_binding/tcp_wrap.ts(5 hunks)ext/node/polyfills/net.ts(1 hunks)tests/unit_node/dns_test.ts(2 hunks)tests/unit_node/http2_test.ts(1 hunks)tests/unit_node/http_test.ts(1 hunks)tests/unit_node/https_test.ts(1 hunks)tests/unit_node/net_test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- ext/node/polyfills/dns.ts
- tests/unit_node/http2_test.ts
- ext/node/polyfills/net.ts
- tests/unit_node/http_test.ts
- ext/node/polyfills/internal/dns/promises.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
tests/unit_node/https_test.tstests/unit_node/net_test.tsext/node/polyfills/http.tsext/node/polyfills/internal_binding/tcp_wrap.tstests/unit_node/dns_test.ts
🧬 Code graph analysis (3)
tests/unit_node/net_test.ts (1)
ext/net/01_net.js (4)
port(593-593)port(643-643)port(660-660)port(697-697)
ext/node/polyfills/http.ts (2)
ext/http/lib.rs (1)
addr(1734-1734)ext/node/polyfills/net.ts (1)
isIP(2756-2756)
ext/node/polyfills/internal_binding/tcp_wrap.ts (1)
ext/node/polyfills/http.ts (1)
address(2234-2245)
Tango992
left a comment
There was a problem hiding this comment.
Thanks! Just a minor review
Aligns Deno's Node.js compatibility layer with Node.js v18.4.0+ behavior
by returning the `family` property as a string ("IPv4" or "IPv6") rather
than a number in `server.address()` and socket address methods.
Node.js briefly changed `family` from string to number in v18.0.0
(nodejs/node#41431), but reverted in v18.4.0 (nodejs/node#43054) due to
ecosystem breakage (nodejs/node#43014).
Changes:
- `http.Server.address()`, `https.Server.address()`, `http2.Server.address()`
now include `family` property as string
- `net.Server.address()` returns string `family` via tcp_wrap.ts
- `socket.remoteFamily` returns string instead of number
- `dns.lookup()` accepts both numeric (0, 4, 6) and string ("IPv4", "IPv6")
family values, matching Node.js behavior
Family value derivation follows Node.js C++ implementation:
- isIP() === 4 → "IPv4"
- isIP() === 6 → "IPv6"
- isIP() === 0 → undefined (defensive, matches Node.js behavior for
non-IP addresses, though not practically reachable via TCP server APIs)
Refs:
- nodejs/node#43054
- nodejs/node@70b516e
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
7fe7995 to
6911007
Compare
|
Thanks for the review. Let me know if I should fix up the last commit, or anything else. I look forward to seeing this fix released— I've encountered this issue while trying to use the latest version of Playwright in a Deno web project. |
|
Thanks, no action is needed from your side. In the meantime can do |
Summary
Aligns Deno's Node.js compatibility layer (
node:net,node:http,node:https,node:http2,node:dns) with Node.js v18.4.0+ behavior by returning thefamilyproperty as a string (
"IPv4"or"IPv6") rather than a number inserver.address()and socket address methods.
Node.js briefly changed
familyfrom string to number in v18.0.0 (nodejs/node#41431),but reverted in v18.4.0 (nodejs/node#43054) due to ecosystem breakage
(nodejs/node#43014).
This fix ensures compatibility with npm packages that rely on the string format, which
has been the stable API since Node.js v18.4.0.
Changes
ext/node/polyfills/http.tsto addfamilyproperty toaddress()returnext/node/polyfills/internal_binding/tcp_wrap.tsto return stringfamilyinstead of number in
getsockname(),getpeername(), and#connect()ext/node/polyfills/net.tsto fixsocket.remoteFamilygetter (no longerneeds conversion since
familyis now a string)ext/node/polyfills/dns.tsandext/node/polyfills/internal/dns/promises.tsto accept string family values ("IPv4","IPv6") inlookup(), matching Node.js behaviortests/unit_node/http_test.ts,tests/unit_node/http2_test.ts,tests/unit_node/https_test.ts,tests/unit_node/dns_test.ts, andtests/unit_node/net_test.tsNode.js Compatibility Note
For non-IP addresses (when
isIP()returns 0), thefamilyproperty isundefined.This matches Node.js C++ behavior in
AddressToJSwhere familyis only set for
AF_INET("IPv4") andAF_INET6("IPv6"), and left undefinedotherwise.
Refs