refactor: replace internal redirects with high-level ones

docs/api/Agent.md

@@ -19,7 +19,6 @@ Returns: `Agent`
 Extends: [`PoolOptions`](Pool.md#parameter-pooloptions)
 
 * **factory** `(origin: URL, opts: Object) => Dispatcher` - Default: `(origin, opts) => new Pool(origin, opts)`
-* **maxRedirections** `Integer` - Default: `0`. The number of HTTP redirection to follow unless otherwise specified in `DispatchOptions`.
 
 ## Instance Properties
 

lib/agent.js

@@ -3,15 +3,13 @@
 const { InvalidArgumentError } = require('./core/errors')
 const { kClients, kRunning, kClose, kDestroy, kDispatch } = require('./core/symbols')
 const DispatcherBase = require('./dispatcher-base')
-const RedirectHandler = require('./handler/RedirectHandler')
 const Pool = require('./pool')
 const Client = require('./client')
 const util = require('./core/util')
 
 const kOnConnect = Symbol('onConnect')
 const kOnDisconnect = Symbol('onDisconnect')
 const kOnConnectionError = Symbol('onConnectionError')
-const kMaxRedirections = Symbol('maxRedirections')
 const kOnDrain = Symbol('onDrain')
 const kFactory = Symbol('factory')
 const kOptions = Symbol('options')
@@ -23,7 +21,7 @@ function defaultFactory (origin, opts) {
 }
 
 class Agent extends DispatcherBase {
-  constructor ({ factory = defaultFactory, maxRedirections = 0, connect, ...options } = {}) {
+  constructor ({ factory = defaultFactory, connect, ...options } = {}) {
     super()
 
     if (typeof factory !== 'function') {
@@ -34,16 +32,11 @@ class Agent extends DispatcherBase {
       throw new InvalidArgumentError('connect must be a function or an object')
     }
 
-    if (!Number.isInteger(maxRedirections) || maxRedirections < 0) {
-      throw new InvalidArgumentError('maxRedirections must be a positive number')
-    }
-
     if (connect && typeof connect !== 'function') {
       connect = { ...connect }
     }
 
     this[kOptions] = { ...util.deepClone(options), connect }
-    this[kMaxRedirections] = maxRedirections
     this[kFactory] = factory
     this[kClients] = new Map()
 
@@ -95,18 +88,6 @@ class Agent extends DispatcherBase {
       this[kClients].set(key, dispatcher)
     }
 
-    if (this[kMaxRedirections] > 0) {
-      return dispatcher.dispatch(
-        opts,
-        new RedirectHandler(
-          dispatcher.dispatch.bind(dispatcher),
-          this[kMaxRedirections],
-          opts,
-          handler
-        )
-      )
-    }
-
     return dispatcher.dispatch(opts, handler)
   }
 

lib/api/api-connect.js

@@ -3,6 +3,7 @@
 const { AsyncResource } = require('node:async_hooks')
 const { InvalidArgumentError, RequestAbortedError, SocketError } = require('../core/errors')
 const util = require('../core/util')
+const RedirectHandler = require('../handler/RedirectHandler')
 const { addSignal, removeSignal } = require('./abort-signal')
 
 class ConnectHandler extends AsyncResource {
@@ -91,6 +92,16 @@ function connect (opts, callback) {
 
   try {
     const connectHandler = new ConnectHandler(opts, callback)
+
+    if (opts?.maxRedirections != null && (!Number.isInteger(opts?.maxRedirections) || opts?.maxRedirections < 0)) {
+      throw new InvalidArgumentError('maxRedirections must be a positive number')
+    }
+
+    if (opts?.maxRedirections > 0) {
+      RedirectHandler.buildDispatch(this, opts.maxRedirections)(opts, connectHandler)
+      return
+    }
+
     this.dispatch({ ...opts, method: 'CONNECT' }, connectHandler)
   } catch (err) {
     if (typeof callback !== 'function') {

lib/api/api-pipeline.js

@@ -5,15 +5,16 @@ const {
   Duplex,
   PassThrough
 } = require('node:stream')
+const assert = require('node:assert')
+const { AsyncResource } = require('node:async_hooks')
 const {
   InvalidArgumentError,
   InvalidReturnValueError,
   RequestAbortedError
 } = require('../core/errors')
 const util = require('../core/util')
-const { AsyncResource } = require('node:async_hooks')
+const RedirectHandler = require('../handler/RedirectHandler')
 const { addSignal, removeSignal } = require('./abort-signal')
-const assert = require('node:assert')
 
 const kResume = Symbol('resume')
 
@@ -239,7 +240,17 @@ class PipelineHandler extends AsyncResource {
 function pipeline (opts, handler) {
   try {
     const pipelineHandler = new PipelineHandler(opts, handler)
-    this.dispatch({ ...opts, body: pipelineHandler.req }, pipelineHandler)
+
+    if (opts?.maxRedirections != null && (!Number.isInteger(opts?.maxRedirections) || opts?.maxRedirections < 0)) {
+      throw new InvalidArgumentError('maxRedirections must be a positive number')
+    }
+
+    if (opts?.maxRedirections > 0) {
+      RedirectHandler.buildDispatch(this, opts.maxRedirections)({ ...opts, body: pipelineHandler.req }, pipelineHandler)
+    } else {
+      this.dispatch({ ...opts, body: pipelineHandler.req }, pipelineHandler)
+    }
+
     return pipelineHandler.ret
   } catch (err) {
     return new PassThrough().destroy(err)

lib/api/api-request.js

@@ -1,13 +1,14 @@
 'use strict'
 
+const { AsyncResource } = require('node:async_hooks')
 const Readable = require('./readable')
 const {
   InvalidArgumentError,
   RequestAbortedError
 } = require('../core/errors')
 const util = require('../core/util')
+const RedirectHandler = require('../handler/RedirectHandler')
 const { getResolveErrorBodyCallback } = require('./util')
-const { AsyncResource } = require('node:async_hooks')
 const { addSignal, removeSignal } = require('./abort-signal')
 
 class RequestHandler extends AsyncResource {
@@ -166,7 +167,18 @@ function request (opts, callback) {
   }
 
   try {
-    this.dispatch(opts, new RequestHandler(opts, callback))
+    const handler = new RequestHandler(opts, callback)
+
+    if (opts?.maxRedirections != null && (!Number.isInteger(opts?.maxRedirections) || opts?.maxRedirections < 0)) {
+      throw new InvalidArgumentError('maxRedirections must be a positive number')
+    }
+
+    if (opts?.maxRedirections > 0) {
+      RedirectHandler.buildDispatch(this, opts.maxRedirections)(opts, handler)
+      return
+    }
+
+    this.dispatch(opts, handler)
   } catch (err) {
     if (typeof callback !== 'function') {
       throw err

lib/api/api-stream.js

@@ -7,6 +7,7 @@ const {
   RequestAbortedError
 } = require('../core/errors')
 const util = require('../core/util')
+const RedirectHandler = require('../handler/RedirectHandler')
 const { getResolveErrorBodyCallback } = require('./util')
 const { AsyncResource } = require('node:async_hooks')
 const { addSignal, removeSignal } = require('./abort-signal')
@@ -207,7 +208,18 @@ function stream (opts, factory, callback) {
   }
 
   try {
-    this.dispatch(opts, new StreamHandler(opts, factory, callback))
+    const handler = new StreamHandler(opts, factory, callback)
+
+    if (opts?.maxRedirections != null && (!Number.isInteger(opts?.maxRedirections) || opts?.maxRedirections < 0)) {
+      throw new InvalidArgumentError('maxRedirections must be a positive number')
+    }
+
+    if (opts?.maxRedirections > 0) {
+      RedirectHandler.buildDispatch(this, opts.maxRedirections)(opts, handler)
+      return
+    }
+
+    this.dispatch(opts, handler)
   } catch (err) {
     if (typeof callback !== 'function') {
       throw err

lib/api/api-upgrade.js

@@ -1,10 +1,11 @@
 'use strict'
 
-const { InvalidArgumentError, RequestAbortedError, SocketError } = require('../core/errors')
+const assert = require('node:assert')
 const { AsyncResource } = require('node:async_hooks')
+const { InvalidArgumentError, RequestAbortedError, SocketError } = require('../core/errors')
 const util = require('../core/util')
+const RedirectHandler = require('../handler/RedirectHandler')
 const { addSignal, removeSignal } = require('./abort-signal')
-const assert = require('node:assert')
 
 class UpgradeHandler extends AsyncResource {
   constructor (opts, callback) {
@@ -88,11 +89,22 @@ function upgrade (opts, callback) {
 
   try {
     const upgradeHandler = new UpgradeHandler(opts, callback)
-    this.dispatch({
+    const upgradeOpts = {
       ...opts,
       method: opts.method || 'GET',
       upgrade: opts.protocol || 'Websocket'
-    }, upgradeHandler)
+    }
+
+    if (opts?.maxRedirections != null && (!Number.isInteger(opts?.maxRedirections) || opts?.maxRedirections < 0)) {
+      throw new InvalidArgumentError('maxRedirections must be a positive number')
+    }
+
+    if (opts?.maxRedirections > 0) {
+      RedirectHandler.buildDispatch(this, opts.maxRedirections)(upgradeOpts, upgradeHandler)
+      return
+    }
+
+    this.dispatch(upgradeOpts, upgradeHandler)
   } catch (err) {
     if (typeof callback !== 'function') {
       throw err

lib/client.js

@@ -62,7 +62,6 @@ const {
   kBodyTimeout,
   kStrictContentLength,
   kConnector,
-  kMaxRedirections,
   kMaxRequests,
   kCounter,
   kClose,
@@ -137,7 +136,6 @@ class Client extends DispatcherBase {
     tls,
     strictContentLength,
     maxCachedSessions,
-    maxRedirections,
     connect,
     maxRequestsPerClient,
     localAddress,
@@ -206,10 +204,6 @@ class Client extends DispatcherBase {
       throw new InvalidArgumentError('connect must be a function or an object')
     }
 
-    if (maxRedirections != null && (!Number.isInteger(maxRedirections) || maxRedirections < 0)) {
-      throw new InvalidArgumentError('maxRedirections must be a positive number')
-    }
-
     if (maxRequestsPerClient != null && (!Number.isInteger(maxRequestsPerClient) || maxRequestsPerClient < 0)) {
       throw new InvalidArgumentError('maxRequestsPerClient must be a positive number')
     }
@@ -268,7 +262,6 @@ class Client extends DispatcherBase {
     this[kBodyTimeout] = bodyTimeout != null ? bodyTimeout : 300e3
     this[kHeadersTimeout] = headersTimeout != null ? headersTimeout : 300e3
     this[kStrictContentLength] = strictContentLength == null ? true : strictContentLength
-    this[kMaxRedirections] = maxRedirections
     this[kMaxRequests] = maxRequestsPerClient
     this[kClosedResolve] = null
     this[kMaxResponseSize] = maxResponseSize > -1 ? maxResponseSize : -1

lib/handler/RedirectHandler.js

@@ -24,6 +24,15 @@ class BodyAsyncIterable {
 }
 
 class RedirectHandler {
+  static buildDispatch (dispatcher, maxRedirections) {
+    if (maxRedirections != null && (!Number.isInteger(maxRedirections) || maxRedirections < 0)) {
+      throw new InvalidArgumentError('maxRedirections must be a positive number')
+    }
+
+    const dispatch = dispatcher.dispatch.bind(dispatcher)
+    return (opts, originalHandler) => dispatch(opts, new RedirectHandler(dispatch, maxRedirections, opts, originalHandler))
+  }
+
   constructor (dispatch, maxRedirections, opts, handler) {
     if (maxRedirections != null && (!Number.isInteger(maxRedirections) || maxRedirections < 0)) {
       throw new InvalidArgumentError('maxRedirections must be a positive number')

test/redirect-request.js

@@ -90,7 +90,7 @@ for (const factory of [
 
     const server = await startRedirectingServer()
 
-    const { statusCode, headers, body: bodyStream, context: { history } } = await request(t, server, { maxRedirections: 10 }, `http://${server}/300?key=value`)
+    const { statusCode, headers, body: bodyStream, context: { history } } = await request(t, server, undefined, `http://${server}/300?key=value`, { maxRedirections: 10 })
     const body = await bodyStream.text()
 
     t.strictEqual(statusCode, 200)
@@ -380,10 +380,9 @@ for (const factory of [
     t = tspl(t, { plan: 1 })
 
     try {
-      await request(t, 'localhost', {
+      await request(t, 'localhost', undefined, 'http://localhost', {
+        method: 'GET',
         maxRedirections: 'INVALID'
-      }, 'http://localhost', {
-        method: 'GET'
       })
 
       t.fail('Did not throw')

test/redirect-stream.js

@@ -36,7 +36,7 @@ test('should always have a history with the final URL even if no redirections we
   t.strictEqual(body.join(''), `GET /5 key=value :: host@${server} connection@keep-alive`)
 })
 
-test('should not follow redirection by default if not using RedirectAgent', async t => {
+test('should not follow redirection by default if max redirect = 0', async t => {
   t = tspl(t, { plan: 3 })
 
   const body = []