Skip to content

fix: remove deprecated url.parse #1129

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dflynn15 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: remove deprecated url.parse #1129

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions lib/utils/http_request_handler/request_handler.node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
*/
import http from 'http';
import https from 'https';
import url from 'url';
import { AbortableRequest, Headers, RequestHandler, Response } from './http';
import decompressResponse from 'decompress-response';
import { LoggerFacade } from '../../logging/logger';
Expand All @@ -27,7 +26,6 @@ import { Platform } from '../../platform_support';
* Handles sending requests and receiving responses over HTTP via NodeJS http module
*/


export class NodeRequestHandler implements RequestHandler {
private readonly logger?: LoggerFacade;
private readonly timeout: number;
Expand All @@ -46,7 +44,7 @@ export class NodeRequestHandler implements RequestHandler {
* @returns AbortableRequest contains both the response Promise and capability to abort()
*/
makeRequest(requestUrl: string, headers: Headers, method: string, data?: string): AbortableRequest {
const parsedUrl = url.parse(requestUrl);
const parsedUrl = new URL(requestUrl);

if (parsedUrl.protocol !== 'https:' && parsedUrl.protocol !== 'http:') {
return {
Expand All @@ -62,7 +60,7 @@ export class NodeRequestHandler implements RequestHandler {
headers: {
...headers,
'accept-encoding': 'gzip,deflate',
'content-length': String(data?.length || 0)
'content-length': String(data?.length || 0),
},
timeout: this.timeout,
});
Expand All @@ -82,11 +80,11 @@ export class NodeRequestHandler implements RequestHandler {
* @private
* @returns http.RequestOptions Standard request options dictionary compatible with both http and https
*/
private getRequestOptionsFromUrl(url: url.UrlWithStringQuery): http.RequestOptions {
private getRequestOptionsFromUrl(url: URL): http.RequestOptions {
return {
hostname: url.hostname,
path: url.path,
port: url.port,
path: url.pathname + url.search,
port: url.port || null,
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

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

The port handling logic is incorrect. The WHATWG URL API's port property returns an empty string ("") when no explicit port is specified, not a falsy value. Using url.port || null will incorrectly set the port to null even when a valid port exists if the port is "0", and will correctly handle empty strings but for the wrong reason.

The correct approach is to check for an empty string explicitly: url.port ? url.port : null or url.port !== '' ? url.port : null. This ensures that:

  • Valid ports (including "0") are passed through
  • Empty strings (no explicit port) become null
  • The http.RequestOptions receives the appropriate value
Suggested change
port: url.port || null,
port: url.port !== '' ? url.port : null,

Copilot uses AI. Check for mistakes.
protocol: url.protocol,
};
}
Expand Down