Merge pull request #37 from dnlup/fix-url

Full url support

Author: dnlup

README.md

@@ -6,7 +6,7 @@
 
 > A simple pool manager for [`undici`](https://github.com/nodejs/undici).
 
-You might find this module useful if you are using [`undici`](https://github.com/nodejs/undici) and need to manage connections to different and unknown hosts.
+You might find this module useful if you use [`undici`](https://github.com/nodejs/undici) and need to manage connections to different and unknown hosts.
 
 `agent-11` controls [`undici`'s](https://github.com/nodejs/undici) pool connections to different hosts. Each time you request a new one, it creates a new pool.
 If you don't request this connection after a certain amount of time, `agent-11` will close it.
@@ -20,6 +20,8 @@ If you don't request this connection after a certain amount of time, `agent-11`
 - [Usage](#usage)
 - [API](#api)
   * [Class: `Agent11`](#class-agent11)
+    + [Static method: `Agent11.urlToObject(url)`](#static-method-agent11urltoobjecturl)
+    + [Static method: `Agent11.getKey(url[, options])`](#static-method-agent11getkeyurl-options)
     + [new `Agent11([options])`](#new-agent11options)
     + [`agent.getConnection(url, [options])`](#agentgetconnectionurl-options)
     + [`agent.close()`](#agentclose)
@@ -84,6 +86,19 @@ agent.destroy(new Error('no more!')).then().catch(console.error)
 
 It manages `undici`'s pool connections.
 
+#### Static method: `Agent11.urlToObject(url)`
+
+* `url` `<string||URL|Object>`: the url to convert.
+* Returns: `<Object>` A url-like object with the properties `protocol`, `hostname` and `port`.
+
+#### Static method: `Agent11.getKey(url[, options])`
+
+* `url` `<Object>`: a url-like object.
+* `options` `<Object>`: connection options. See [undici documentation](https://github.com/nodejs/undici#new-undiciclienturl-opts).
+* Returns: `<string>`: the key that maps the url.
+
+This method creates a key that maps a connection pool to a url.
+
 #### new `Agent11([options])`
 
 * `options` `<Object>`
@@ -131,3 +146,4 @@ $ npm lint
 # Create the TOC in the README
 $ npm run doc
 ```
+

benchmarks/bench.js

@@ -7,8 +7,17 @@ const suite = new Benchmark.Suite()
 const agent = new Agent11({
   destroyTimeout: 1e9
 })
-const urlString = 'http://localhost:3000/'
-const urlObject = new URL('http://localhost:3000')
+const urlString = 'http://localhost:3000/some/path'
+const urlObject = {
+  protocol: 'http:',
+  hostname: 'localhost',
+  port: 3000,
+  pathname: '/some/path'
+}
+const url = new URL('http://localhost:3000/some/path')
+
+// setup the connection first
+agent.getConnection(urlString)
 
 /* eslint-disable */
 suite
@@ -18,6 +27,9 @@ suite
   .add('Agent11.getConnection(<object>)', function getConnectionFromObject() {
     const connection = agent.getConnection(urlObject)
   })
+  .add('Agent11.getConnection(<URL>)', function getConnectionFromObject() {
+    const connection = agent.getConnection(url)
+  })
   .on('cycle', event => console.log(String(event.target)))
   .on('complete', function onComplete() {
     agent.close().catch()

index.js

@@ -12,10 +12,7 @@ const {
   kClose,
   kRefresh,
   kStore,
-  kTimersMap,
-  kGetKey,
-  kGetKeyFromString,
-  kGetKeyFromObject
+  kTimersMap
 } = require('./symbols')
 
 const noop = () => {}
@@ -47,6 +44,43 @@ class Agent11 {
     this[kTimersMap] = new Map()
   }
 
+  static urlToObject (url) {
+    if (typeof url === 'string' && url.length) {
+      const match = URL_REG.exec(url)
+      return {
+        protocol: match && match[1],
+        hostname: match && match[2],
+        port: match && match[3]
+      }
+    // perf: this branch has polymorphic inline caches
+    } else if (typeof url === 'object' && url !== null) {
+      return {
+        protocol: url.protocol,
+        hostname: url.hostname,
+        port: url.port
+      }
+    }
+    throw new TypeError(`Invalid url, received: ${url}`)
+  }
+
+  static getKey (url, options) {
+    let key = url.protocol || 'http:'
+    // perf: this part has polymorphic inline caches
+    if (key.charAt(key.length - 1) !== ':') {
+      key += ':'
+    }
+    key += url.hostname
+    if ((typeof url.port === 'string' && url.port.length) || typeof url.port === 'number') {
+      key += ':'
+      key += url.port
+    }
+    if (options && options.socketPath) {
+      key += ':'
+      key += options.socketPath
+    }
+    return key
+  }
+
   [kSetupConnection] (url, options) {
     if (this.size === this[kMaxHosts]) {
       throw new Error(`Maximum number of ${this[kMaxHosts]} hosts reached`)
@@ -81,46 +115,7 @@ class Agent11 {
     return this[kHostsMap].size
   }
 
-  [kGetKeyFromString] (url) {
-    const match = URL_REG.exec(url)
-    return this[kGetKeyFromObject]({
-      protocol: match && match[1],
-      hostname: match && match[2],
-      port: match && match[3]
-    })
-  }
-
-  [kGetKeyFromObject] (url) {
-    let key = url.protocol || 'http:'
-    if (key.charAt(key.length - 1) !== ':') {
-      key += ':'
-    }
-    key += url.hostname
-    if (typeof url.port === 'string' || typeof url.port === 'number') {
-      key += ':'
-      key += url.port
-    }
-    return key
-  }
-
-  [kGetKey] (url, options) {
-    let key = ''
-    if (typeof url === 'string') {
-      key = this[kGetKeyFromString](url)
-    } else if (typeof url === 'object' && url !== null) {
-      key = this[kGetKeyFromObject](url)
-    } else {
-      throw new TypeError(`Can't get key from url: '${url}'`)
-    }
-    if (options && options.socketPath) {
-      key += ':'
-      key += options.socketPath
-    }
-    return key
-  }
-
-  getConnection (url, options) {
-    const key = this[kGetKey](url, options)
+  connection (key, url, options) {
     if (this[kHostsMap].has(key)) {
       const pool = this[kHostsMap].get(key)
       this[kRefresh](pool)
@@ -132,6 +127,12 @@ class Agent11 {
     }
   }
 
+  getConnection (url, options) {
+    url = Agent11.urlToObject(url)
+    const key = Agent11.getKey(url, options)
+    return this.connection(key, url, options)
+  }
+
   close () {
     const closing = []
     for (const [key, pool] of this[kHostsMap]) {

symbols.js

@@ -10,8 +10,5 @@ module.exports = {
   kClose: Symbol('kClose'),
   kRefresh: Symbol('kRefresh'),
   kStore: Symbol('kStore'),
-  kTimersMap: Symbol('kTimersMap'),
-  kGetKey: Symbol('kGetKey'),
-  kGetKeyFromString: Symbol('kGetKeyFromString'),
-  kGetKeyFromObject: Symbol('kGetKeyFromObject')
+  kTimersMap: Symbol('kTimersMap')
 }

test.js

@@ -3,7 +3,6 @@
 const { test } = require('tap')
 const { promisify } = require('util')
 const Agent11 = require('./')
-const { kGetKey } = require('./symbols')
 
 const sleep = promisify(setTimeout)
 
@@ -41,89 +40,121 @@ test('invalid options', t => {
   t.end()
 })
 
-test('kGetKey from url and options', t => {
+test('Agent11.urlToObject', t => {
+  let obj = Agent11.urlToObject('http://example.com:3444/some/path?q=1')
+  t.same(obj, {
+    protocol: 'http:',
+    hostname: 'example.com',
+    port: '3444'
+  })
+  obj = Agent11.urlToObject(new URL('http://example.com:3444/some/path?q=1'))
+  t.same(obj, {
+    protocol: 'http:',
+    hostname: 'example.com',
+    port: '3444'
+  })
+  obj = Agent11.urlToObject('https://example.com/some/path?q=1')
+  t.same(obj, {
+    protocol: 'https:',
+    hostname: 'example.com',
+    port: undefined
+  })
+  obj = Agent11.urlToObject('example.com/some/path?q=1')
+  t.same(obj, {
+    protocol: undefined,
+    hostname: 'example.com',
+    port: undefined
+  })
+  t.end()
+})
+
+test('Agent11.getKey from url and options', t => {
+  // TODO: convert this list in sequential manual tests, lists are harder to debug.
   const list = [
     {
       opts: [
-        new URL('http://localhost:3333')
+        {
+          protocol: 'http:',
+          hostname: 'example.com',
+          port: '3000'
+        }
       ],
-      expected: 'http:localhost:3333'
+      expected: 'http:example.com:3000'
     },
     {
       opts: [
-        { hostname: 'localhost' }
+        {
+          protocol: 'http:',
+          hostname: 'example.com',
+          port: 3000
+        }
       ],
-      expected: 'http:localhost'
+      expected: 'http:example.com:3000'
     },
     {
       opts: [
         {
-          protocol: 'http',
-          hostname: 'localhost'
+          protocol: 'http:',
+          hostname: 'example.com',
+          port: 0
         }
       ],
-      expected: 'http:localhost'
+      expected: 'http:example.com:0'
     },
     {
       opts: [
-        new URL('https://localhost:3400'),
-        {
-          socketPath: '/tmp/agent-11/agent.sock'
-        }
+        { hostname: 'example.com' }
       ],
-      expected: 'https:localhost:3400:/tmp/agent-11/agent.sock'
+      expected: 'http:example.com'
     },
     {
       opts: [
-        'example.com/1/2/3?some=false'
+        new URL('http://example.com')
       ],
       expected: 'http:example.com'
     },
     {
       opts: [
-        'https://example.some.com/1/2/3?some=false',
+        {
+          protocol: 'https:',
+          hostname: 'example.com',
+          port: 3400
+        },
         {
           socketPath: '/tmp/agent-11/agent.sock'
         }
       ],
-      expected: 'https:example.some.com:/tmp/agent-11/agent.sock'
+      expected: 'https:example.com:3400:/tmp/agent-11/agent.sock'
     },
     {
       opts: [
-        'localhost:3000/1/2/3?some=false'
-      ],
-      expected: 'http:localhost:3000'
-    },
-    {
-      opts: [
-        'https://localhost:3000/1/2/3?some=false',
+        new URL('https://example.com:3400'),
         {
           socketPath: '/tmp/agent-11/agent.sock'
         }
       ],
-      expected: 'https:localhost:3000:/tmp/agent-11/agent.sock'
+      expected: 'https:example.com:3400:/tmp/agent-11/agent.sock'
     },
     {
       opts: [
         {
-          hostname: 'some.com',
-          port: 0
+          protocol: 'http',
+          hostname: 'example.com',
+          pathname: '/some/path?q=1'
         }
       ],
-      expected: 'http:some.com:0'
+      expected: 'http:example.com'
     },
     {
       opts: [
-        'some.com:0'
+        new URL('http://example.com/some/path?q=1')
       ],
-      expected: 'http:some.com:0'
+      expected: 'http:example.com'
     }
   ]
 
-  const agent = new Agent11()
-
   for (const [index, item] of list.entries()) {
-    const key = agent[kGetKey](...item.opts)
+    const key = Agent11.getKey(...item.opts)
     t.is(key, item.expected, `list item ${index}`)
   }
   t.end()
@@ -169,6 +200,24 @@ test('getConnection with a unix socket', t => {
   t.end()
 })
 
+test('getConnection should return the same pool', t => {
+  const agent = new Agent11()
+  t.teardown(() => agent.close())
+  const string = 'http://xyz.xyz/some/path?q=1'
+  const obj = {
+    protocol: 'http:',
+    hostname: 'xyz.xyz'
+  }
+  const url = new URL('http://xyz.xyz/some/other/path?q=2')
+  const options = {
+    socketPath: '/tmp/agent-11/agent.sock'
+  }
+  const pool = agent.getConnection(string, options)
+  t.is(pool, agent.getConnection(obj, options))
+  t.is(pool, agent.getConnection(url, options))
+  t.end()
+})
+
 test('getConnection should error if max hosts is reached', t => {
   const agent = new Agent11({ maxHosts: 1 })
   t.teardown(() => agent.close())
@@ -184,9 +233,9 @@ test('getConnection should error if the url is invalid', t => {
   const agent = new Agent11()
   t.teardown(() => agent.close())
   let error = t.throws(() => agent.getConnection(null))
-  t.is(error.message, 'Can\'t get key from url: \'null\'')
+  t.is(error.message, 'Invalid url, received: null')
   error = t.throws(() => agent.getConnection(''))
-  t.is(error.message, 'Invalid URL: ')
+  t.is(error.message, 'Invalid url, received: ')
   error = t.throws(() => agent.getConnection({}))
   t.is(error.message, 'invalid protocol')
   t.end()