fix(ClientRequest): suppress "ENETUNREACH" socket error

[mikicho]

I didn't add a test because we already tested this logic in the following:

1. emits the ENOTFOUND error connecting to a non-existing hostname given no mocked response
2. emits the ECONNREFUSED error connecting to an inactive server given no mocked response

[mikicho]

@kettanaito I encountered something interesting. I think there is a minor confusion between hostname and host in the getUrlByRequestOptions function,

AFAIK, the host is the one with the port, and the hostname is only the.. well.. the hostname.

I went ahead and made the required changes, but before I fix all the tests, (~7), please review the changes and let me know if I missed big time :)

[mikicho]

@kettanaito About the undefined, there is a test that expects the host to be localhost. How would you like to handle this?

12:43:31:468 [utils getUrlByRequestOptions] request options {}
12:43:31:469 [utils getUrlByRequestOptions] figuring out url from request options...
12:43:31:470 [utils getUrlByRequestOptions] protocol http:
12:43:31:470 [utils getUrlByRequestOptions] host undefined
12:43:31:470 [utils getUrlByRequestOptions] port undefined
12:43:31:470 [utils getUrlByRequestOptions] hostname undefined
12:43:31:470 [utils getUrlByRequestOptions] path /
12:43:31:470 [utils getUrlByRequestOptions] credentials undefined
12:43:31:470 [utils getUrlByRequestOptions] auth string:
12:43:31:470 [utils getUrlByRequestOptions] created url: "http://undefined/"
 ❯ src/utils/getUrlByRequestOptions.test.ts (12)
   ↓ returns a URL based on the basic RequestOptions [skipped]
   ↓ inherits protocol and port from http.Agent, if set [skipped]
   ↓ inherits protocol and port from https.Agent, if set [skipped]
   ↓ resolves protocol to "http" given no explicit protocol and no certificate [skipped]
   ↓ resolves protocol to "https" given no explicit protocol, but certificate [skipped]
   ↓ resolves protocol to "https" given no explicit protocol, but port is 443 [skipped]
   ↓ resolves protocol to "https" given no explicit protocol, but agent port is 443 [skipped]
   ↓ respects explicitly provided port [skipped]
   ↓ inherits "username" and "password" [skipped]
   × resolves host to localhost if none provided
   ↓ supports "host" instead of "hostname" and "port" [skipped]
   ↓ handles IPv6 host [skipped]

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Failed Tests 1 ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

 FAIL  src/utils/getUrlByRequestOptions.test.ts > resolves host to localhost if none provided
AssertionError: expected 'undefined' to be 'localhost' // Object.is equality
 ❯ src/utils/getUrlByRequestOptions.test.ts:105:43
    103|
    104| it.only('resolves host to localhost if none provided', () => {
    105|   expect(getUrlByRequestOptions({}).host).toBe('localhost')
       |                                           ^
    106| })
    107|

  - Expected   "localhost"
  + Received   "undefined"

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯

 Test Files  1 failed (1)
      Tests  1 failed | 11 skipped (12)
   Start at  12:43:30
   Duration  591ms (transform 80ms, setup 0ms, collect 80ms, tests 12ms)

[mikicho]

I went through all URL properties in the MDN site and tried to figure out which heuristic to use while parsing RequestOptions to URL using Node.js docs and source code.

TL;DR

const protocol = options.protocol || defaultProtocol
const hostname = options.hostname || options.host || 'localhost'
const port = options.port || agent.defaultPort || defaultPort
const path = options.path | | '/'
const portString = typeof port !== 'undefined' ? `:${port}` : ''
const url = new URL(`${protocol}//${hostname}${portString}${path}`)
url.username = credentials?.username || ''
url.password = credentials?.password || ''

*not "production ready"

Heuristic

host

It's important to notice that "URL hostname" != "URL host", but in Node.js they are the same:

Node docs - "Alias for host. To support url.parse(), hostname will be used if both host and hostname are specified."

URL host = URL hostname + ":" + URL port

hostname

hostname = RequestOptions.hostname || RequestOptions.host || 'localhost' (source)

port

RequestOptions host can not contain port. We shouldn't (can't) extract the port from the host

port = options.port || defaultPort || agent.defaultPort (source)

auth:

Authorization header || RequestOptions.auth (source)

protocol

options.protocol || defaultProtocol (source)

---

P.S: Assuming options always takes precedence over URL. Why do we figure out the URL from the options instead of the other way around? This way, NodeClientRequest would take only options and callback, and getUrlByRequestOptions would be redundant because we pass the options almost as-is.

@kettanaito WDYT?

[kettanaito]

@mikicho, thanks for looking into that! As long as the unit tests pass we can rewrite the options parsing logic but I would be extremely careful when considering changing tests. There's a convoluted edge case behind every test case and I'd rather we keep tests as-is, even if it seems like they make no sense (there are all sorts of usages in real world, some even incorrect according to Node.js docs but they are there and we must handle them).

Why do we figure out the URL from the options instead of the other way around?

Because there are third-party packages that use ClientRequest in every imaginable way. I've seen scenarios with partial URL object + RequestOptions that compliment each other. In those scenarios, neither can be taken as the complete source of truth as Node itself merges the two to produce the complete request options. That's why we try to take URL as a primary input (the first argument) but still check if there are any options that'd modify it.

[kettanaito]

If we can reproduce the ENETUNREACH error with a non-IPv6 address, then I highly recommend we extract the actual fix from this pull request and open a new one designated for that. I hope we don't have to refactor the options parsing logic to have that network error suppressed.

We can keep the current pull request aimed at improving IPv6 support specifically and any related refactoring to options parsing.

[mikicho]

@kettanaito I agree. IIUC, the test now should be in the NodeClientRequest.test.ts, so we don't need to bother with the URL options now.

I did a force push, for future reference, the commit I overridden was: f0ada9663ce02c27d98d50f3ad20bd2ce1f52dac

[kettanaito]

@mikicho, simply fantastic! Giving this a quick review and will merge if there's no comments from my side.

[kettanaito]

Released: v0.25.7 🎉

This has been released in v0.25.7!

• 📄 Release notes
• 📦 npm package

Make sure to always update to the latest version (npm i @mswjs/interceptors@latest) to get the newest features and bug fixes.

---

Predictable release automation by @ossjs/release.