(deno_node/http.ts) `ClientRequest::_createUrlStrFromOptions` does not match the logic in nodejs

[jerry4718]

import { request } from 'node:http';

const options = {
    port: "8080",
    host: "192.168.0.2:8080",
    hostname: "192.168.0.2",
    method: "GET",
    headers: {},
    path: "/test"
};

const req = request(options, () => { console.log('success') });
req.on('error', console.error);
req.end();

It can be executed normally in nodejs, But such an error will be thrown in deno:

TypeError: Invalid URL: 'http://192.168.0.2:8080:8080/test'
    at getSerialization (ext:deno_url/00_url.js:74:11)
    at opUrlParse (ext:deno_url/00_url.js:65:10)
    at new URL (ext:deno_url/00_url.js:363:27)
    at new Request (ext:deno_fetch/23_request.js:323:25)
    at ext:deno_fetch/26_fetch.js:418:27
    at new Promise (<anonymous>)
    at fetch (ext:deno_fetch/26_fetch.js:414:18)
    at ClientRequest._final (ext:deno_node/http.ts:162:29)

I found that http://192.168.0.2:8080:8080/test was created by ClientRequest._createUrlStrFromOptions

[Sanskar531]

Hi, I investigated this issue and also compared with how node implements it. The problem is when the host field is specified the the port number is still be added to the string. But, host should already have the port number. Hence, a check can be done before port number is added to the string based on whether the host is defined or not and it should fix this issue.

I have a pr to fix this https://github.com/denoland/deno/pull/18258

[jerry4718]

Hi, I investigated this issue and also compared with how node implements it. The problem is when the host field is specified the the port number is still be added to the string. But, host should already have the port number. Hence, a check can be done before port number is added to the string based on whether the host is defined or not and it should fix this issue.

I have a pr to fix this #18258

During my testing of node:http, I found that its original implementation was that opts.hostname should have a higher priority than opts.host, and the priority of opts.port should also be higher than the :port existing in opts.host.

[kt3k]

It looks like this issue is already fixes as part of https://github.com/denoland/deno/commit/867a6d303285cdffd060e6bb4b0e97de73925cfe (which is available since v1.34). Please check the behavior again with the latest Deno when you have time