Skip to content

Revert the change of network interfaces family from string to integer - #43054

Closed
aduh95 wants to merge 5 commits into
nodejs:mainfrom
aduh95:revert-net-family-integer
Closed

Revert the change of network interfaces family from string to integer#43054
aduh95 wants to merge 5 commits into
nodejs:mainfrom
aduh95:revert-net-family-integer

Conversation

@aduh95

@aduh95 aduh95 commented May 11, 2022

Copy link
Copy Markdown
Contributor

This reverts commits 68fb0bf, 4ad342a, and 3a26db9.

Fixes: #43014
@nodejs-github-bot

nodejs-github-bot commented May 11, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/net
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels May 11, 2022
@aduh95 aduh95 added semver-major PRs that contain breaking changes and should be released in the next major version. tsc-agenda Issues and PRs to discuss during Technical Steering Committee meetings. labels May 11, 2022
@targos

targos commented May 11, 2022

Copy link
Copy Markdown
Member

IMO the revert doesn't make sense if it is semver-major, because everyone will have to adapt to the change for v18 (which is going to be LTS).
mcollina
mcollina approved these changes May 11, 2022
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm
mcollina
mcollina approved these changes May 11, 2022
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm
@mcollina

mcollina commented May 11, 2022

Copy link
Copy Markdown
Member

+1 to land it on Node v18.
@mcollina mcollina added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels May 11, 2022
@mcollina

mcollina commented May 11, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

@nodejs/tsc at Today meeting we did not have quorum to LGTM as a non-semver-major change. Does anybody from the TSC objects to this?

We actually had quorum at the end of the meeting, we approve landing this as semver-minor.
@mcollina mcollina added semver-minor PRs that contain new features and should be released in the next minor version. dont-land-on-v12.x and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels May 11, 2022
@RaisinTen RaisinTen added the notable-change PRs with changes that should be highlighted in changelogs. label May 11, 2022
RaisinTen
RaisinTen approved these changes May 11, 2022
View reviewed changes
Comment thread lib/dns.js Outdated
@RaisinTen

RaisinTen commented May 11, 2022

Copy link
Copy Markdown
Member

Is it okay if we remove this from the tsc-agenda now, since everyone in the meeting was +1 with reverting?
@aduh95 aduh95 added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. and removed backport-requested-v18.x labels May 11, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 11, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
Trott
Trott approved these changes May 11, 2022
View reviewed changes

@Trott Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubber stamp
@nodejs-github-bot

nodejs-github-bot commented Jun 6, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/44397/
@nodejs-github-bot

nodejs-github-bot commented Jun 7, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/44401/
@nodejs-github-bot

nodejs-github-bot commented Jun 8, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/44409/
@github-actions github-actions Bot mentioned this pull request Jun 13, 2022
Open
15 tasks
@mhdawson mhdawson mentioned this pull request Jun 15, 2022
Closed
@mhdawson

mhdawson commented Jun 15, 2022

Copy link
Copy Markdown
Member

We discussed in the TSC meeting and there were no objections to this landing. Doing that now.
mhdawson pushed a commit that referenced this pull request Jun 15, 2022
@aduh95 @mhdawson
dns: accept 'IPv4' and 'IPv6' for family
70b516e 
Refs: #43014

PR-URL: #43054
Fixes: #43014
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@mhdawson

mhdawson commented Jun 15, 2022

Copy link
Copy Markdown
Member

Landed as 70b516e
@mhdawson mhdawson closed this Jun 15, 2022
@aduh95 aduh95 mentioned this pull request Jun 15, 2022
Merged
danielleadams pushed a commit that referenced this pull request Jun 16, 2022
@aduh95 @danielleadams
dns: accept 'IPv4' and 'IPv6' for family
0bb84b0 
Refs: #43014

PR-URL: #43054
Fixes: #43014
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
danielleadams added a commit that referenced this pull request Jun 16, 2022
@danielleadams
2022-06-16, Version 18.4.0 (Current)
e339e9c 
Notable changes:

* crypto:
  * remove Node.js-specific webcrypto extensions (Filip Skokan) #43310
  * add CFRG curves to Web Crypto API (Filip Skokan) #42507
* dns:
  * accept `'IPv4'` and `'IPv6'` for `family` (Antoine du Hamel) #43054
* report:
  * add more heap infos in process report (theanarkh) #43116

PR-URL: #43385
danielleadams added a commit that referenced this pull request Jun 16, 2022
@danielleadams
2022-06-16, Version 18.4.0 (Current)
f50a80c 
Notable changes:

* crypto:
  * remove Node.js-specific webcrypto extensions (Filip Skokan) #43310
  * add CFRG curves to Web Crypto API (Filip Skokan) #42507
* dns:
  * accept `'IPv4'` and `'IPv6'` for `family` (Antoine du Hamel) #43054
* report:
  * add more heap infos in process report (theanarkh) #43116

