net: Support lookup of one address in net.LookupFunction (autoSelectFamily: false)

[lefebvree]

This PR updates the net.LookupFunction to accept lookup of a single address on Node 18+.

The net.LookupFunction was updated to only accept dns.LookupAllOptions options instead of dns.LookupOneOptions and a callback signature expecting an array of dns.LookupAddress:

• [@types/node] update 'net.LookupFunction' to reflect that it looks up all addresses instead of one #66031

DefinitelyTyped/types/node/net.d.ts

Lines 21 to 22 in 53a5da9

	options: dns.LookupAllOptions,
	callback: (err: NodeJS.ErrnoException | null, addresses: dns.LookupAddress[]) => void,

However even though the default autoSelectFamily was changed, the node implementation still supports the lookup of a single address with autoSelectFamily: false or globally with net.setDefaultAutoSelectFamily(false)

net.LookupFunction diverges from the dns.lookup one (which is typed to support both). This does not compile but should:

net.createConnection({
  autoSelectFamily: false, // expects callback with a single address
  port: 80,
  lookup: (hostname, options, callback) => {
    callback(null, '127.0.0.1', 4) // this should be accepted
  },
})

Node code to lookup a single address with `dns.lookup`

const net = require('net')
const dns = require('dns')

var socket = net.createConnection({
  host: 'google.com',
  port: 80,
  autoSelectFamily: false, // or net.setDefaultAutoSelectFamily(false)
  lookup: (hostname, options, callback) => {
    console.log('lookup options', options)
    dns.lookup(hostname, options, (...args) => {
      console.warn('lookup callback', args)
      callback(...args)
    })
  },
})
socket.on('connect', () => socket.write("GET / HTTP/1.0\r\n\r\n"))

lookup options { family: undefined, hints: 1024 } // ← all: true is not set
lookup callback [ null, '172.217.20.174', 4 ]     // ← callback with a single address and family

---

• Use a meaningful title for the pull request. Include the name of the package modified.
• Test the change in your own code. (Compile and run.)
• Add or edit tests to reflect the change.
• Follow the advice from the readme.
• Avoid common mistakes.
• Run pnpm test <package to test>.

If changing an existing definition:

• Provide a URL to documentation or source code which provides context for the suggested changes:
  • net.lookupAndConnect: looking up a single address
    The emitLookup callback expects a single ip and addressType
  • net.lookupAndConnect: looking up multiple addresses
  • Support for autoSelectFamily introduced in Node 18: net: add autoSelectFamily option nodejs/node#44731

[typescript-bot]

@lefebvree Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through.

This is a live comment which I will keep updated.

1 package in this PR

• node — on npm, on unpkg
  • owner-approval: @peterblazejewicz, @btoueg

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

You can test the changes of this PR in the Playground.

Status

• ✅ No merge conflicts
• ✅ Continuous integration tests have passed
• ✅ Most recent commit is approved by type definition owners or DT maintainers

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

---

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 68213,
  "author": "lefebvree",
  "headCommitOid": "9d23f07a7ec47f50719c04d9a1cc5ee620d6851c",
  "mergeBaseOid": "68a06dbd340bd1a6442f1d22d01ac071e7ec9dd1",
  "lastPushDate": "2024-01-16T13:03:06.000Z",
  "lastActivityDate": "2024-02-01T09:02:48.000Z",
  "maintainerBlessed": "Waiting for Code Reviews",
  "mergeOfferDate": "2024-01-31T18:41:28.000Z",
  "mergeRequestDate": "2024-02-01T09:02:48.000Z",
  "mergeRequestUser": "lefebvree",
  "hasMergeConflict": false,
  "isFirstContribution": true,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "node",
      "kind": "edit",
      "files": [
        {
          "path": "types/node/net.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/test/http.ts",
          "kind": "test"
        },
        {
          "path": "types/node/test/https.ts",
          "kind": "test"
        },
        {
          "path": "types/node/test/net.ts",
          "kind": "test"
        },
        {
          "path": "types/node/ts4.8/net.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/ts4.8/test/http.ts",
          "kind": "test"
        },
        {
          "path": "types/node/ts4.8/test/https.ts",
          "kind": "test"
        },
        {
          "path": "types/node/ts4.8/test/net.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v18/net.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v18/test/net.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v18/ts4.8/net.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v18/ts4.8/test/net.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "Microsoft",
        "jkomyno",
        "alvis",
        "r3nya",
        "btoueg",
        "smac89",
        "touffy",
        "DeividasBakanas",
        "eyqs",
        "Hannes-Magnusson-CK",
        "hoo29",
        "kjin",
        "ajafff",
        "islishude",
        "mwiktorczyk",
        "mohsen1",
        "n-e",
        "galkin",
        "parambirs",
        "eps1lon",
        "ThomasdenH",
        "WilcoBakker",
        "wwwy3y3",
        "samuela",
        "kuehlein",
        "bhongy",
        "chyzwar",
        "trivikr",
        "yoursunny",
        "qwelias",
        "ExE-Boss",
        "peterblazejewicz",
        "addaleax",
        "victorperin",
        "ZYSzys",
        "NodeJS",
        "LinusU",
        "wafuwafu13",
        "mcollina",
        "Semigradsky"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "peterblazejewicz",
      "date": "2024-01-31T19:11:27.000Z",
      "isMaintainer": true
    },
    {
      "type": "approved",
      "reviewer": "btoueg",
      "date": "2024-01-31T13:54:40.000Z",
      "isMaintainer": false
    },
    {
      "type": "approved",
      "reviewer": "chinthakagodawita",
      "date": "2024-01-24T06:43:47.000Z",
      "isMaintainer": false
    }
  ],
  "mainBotCommentID": 1893761751,
  "ciResult": "pass"
}

