feat: refactor ProxyAgent constructor to also accept single URL argument (nodejs#2810)

* feat: add support for opts as URL in ProxyAgent

* test: update ProxyAgent unit tests

* docs: update ProxyAgent documentation

Author: rossilor95

docs/docs/api/ProxyAgent.md

@@ -16,7 +16,7 @@ Returns: `ProxyAgent`
 
 Extends: [`AgentOptions`](Agent.md#parameter-agentoptions)
 
-* **uri** `string` (required) - It can be passed either by a string or a object containing `uri` as string.
+* **uri** `string | URL` (required) - The URI of the proxy server.  This can be provided as a string, as an instance of the URL class, or as an object with a `uri` property of type string.
 * **token** `string` (optional) - It can be passed by a string of token for authentication.
 * **auth** `string` (**deprecated**) - Use token.
 * **clientFactory** `(origin: URL, opts: Object) => Dispatcher` (optional) - Default: `(origin, opts) => new Pool(origin, opts)`
@@ -30,6 +30,8 @@ import { ProxyAgent } from 'undici'
 
 const proxyAgent = new ProxyAgent('my.proxy.server')
 // or
+const proxyAgent = new ProxyAgent(new URL('my.proxy.server'))
+// or
 const proxyAgent = new ProxyAgent({ uri: 'my.proxy.server' })
 ```
 

lib/proxy-agent.js

@@ -19,55 +19,35 @@ function defaultProtocolPort (protocol) {
   return protocol === 'https:' ? 443 : 80
 }
 
-function buildProxyOptions (opts) {
-  if (typeof opts === 'string') {
-    opts = { uri: opts }
-  }
-
-  if (!opts || !opts.uri) {
-    throw new InvalidArgumentError('Proxy opts.uri is mandatory')
-  }
-
-  return {
-    uri: opts.uri,
-    protocol: opts.protocol || 'https'
-  }
-}
-
 function defaultFactory (origin, opts) {
   return new Pool(origin, opts)
 }
 
 class ProxyAgent extends DispatcherBase {
   constructor (opts) {
-    super(opts)
-    this[kProxy] = buildProxyOptions(opts)
-    this[kAgent] = new Agent(opts)
-    this[kInterceptors] = opts.interceptors?.ProxyAgent && Array.isArray(opts.interceptors.ProxyAgent)
-      ? opts.interceptors.ProxyAgent
-      : []
+    super()
 
-    if (typeof opts === 'string') {
-      opts = { uri: opts }
-    }
-
-    if (!opts || !opts.uri) {
-      throw new InvalidArgumentError('Proxy opts.uri is mandatory')
+    if (!opts || (typeof opts === 'object' && !(opts instanceof URL) && !opts.uri)) {
+      throw new InvalidArgumentError('Proxy uri is mandatory')
     }
 
     const { clientFactory = defaultFactory } = opts
-
     if (typeof clientFactory !== 'function') {
       throw new InvalidArgumentError('Proxy opts.clientFactory must be a function.')
     }
 
+    const url = this.#getUrl(opts)
+    const { href, origin, port, protocol, username, password } = url
+
+    this[kProxy] = { uri: href, protocol }
+    this[kAgent] = new Agent(opts)
+    this[kInterceptors] = opts.interceptors?.ProxyAgent && Array.isArray(opts.interceptors.ProxyAgent)
+      ? opts.interceptors.ProxyAgent
+      : []
     this[kRequestTls] = opts.requestTls
     this[kProxyTls] = opts.proxyTls
     this[kProxyHeaders] = opts.headers || {}
 
-    const resolvedUrl = new URL(opts.uri)
-    const { origin, port, username, password } = resolvedUrl
-
     if (opts.auth && opts.token) {
       throw new InvalidArgumentError('opts.auth cannot be used in combination with opts.token')
     } else if (opts.auth) {
@@ -81,7 +61,7 @@ class ProxyAgent extends DispatcherBase {
 
     const connect = buildConnector({ ...opts.proxyTls })
     this[kConnectEndpoint] = buildConnector({ ...opts.requestTls })
-    this[kClient] = clientFactory(resolvedUrl, { connect })
+    this[kClient] = clientFactory(url, { connect })
     this[kAgent] = new Agent({
       ...opts,
       connect: async (opts, callback) => {
@@ -138,6 +118,20 @@ class ProxyAgent extends DispatcherBase {
     )
   }
 
+  /**
+   * @param {import('../types/proxy-agent').ProxyAgent.Options | string | URL} opts
+   * @returns {URL}
+   */
+  #getUrl (opts) {
+    if (typeof opts === 'string') {
+      return new URL(opts)
+    } else if (opts instanceof URL) {
+      return opts
+    } else {
+      return new URL(opts.uri)
+    }
+  }
+
   async [kClose] () {
     await this[kAgent].close()
     await this[kClient].close()

test/proxy-agent.js

@@ -29,9 +29,10 @@ test('using auth in combination with token should throw', (t) => {
   )
 })
 
-test('should accept string and object as options', (t) => {
-  t = tspl(t, { plan: 2 })
+test('should accept string, URL and object as options', (t) => {
+  t = tspl(t, { plan: 3 })
   t.doesNotThrow(() => new ProxyAgent('http://example.com'))
+  t.doesNotThrow(() => new ProxyAgent(new URL('http://example.com')))
   t.doesNotThrow(() => new ProxyAgent({ uri: 'http://example.com' }))
 })
 
@@ -148,6 +149,47 @@ test('use proxy-agent to connect through proxy using path with params', async (t
   proxyAgent.close()
 })
 
+test('use proxy-agent to connect through proxy with basic auth in URL', async (t) => {
+  t = tspl(t, { plan: 7 })
+  const server = await buildServer()
+  const proxy = await buildProxy()
+
+  const serverUrl = `http://localhost:${server.address().port}`
+  const proxyUrl = new URL(`http://user:pass@localhost:${proxy.address().port}`)
+  const proxyAgent = new ProxyAgent(proxyUrl)
+  const parsedOrigin = new URL(serverUrl)
+
+  proxy.authenticate = function (req, fn) {
+    t.ok(true, 'authentication should be called')
+    fn(null, req.headers['proxy-authorization'] === `Basic ${Buffer.from('user:pass').toString('base64')}`)
+  }
+  proxy.on('connect', () => {
+    t.ok(true, 'proxy should be called')
+  })
+
+  server.on('request', (req, res) => {
+    t.strictEqual(req.url, '/hello?foo=bar')
+    t.strictEqual(req.headers.host, parsedOrigin.host, 'should not use proxyUrl as host')
+    res.setHeader('content-type', 'application/json')
+    res.end(JSON.stringify({ hello: 'world' }))
+  })
+
+  const {
+    statusCode,
+    headers,
+    body
+  } = await request(serverUrl + '/hello?foo=bar', { dispatcher: proxyAgent })
+  const json = await body.json()
+
+  t.strictEqual(statusCode, 200)
+  t.deepStrictEqual(json, { hello: 'world' })
+  t.strictEqual(headers.connection, 'keep-alive', 'should remain the connection open')
+
+  server.close()
+  proxy.close()
+  proxyAgent.close()
+})
+
 test('use proxy-agent with auth', async (t) => {
   t = tspl(t, { plan: 7 })
   const server = await buildServer()