fix: Align arguments of connect() in node:tls and node:net to Node (#…

…10870)

Co-authored-by: dave caruso <me@paperdave.net>

packages/bun-types/overrides.d.ts

@@ -61,4 +61,11 @@ declare module "tls" {
   }
 
   function connect(options: BunConnectionOptions, secureConnectListener?: () => void): TLSSocket;
+  function connect(path: string, options?: BunConnectionOptions, secureConnectListener?: () => void): TLSSocket;
+  function connect(
+    port: number,
+    host?: string,
+    options?: BunConnectionOptions,
+    secureConnectListener?: () => void,
+  ): TLSSocket;
 }

src/js/node/net.ts

@@ -422,66 +422,49 @@ const Socket = (function (InternalSocket) {
       process.nextTick(closeNT, connection);
     }
 
-    connect(port, host, connectListener) {
-      var path;
+    connect(...args) {
+      const [options, callback] = normalizeArgs(args);
       var connection = this.#socket;
       var _checkServerIdentity = undefined;
-      if (typeof port === "string") {
-        path = port;
-        port = undefined;
 
-        if (typeof host === "function") {
-          connectListener = host;
-          host = undefined;
-        }
-      } else if (typeof host == "function") {
-        if (typeof port === "string") {
-          path = port;
-          port = undefined;
-        }
-
-        connectListener = host;
-        host = undefined;
+      var {
+        fd,
+        port,
+        host,
+        path,
+        socket,
+        // TODOs
+        localAddress,
+        localPort,
+        family,
+        hints,
+        lookup,
+        noDelay,
+        keepAlive,
+        keepAliveInitialDelay,
+        requestCert,
+        rejectUnauthorized,
+        pauseOnConnect,
+        servername,
+        checkServerIdentity,
+        session,
+      } = options;
+
+      _checkServerIdentity = checkServerIdentity;
+      this.servername = servername;
+      if (socket) {
+        connection = socket;
       }
-      if (typeof port == "object") {
-        var {
+      if (fd) {
+        bunConnect({
+          data: this,
           fd,
-          port,
-          host,
-          path,
-          socket,
-          // TODOs
-          localAddress,
-          localPort,
-          family,
-          hints,
-          lookup,
-          noDelay,
-          keepAlive,
-          keepAliveInitialDelay,
-          requestCert,
-          rejectUnauthorized,
-          pauseOnConnect,
-          servername,
-          checkServerIdentity,
-          session,
-        } = port;
-        _checkServerIdentity = checkServerIdentity;
-        this.servername = servername;
-        if (socket) {
-          connection = socket;
-        }
-        if (fd) {
-          bunConnect({
-            data: this,
-            fd,
-            socket: this.#handlers,
-            tls,
-          }).catch(error => {
-            this.emit("error", error);
-            this.emit("close");
-          });
-        }
+          socket: this.#handlers,
+          tls,
+        }).catch(error => {
+          this.emit("error", error);
+          this.emit("close");
+        });
       }
 
       this.pauseOnConnect = pauseOnConnect;
@@ -536,8 +519,8 @@ const Socket = (function (InternalSocket) {
         this._secureEstablished = false;
         this._securePending = true;
 
-        if (connectListener) this.on("secureConnect", connectListener);
-      } else if (connectListener) this.on("connect", connectListener);
+        if (callback) this.on("secureConnect", callback);
+      } else if (callback) this.on("connect", callback);
 
       // start using existing connection
       try {
@@ -740,13 +723,36 @@ const Socket = (function (InternalSocket) {
   },
 );
 
-function createConnection(port, host, connectListener) {
-  if (typeof port === "object") {
-    // port is option pass Socket options and let connect handle connection options
-    return new Socket(port).connect(port, host, connectListener);
+// we gotta handle the different signatures that nodejs tls.connect accepts
+// connect(options[, callback])
+// connect(path[, callback])
+// connect(port[, host][, callback])
+function normalizeArgs(args) {
+  if (args.length === 0) {
+    return [{}, null];
+  }
+
+  const [arg0, arg1] = args;
+  let options = {} as any;
+  if (typeof arg0 === "object" && arg0 !== null) {
+    options = arg0;
+  } else if (typeof arg0 === "string") {
+    options.path = arg0;
+  } else {
+    options.port = arg0;
+    if (typeof arg1 === "string") {
+      options.host = arg1;
+    }
   }
-  // port is path or host, let connect handle this
-  return new Socket().connect(port, host, connectListener);
+
+  const connectListener = typeof args[args.length - 1] === "function" ? args[args.length - 1] : null;
+
+  return [options, connectListener];
+}
+
+function createConnection(...args) {
+  const [options, callback] = normalizeArgs(args);
+  return new Socket(options).connect(options, callback);
 }
 
 const connect = createConnection;
@@ -1027,7 +1033,7 @@ export default {
   isIPv6,
   Socket,
   [Symbol.for("::bunternal::")]: SocketClass,
-
+  _normalizeArgs: normalizeArgs,
   getDefaultAutoSelectFamily: $zig("node_net_binding.zig", "getDefaultAutoSelectFamily"),
   setDefaultAutoSelectFamily: $zig("node_net_binding.zig", "setDefaultAutoSelectFamily"),
   getDefaultAutoSelectFamilyAttemptTimeout: $zig("node_net_binding.zig", "getDefaultAutoSelectFamilyAttemptTimeout"),

src/js/node/tls.ts

@@ -314,7 +314,7 @@ const TLSSocket = (function (InternalTLSSocket) {
     enumerable: false,
   });
   function Socket(options) {
-    return new InternalTLSSocket(options);
+    return new InternalTLSSocket(options.socket, options);
   }
   Socket.prototype = InternalTLSSocket.prototype;
   return Object.defineProperty(Socket, Symbol.hasInstance, {
@@ -652,9 +652,22 @@ class Server extends NetServer {
   }
 }
 
+function normalizeConnectArgs(args) {
+  let [options, callback] = net._normalizeArgs(args);
+
+  if (args[1] !== null && typeof args[1] === "object") {
+    Object.assign(options, args[1]);
+  } else if (args[2] !== null && typeof args[2] === "object") {
+    Object.assign(options, args[2]);
+  }
+
+  return [options, callback];
+}
+
 function createServer(options, connectionListener) {
   return new Server(options, connectionListener);
 }
+
 const DEFAULT_ECDH_CURVE = "auto",
   // https://github.com/Jarred-Sumner/uSockets/blob/fafc241e8664243fc0c51d69684d5d02b9805134/src/crypto/openssl.c#L519-L523
   DEFAULT_CIPHERS =
@@ -674,7 +687,33 @@ const DEFAULT_ECDH_CURVE = "auto",
     // port is path or host, let connect handle this
     return new TLSSocket().connect(port, host, connectListener);
   },
-  connect = createConnection;
+  connect = (...args) => {
+    let [options, callback] = normalizeConnectArgs(args);
+    if (options.ALPNProtocols) {
+      convertALPNProtocols(options.ALPNProtocols, options);
+    }
+
+    options = {
+      rejectUnauthorized: rejectUnauthorizedDefault,
+      ciphers: DEFAULT_CIPHERS,
+      checkServerIdentity,
+      ...options,
+    };
+
+    // @ts-ignore
+    const tlsSocket = new TLSSocket(options);
+
+    // Node.js connects the socket right away if no options.socket is provided. For compatibility, I guess we should do the same.
+    if (!options.socket) {
+      tlsSocket.connect(options, callback);
+    }
+
+    if (options.servername && !net.isIP(options.servername)) {
+      tlsSocket.setServername(options.servername);
+    }
+
+    return tlsSocket;
+  };
 
 function getCiphers() {
   return DEFAULT_CIPHERS.split(":");

test/js/node/net/node-net.test.ts

@@ -393,8 +393,6 @@ describe("net.Socket write", () => {
     });
 
     const socket = new Socket();
-    socket.on("data", data => console.log(data.toString()));
-    socket.on("error", err => console.error(err));
 
     async function run() {
       return new Promise((resolve, reject) => {
@@ -418,7 +416,7 @@ it("should handle connection error", done => {
   let errored = false;
 
   // @ts-ignore
-  const socket = connect(55555, () => {
+  const socket = connect(55555, "localhost", () => {
     done(new Error("Should not have connected"));
   });
 

test/js/node/tls/node-tls-connect.test.ts

@@ -2,6 +2,8 @@ import tls, { TLSSocket, connect, checkServerIdentity, createServer, Server } fr
 import { join } from "path";
 import { AddressInfo } from "ws";
 
+const symbolConnectOptions = Symbol.for("::buntlsconnectoptions::");
+
 it("should work with alpnProtocols", done => {
   try {
     let socket: TLSSocket | null = connect({
@@ -210,3 +212,81 @@ it("should have checkServerIdentity", async () => {
   expect(checkServerIdentity).toBeFunction();
   expect(tls.checkServerIdentity).toBeFunction();
 });
+
+// Test using only options
+it("should process options correctly when connect is called with only options", done => {
+  let socket = connect({
+    port: 443,
+    host: "bun.sh",
+    rejectUnauthorized: false,
+  });
+
+  socket.on("secureConnect", () => {
+    expect(socket.remotePort).toBe(443);
+    expect(socket[symbolConnectOptions].serverName).toBe("bun.sh");
+    socket.end();
+    done();
+  });
+
+  socket.on("error", err => {
+    socket.end();
+    done(err);
+  });
+});
+
+// Test using port and host
+it("should process port and host correctly", done => {
+  let socket = connect(443, "bun.sh", {
+    rejectUnauthorized: false,
+  });
+
+  socket.on("secureConnect", () => {
+    expect(socket.remotePort).toBe(443);
+    expect(socket[symbolConnectOptions].serverName).toBe("bun.sh");
+    socket.end();
+    done();
+  });
+
+  socket.on("error", err => {
+    socket.end();
+    done(err);
+  });
+});
+
+// Test using port, host, and callback
+it("should process port, host, and callback correctly", done => {
+  let socket = connect(
+    443,
+    "bun.sh",
+    {
+      rejectUnauthorized: false,
+    },
+    () => {
+      expect(socket.remotePort).toBe(443);
+      expect(socket[symbolConnectOptions].serverName).toBe("bun.sh");
+      socket.end();
+      done();
+    },
+  ).on("error", err => {
+    done(err);
+  });
+});
+
+// Additional tests to ensure the callback is optional and handled correctly
+it("should handle the absence of a callback gracefully", done => {
+  let socket = connect(443, "bun.sh", {
+    rejectUnauthorized: false,
+  });
+
+  socket.on("secureConnect", () => {
+    expect(socket[symbolConnectOptions].serverName).toBe("bun.sh");
+    expect(socket.remotePort).toBe(443);
+    socket.end();
+    done();
+  });
+
+  socket.on("error", err => {
+    socket.end();
+    done(err);
+  });
+});