[typescript-bot]

🔔 @microsoft @jkomyno @alvis @r3nya @btoueg @smac89 @Touffy @DeividasBakanas @eyqs @Hannes-Magnusson-CK @hoo29 @kjin @ajafff @islishude @mwiktorczyk @mohsen1 @n-e @galkin @parambirs @eps1lon @ThomasdenH @WilcoBakker @wwwy3y3 @samuela @kuehlein @bhongy @chyzwar @trivikr @yoursunny @qwelias @ExE-Boss @peterblazejewicz @addaleax @victorperin @ZYSzys @nodejs @LinusU @wafuwafu13 @mcollina @Semigradsky — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

[Semigradsky]

cc @chinthakagodawita

[chinthakagodawita]

Ouch, I hadn't considered the case where autoSelectFamily was explicitly disabled!

One question about Node 16, but otherwise this change looks good to me :)

[chinthakagodawita]

I'm not sure this correct. From what I can see, "lookup multiple" only landed in Node 18+ with the advent of auto-select-family

Specifically Node 18.13:

• Release notes: https://nodejs.org/en/blog/release/v18.13.0
• PR: net: add autoSelectFamily option nodejs/node#44731

For reference, this is the Node 16 code: https://github.com/nodejs/node/blob/76f9a7d27f5ae9852b9ffc67a7dec1af503e0813/lib/net.js#L1107-L1195

[lefebvree]

You're right, thanks for catching that! I assumed net.LookupFunction and dns.lookup needed to match but that's not the case prior to Node 18, I amended the commit to leave v16 as it was.

[typescript-bot]

@chinthakagodawita Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

[chinthakagodawita]

LGTM 👍

[typescript-bot]

Re-ping @microsoft, @jkomyno, @alvis, @r3nya, @btoueg, @smac89, @Touffy, @DeividasBakanas, @eyqs, @Hannes-Magnusson-CK, @hoo29, @kjin, @ajafff, @islishude, @mwiktorczyk, @mohsen1, @n-e, @galkin, @parambirs, @eps1lon, @ThomasdenH, @WilcoBakker, @wwwy3y3, @samuela, @kuehlein, @bhongy, @chyzwar, @trivikr, @yoursunny, @qwelias, @ExE-Boss, @peterblazejewicz, @addaleax, @victorperin, @ZYSzys, @nodejs, @LinusU, @wafuwafu13, @mcollina, @Semigradsky:

This PR has been out for over a week, yet I haven't seen any reviews.

Could someone please give it some attention? Thanks!

[btoueg]

LGTM

[typescript-bot]

@lefebvree: Everything looks good here. I am ready to merge this PR (at 9d23f07) on your behalf whenever you think it's ready.

If you'd like that to happen, please post a comment saying:

Ready to merge

and I'll merge this PR almost instantly. Thanks for helping out! ❤️

(@microsoft, @jkomyno, @alvis, @r3nya, @btoueg, @smac89, @Touffy, @DeividasBakanas, @eyqs, @Hannes-Magnusson-CK, @hoo29, @kjin, @ajafff, @islishude, @mwiktorczyk, @mohsen1, @n-e, @galkin, @parambirs, @eps1lon, @ThomasdenH, @WilcoBakker, @wwwy3y3, @samuela, @kuehlein, @bhongy, @chyzwar, @trivikr, @yoursunny, @qwelias, @ExE-Boss, @peterblazejewicz, @addaleax, @victorperin, @ZYSzys, @nodejs, @LinusU, @wafuwafu13, @mcollina, @Semigradsky: you can do this too.)

[lefebvree]

Ready to merge