dns: remove `dns.lookup` and `dnsPromises.lookup` options type coercion by aduh95 · Pull Request #41431 · nodejs/node

aduh95 · 2022-01-07T16:26:44Z

No description provided.

ghost · 2022-01-07T16:26:48Z

Review requested:

@nodejs/net

ghost · 2022-04-03T17:29:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/43296/

ghost · 2022-04-03T19:39:17Z

CI: https://ci.nodejs.org/job/node-test-pull-request/43301/

ghost · 2022-04-03T19:55:22Z

@nodejs/tsc can we get this into v18.x?

ghost

LGTM if CITGM doesn't find problems.

ghost · 2022-04-03T20:18:49Z

I'm concerned about these two specific changes, moving from IPv4 to 4 and IPv6 to 6. I'm 100% sure those will break some of my modules (Fastify v4).

The reason for this change is that currently passing an address object to the DNS resolver, the 'IPv6' string is converted to 0.
Would it be reasonable to ask you to update the code that using that to expect one form or the other? Something like info.family === 6 || info.family === 'IPv6'? If not, we could make the DNS resolver accept a string and interpret 'IPv6' same as 6.

I assume this change will hurt the community a lot.

I personally would see this:

If not, we could make the DNS resolver accept a string and interpret 'IPv6' same as 6.

as the better solution

Agreed here. I realize that Node v18 was a major version change, but changing the output of a widely used API (socket.address()) just so another core-Node API could match up with it instead of making the DNS resolver more flexible seems pretty overkill!

Now that it's out in the wild, can confirm it caused some widespread breaking for us that's going to make node 18 a pain and very uncertain to roll out.

I assume this change will hurt the community a lot.

Can confirm completely unexpected breakage

@aduh95 @mcollina Any chance to think about that change again and consider alternatives?

maybe a new issue is more visible to the nodejs team and allows a better discussion ... so see #43014 ... Please "thumbs up" it if you support to adjust this to be backward compatible. Thank you

ghost

lgtm

ghost · 2022-04-04T13:36:29Z

CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2904/
CITGM (aduh95:no-dns-lookup-options-type-coercion): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2905/

ghost · 2022-04-06T22:14:45Z

CI: https://ci.nodejs.org/job/node-test-pull-request/43357/

ghost · 2022-04-08T10:11:46Z

CI: https://ci.nodejs.org/job/node-test-pull-request/43390/

ghost · 2022-04-08T14:06:16Z

@aduh95 If it helps the failures in the recent CI runs for this PR have all been running inside containers.

ghost · 2022-04-08T18:19:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/43400/

ghost · 2022-04-08T18:22:00Z

CI: https://ci.nodejs.org/job/node-test-pull-request/43401/

aduh95 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 7, 2022

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 Jan 7, 2022

VoltrexKeyva reviewed Jan 8, 2022

View reviewed changes

Comment thread doc/api/deprecations.md Resolved

aduh95 force-pushed the no-dns-lookup-options-type-coercion branch from 740a691 to 8e53d29 Compare April 3, 2022 17:08

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 3, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 3, 2022

aduh95 force-pushed the no-dns-lookup-options-type-coercion branch from 8e53d29 to 13c8788 Compare April 3, 2022 19:39

Trott approved these changes Apr 3, 2022

View reviewed changes

mcollina reviewed Apr 3, 2022

View reviewed changes

mcollina approved these changes Apr 4, 2022

View reviewed changes

aduh95 force-pushed the no-dns-lookup-options-type-coercion branch from 13c8788 to 84164de Compare April 4, 2022 13:31

mcollina added the notable-change PRs with changes that should be highlighted in changelogs. label Apr 4, 2022

aduh95 added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. author ready PRs with CI started, the required approvals, and no outstanding review comments. labels Apr 6, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 6, 2022

This was referenced Apr 7, 2022

CI Reliability 2022-04-07 nodejs/reliability#244

Open

CI Reliability 2022-04-08 nodejs/reliability#245

Open

aduh95 marked this pull request as draft April 8, 2022 10:11

IvanGoncharov mentioned this pull request Apr 25, 2022

test: fix failing integration tests on node@18 apollographql/apollo-server#6350

Merged

Apollon77 mentioned this pull request Apr 25, 2022

Error on Node 18 indutny/node-ip#113

Closed

This was referenced Apr 26, 2022

Fix #945 - Node.js v18 os.networkInterfaces()[].family number/string comparison homebridge/hap-nodejs#947

Merged

Fix #18 - Node.js v18 os.networkInterfaces()[].family number/string c… homebridge/ciao#25

Merged

miherlosev mentioned this pull request Apr 29, 2022

Testcafe doesn't work with node 18.0 DevExpress/testcafe#7000

Closed

bluwy mentioned this pull request Apr 29, 2022

fix: display network logs in node 18 sveltejs/kit#4778

Merged

3 of 5 tasks

panva mentioned this pull request May 3, 2022

os.networkInterfaces().family is no longer a string #42953

Closed

Apollon77 mentioned this pull request May 9, 2022

Revert the change of network interfaces family from String to Integer #43014

Closed

nodech mentioned this pull request May 11, 2022

Localhost lookup and default host settings handshake-org/hsd#722

Closed

JKRhb mentioned this pull request May 22, 2022

chore: update tested node versions to 14, 16, and 18 coapjs/node-coap#347

Merged

RaisinTen mentioned this pull request Sep 29, 2022

net: add autoSelectFamily option #44731

Merged

ksmithut mentioned this pull request Nov 2, 2022

Node 18 family parameter fails when passing url redis/ioredis#1672

Closed

climba03003 mentioned this pull request Sep 16, 2024

dns.lookup options..family throw errors when using 'IPv4' #54961

Closed

avivkeller mentioned this pull request Sep 16, 2024

doc: remove outdated sentence #54963

Closed

nrako mentioned this pull request Nov 30, 2025

fix(node/net): return string family in server.address() denoland/deno#31465

Merged

AugustinMauroy mentioned this pull request Mar 29, 2026

dns.lookup() and dnsPromises.lookup() options type coercion nodejs/userland-migrations#409

Open

Conversation

aduh95 commented Jan 7, 2022

Uh oh!

ghost commented Jan 7, 2022

Uh oh!

Uh oh!

ghost commented Apr 3, 2022

Uh oh!

ghost commented Apr 3, 2022

Uh oh!

ghost commented Apr 3, 2022

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost Apr 3, 2022

Choose a reason for hiding this comment

Uh oh!

ghost Apr 3, 2022

Choose a reason for hiding this comment

Uh oh!

ghost Apr 25, 2022 • edited by Apollon77 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Apr 28, 2022

Choose a reason for hiding this comment

Uh oh!

ghost May 5, 2022

Choose a reason for hiding this comment

Uh oh!

ghost May 7, 2022

Choose a reason for hiding this comment

Uh oh!

ghost May 7, 2022

Choose a reason for hiding this comment

Uh oh!

ghost May 9, 2022

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Apr 4, 2022

Uh oh!

ghost commented Apr 6, 2022

Uh oh!

ghost commented Apr 8, 2022

Uh oh!

ghost commented Apr 8, 2022

Uh oh!

ghost commented Apr 8, 2022

Uh oh!

ghost commented Apr 8, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

ghost Apr 25, 2022 •

edited by Apollon77

Loading