PR-URL: #43385
@chitowncrispy chitowncrispy mentioned this pull request Jul 23, 2022
Closed
1 task
@AdamMajer AdamMajer mentioned this pull request Jul 28, 2022
Closed
@aduh95
aduh95 deleted the revert-net-family-integer branch March 29, 2023 16:00
@aduh95 aduh95 mentioned this pull request Mar 29, 2023
Merged
@sapphi-red sapphi-red mentioned this pull request Aug 10, 2023
Merged
9 tasks
@aduh95 aduh95 mentioned this pull request Sep 16, 2024
Closed
nrako added a commit to nrako/deno that referenced this pull request Nov 30, 2025
@nrako @claude
fix(node/net): return string family in server.address()
ebd08d6 
Aligns Deno's `server.address()` with Node.js v18.4.0+ behavior by
returning the `family` property as a string ("IPv4" or "IPv6") rather
than a number.

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).

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.

Refs:
- nodejs/node#43054
- nodejs/node@70b516e

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@nrako nrako mentioned this pull request Nov 30, 2025
Merged
nrako added a commit to nrako/deno that referenced this pull request Nov 30, 2025
@nrako @claude
fix(node/net): return string family in server.address()
618980d 
Aligns Deno's `server.address()` with Node.js v18.4.0+ behavior by
returning the `family` property as a string ("IPv4" or "IPv6") rather
than a number.

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).

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.

Refs:
- nodejs/node#43054
- nodejs/node@70b516e

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
nrako added a commit to nrako/deno that referenced this pull request Nov 30, 2025
@nrako @claude
fix(node/net): return string family in server.address()
6ba8a42 
Aligns Deno's `server.address()` with Node.js v18.4.0+ behavior by
returning the `family` property as a string ("IPv4" or "IPv6") rather
than a number.

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).

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.

Refs:
- nodejs/node#43054
- nodejs/node@70b516e

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
nrako added a commit to nrako/deno that referenced this pull request Nov 30, 2025
@nrako @claude
fix(node/net): return string family in server.address()
b7c45b3 
Aligns Deno's `server.address()` with Node.js v18.4.0+ behavior by
returning the `family` property as a string ("IPv4" or "IPv6") rather
than a number.

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).

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.

Refs:
- nodejs/node#43054
- nodejs/node@70b516e

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
nrako added a commit to nrako/deno that referenced this pull request Nov 30, 2025
@nrako @claude
fix(node/net): return string family in server.address()
fd3260a 
Aligns Deno's `server.address()` with Node.js v18.4.0+ behavior by
returning the `family` property as a string ("IPv4" or "IPv6") rather
than a number.

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).

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.

Refs:
- nodejs/node#43054
- nodejs/node@70b516e

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
nrako added a commit to nrako/deno that referenced this pull request Nov 30, 2025
@nrako @claude
fix(node/net): return string family in server.address()
7fe7995 
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>
nrako added a commit to nrako/deno that referenced this pull request Dec 4, 2025
@nrako @claude
fix(node/net): return string family in server.address()
6911007 
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>
Tango992 added a commit to denoland/deno that referenced this pull request Dec 6, 2025
@nrako @claude @Tango992
fix(node/net): return string family in server.address() (#31465)
bc9d356 
## 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
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).

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

- Modified `ext/node/polyfills/http.ts` to add `family` property to
`address()` return
- Modified `ext/node/polyfills/internal_binding/tcp_wrap.ts` to return
string `family`
instead of number in `getsockname()`, `getpeername()`, and `#connect()`
- Modified `ext/node/polyfills/net.ts` to fix `socket.remoteFamily`
getter (no longer
   needs conversion since `family` is now a string)
   - Modified `ext/node/polyfills/dns.ts` and
`ext/node/polyfills/internal/dns/promises.ts` to accept string family
values (`"IPv4"`,
`"IPv6"`) in `lookup()`, matching [Node.js
behavior](https://nodejs.org/api/dns.html#dnslookuphostname-options-callback)
- Added tests in `tests/unit_node/http_test.ts`,
`tests/unit_node/http2_test.ts`,
   `tests/unit_node/https_test.ts`, `tests/unit_node/dns_test.ts`, and
   `tests/unit_node/net_test.ts`

   ## Node.js Compatibility Note

For non-IP addresses (when `isIP()` returns 0), the `family` property is
`undefined`.
This matches Node.js C++ behavior in
[`AddressToJS`](https://github.com/nodejs/node/blob/main/src/tcp_wrap.cc)
where family
is only set for `AF_INET` (`"IPv4"`) and `AF_INET6` (`"IPv6"`), and left
undefined
   otherwise.

   ## Refs

   - nodejs/node#43054
   - nodejs/node@70b516e


<!--
Before submitting a PR, please read
https://docs.deno.com/runtime/manual/references/contributing

1. Give the PR a descriptive title.

  Examples of good title:
    - fix(std/http): Fix race condition in server
    - docs(console): Update docstrings
    - feat(doc): Handle nested reexports

  Examples of bad title:
    - fix #7123
    - update docs
    - fix bugs

2. Ensure there is a related issue and it is referenced in the PR text.
3. Ensure there are tests that cover the changes.
4. Ensure `cargo test` passes.
5. Ensure `./tools/format.js` passes without changing files.
6. Ensure `./tools/lint.js` passes.
7. Open as a draft PR if your work is still in progress. The CI won't
run
   all steps, but you can add '[ci]' to a commit message to force it to.
8. If you would like to run the benchmarks on the CI, add the 'ci-bench'
label.
-->

---------

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Daniel Rahmanto <daniel.rahmanto@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. tsc-agenda Issues and PRs to discuss during Technical Steering Committee meetings.