fix: update redirect handler options handling, docs, tests

docs/docs/api/Dispatcher.md

@@ -962,7 +962,7 @@ const { Client, interceptors } = require("undici");
 const { redirect } = interceptors;
 
 const client = new Client("http://example.com").compose(
-  redirect({ maxRedirections: 3, throwOnMaxRedirects: true })
+  redirect({ maxRedirections: 3, throwOnMaxRedirections: true })
 );
 client.request({ path: "/" })
 ```

docs/docs/api/RedirectHandler.md

@@ -2,15 +2,14 @@
 
 A class that handles redirection logic for HTTP requests.
 
-## `new RedirectHandler(dispatch, maxRedirections, opts, handler, redirectionLimitReached)`
+## `new RedirectHandler(dispatch, maxRedirections, opts, handler)`
 
 Arguments:
 
 - **dispatch** `function` - The dispatch function to be called after every retry.
 - **maxRedirections** `number` - Maximum number of redirections allowed.
 - **opts** `object` - Options for handling redirection.
 - **handler** `object` - An object containing handlers for different stages of the request lifecycle.
-- **redirectionLimitReached** `boolean` (default: `false`) - A flag that the implementer can provide to enable or disable the feature. If set to `false`, it indicates that the caller doesn't want to use the feature and prefers the old behavior.
 
 Returns: `RedirectHandler`
 
@@ -20,7 +19,6 @@ Returns: `RedirectHandler`
 - **maxRedirections** `number` (required) - Maximum number of redirections allowed.
 - **opts** `object` (required) - Options for handling redirection.
 - **handler** `object` (required) - Handlers for different stages of the request lifecycle.
-- **redirectionLimitReached** `boolean` (default: `false`) - A flag that the implementer can provide to enable or disable the feature. If set to `false`, it indicates that the caller doesn't want to use the feature and prefers the old behavior.
 
 ### Properties
 
@@ -30,7 +28,6 @@ Returns: `RedirectHandler`
 - **maxRedirections** `number` - Maximum number of redirections allowed.
 - **handler** `object` - Handlers for different stages of the request lifecycle.
 - **history** `Array` - An array representing the history of URLs during redirection.
-- **redirectionLimitReached** `boolean` - Indicates whether the redirection limit has been reached.
 
 ### Methods
 

lib/handler/redirect-handler.js

@@ -40,6 +40,10 @@ class RedirectHandler {
       throw new InvalidArgumentError('maxRedirections must be a positive number')
     }
 
+    if (opts.throwOnMaxRedirections != null && typeof opts.throwOnMaxRedirections !== 'boolean') {
+      throw new InvalidArgumentError('throwOnMaxRedirections must be a boolean')
+    }
+
     this.dispatch = dispatch
     this.location = null
     const { maxRedirections: _, ...cleanOpts } = opts
@@ -92,7 +96,8 @@ class RedirectHandler {
   }
 
   onResponseStart (controller, statusCode, headers, statusMessage) {
-    if (this.opts.throwOnMaxRedirect && this.history.length >= this.maxRedirections) {
+    // throwOnMaxRedirect was not documented or validated, but it was respected here, and may have been used by some users. Keep it for now.
+    if ((this.opts.throwOnMaxRedirections || this.opts.throwOnMaxRedirect) && this.history.length >= this.maxRedirections) {
       throw new Error('max redirects')
     }
 

lib/interceptor/redirect.js

@@ -2,17 +2,17 @@
 
 const RedirectHandler = require('../handler/redirect-handler')
 
-function createRedirectInterceptor ({ maxRedirections: defaultMaxRedirections } = {}) {
+function createRedirectInterceptor ({ maxRedirections: defaultMaxRedirections, throwOnMaxRedirections: defaultThrowOnMaxRedirections } = {}) {
   return (dispatch) => {
     return function Intercept (opts, handler) {
-      const { maxRedirections = defaultMaxRedirections, ...rest } = opts
+      const { maxRedirections = defaultMaxRedirections, throwOnMaxRedirections = defaultThrowOnMaxRedirections, ...rest } = opts
 
       if (maxRedirections == null || maxRedirections === 0) {
         return dispatch(opts, handler)
       }
 
       const dispatchOpts = { ...rest } // Stop sub dispatcher from also redirecting.
-      const redirectHandler = new RedirectHandler(dispatch, maxRedirections, dispatchOpts, handler)
+      const redirectHandler = new RedirectHandler(dispatch, maxRedirections, { throwOnMaxRedirections, ...dispatchOpts }, handler)
       return dispatch(dispatchOpts, redirectHandler)
     }
   }

test/interceptors/redirect.js

@@ -23,23 +23,23 @@ const {
 } = undici
 
 for (const factory of [
-  (server, opts) =>
-    new undici.Agent(opts).compose(
-      redirect({ maxRedirections: opts?.maxRedirections })
+  (server, dispatchOpts) =>
+    new undici.Agent(dispatchOpts).compose(
+      redirect({ maxRedirections: dispatchOpts?.maxRedirections, throwOnMaxRedirections: dispatchOpts?.throwOnMaxRedirections })
     ),
-  (server, opts) =>
-    new undici.Pool(`http://${server}`, opts).compose(
-      redirect({ maxRedirections: opts?.maxRedirections })
+  (server, dispatchOpts) =>
+    new undici.Pool(`http://${server}`, dispatchOpts).compose(
+      redirect({ maxRedirections: dispatchOpts?.maxRedirections, throwOnMaxRedirections: dispatchOpts?.throwOnMaxRedirections })
     ),
-  (server, opts) =>
-    new undici.Client(`http://${server}`, opts).compose(
-      redirect({ maxRedirections: opts?.maxRedirections })
+  (server, dispatchOpts) =>
+    new undici.Client(`http://${server}`, dispatchOpts).compose(
+      redirect({ maxRedirections: dispatchOpts?.maxRedirections, throwOnMaxRedirections: dispatchOpts?.throwOnMaxRedirections })
     )
 ]) {
-  const request = (t, server, opts, ...args) => {
-    const dispatcher = factory(server, opts)
+  const request = (t, server, dispatchOpts, url, requestOpts) => {
+    const dispatcher = factory(server, dispatchOpts)
     after(() => dispatcher.close())
-    return undici.request(args[0], { ...args[1], dispatcher }, args[2])
+    return undici.request(url, { ...requestOpts, dispatcher })
   }
 
   test('should always have a history with the final URL even if no redirections were followed', async t => {
@@ -445,7 +445,7 @@ for (const factory of [
     await t.completed
   })
 
-  test('should follow a redirect chain up to the allowed number of times for redirectionLimitReached', async t => {
+  test('should follow a redirect chain up to the allowed number of times for maxRedirections using the undici.request opts and undocumented throwOnMaxRedirect', async t => {
     t = tspl(t, { plan: 1 })
 
     const server = await startRedirectingServer()
@@ -466,6 +466,48 @@ for (const factory of [
     await t.completed
   })
 
+  test('should follow a redirect chain up to the allowed number of times for maxRedirections using the undici.request opts', async t => {
+    t = tspl(t, { plan: 1 })
+
+    const server = await startRedirectingServer()
+
+    try {
+      await request(t, server, undefined, `http://${server}/300`, {
+        maxRedirections: 2,
+        throwOnMaxRedirections: true
+      })
+    } catch (error) {
+      if (error.message.startsWith('max redirects')) {
+        t.ok(true, 'Max redirects handled correctly')
+      } else {
+        t.fail(`Unexpected error: ${error.message}`)
+      }
+    }
+
+    await t.completed
+  })
+
+  test('should follow a redirect chain up to the allowed number of times for maxRedirections using the dispatch opts', async t => {
+    t = tspl(t, { plan: 1 })
+
+    const server = await startRedirectingServer()
+
+    try {
+      await request(t, server, {
+        maxRedirections: 2,
+        throwOnMaxRedirections: true
+      }, `http://${server}/300`, undefined)
+    } catch (error) {
+      if (error.message.startsWith('max redirects')) {
+        t.ok(true, 'Max redirects handled correctly')
+      } else {
+        t.fail(`Unexpected error: ${error.message}`)
+      }
+    }
+
+    await t.completed
+  })
+
   test('when a Location response header is NOT present', async t => {
     t = tspl(t, { plan: 6 * 3 })
 

test/redirect-request.js

@@ -17,14 +17,14 @@ const { Headers: UndiciHeaders } = require('..')
 const redirect = undici.interceptors.redirect
 
 for (const factory of [
-  (server, opts) => new undici.Agent(opts).compose(redirect({ maxRedirections: opts?.maxRedirections })),
-  (server, opts) => new undici.Pool(`http://${server}`, opts).compose(redirect({ maxRedirections: opts?.maxRedirections })),
-  (server, opts) => new undici.Client(`http://${server}`, opts).compose(redirect({ maxRedirections: opts?.maxRedirections }))
+  (server, dispatchOpts) => new undici.Agent(dispatchOpts).compose(redirect({ maxRedirections: dispatchOpts?.maxRedirections, throwOnMaxRedirections: dispatchOpts?.throwOnMaxRedirections })),
+  (server, dispatchOpts) => new undici.Pool(`http://${server}`, dispatchOpts).compose(redirect({ maxRedirections: dispatchOpts?.maxRedirections, throwOnMaxRedirections: dispatchOpts?.throwOnMaxRedirections })),
+  (server, dispatchOpts) => new undici.Client(`http://${server}`, dispatchOpts).compose(redirect({ maxRedirections: dispatchOpts?.maxRedirections, throwOnMaxRedirections: dispatchOpts?.throwOnMaxRedirections }))
 ]) {
-  const request = (t, server, opts, ...args) => {
-    const dispatcher = factory(server, opts)
+  const request = (t, server, dispatchOpts, url, requestOpts) => {
+    const dispatcher = factory(server, dispatchOpts)
     after(() => dispatcher.close())
-    return undici.request(args[0], { ...args[1], dispatcher }, args[2])
+    return undici.request(url, { ...requestOpts, dispatcher })
   }
 
   test('should always have a history with the final URL even if no redirections were followed', async t => {
@@ -405,7 +405,7 @@ for (const factory of [
     await t.completed
   })
 
-  test('should follow a redirect chain up to the allowed number of times for redirectionLimitReached', async t => {
+  test('should follow a redirect chain up to the allowed number of times for maxRedirections using the undici.request opts and undocumented throwOnMaxRedirect', async t => {
     t = tspl(t, { plan: 1 })
 
     const server = await startRedirectingServer()
@@ -426,6 +426,48 @@ for (const factory of [
     await t.completed
   })
 
+  test('should follow a redirect chain up to the allowed number of times for maxRedirections using the undici.request opts', async t => {
+    t = tspl(t, { plan: 1 })
+
+    const server = await startRedirectingServer()
+
+    try {
+      await request(t, server, undefined, `http://${server}/300`, {
+        maxRedirections: 2,
+        throwOnMaxRedirections: true
+      })
+    } catch (error) {
+      if (error.message.startsWith('max redirects')) {
+        t.ok(true, 'Max redirects handled correctly')
+      } else {
+        t.fail(`Unexpected error: ${error.message}`)
+      }
+    }
+
+    await t.completed
+  })
+
+  test('should follow a redirect chain up to the allowed number of times for maxRedirections using the dispatch opts', async t => {
+    t = tspl(t, { plan: 1 })
+
+    const server = await startRedirectingServer()
+
+    try {
+      await request(t, server, {
+        maxRedirections: 2,
+        throwOnMaxRedirections: true
+      }, `http://${server}/300`, undefined)
+    } catch (error) {
+      if (error.message.startsWith('max redirects')) {
+        t.ok(true, 'Max redirects handled correctly')
+      } else {
+        t.fail(`Unexpected error: ${error.message}`)
+      }
+    }
+
+    await t.completed
+  })
+
   test('when a Location response header is NOT present', async t => {
     t = tspl(t, { plan: 6 * 3 })
 
@@ -462,6 +504,23 @@ for (const factory of [
     await t.completed
   })
 
+  test('should not allow invalid throwOnMaxRedirections arguments', async t => {
+    t = tspl(t, { plan: 1 })
+
+    try {
+      await request(t, 'localhost', undefined, 'http://localhost', {
+        method: 'GET',
+        maxRedirections: 1,
+        throwOnMaxRedirections: 'INVALID'
+      })
+
+      t.fail('Did not throw')
+    } catch (err) {
+      t.strictEqual(err.message, 'throwOnMaxRedirections must be a boolean')
+    }
+    await t.completed
+  })
+
   test('should not allow invalid maxRedirections arguments default', async t => {
     t = tspl(t, { plan: 1 })
 
@@ -479,6 +538,24 @@ for (const factory of [
     await t.completed
   })
 
+  test('should not allow invalid throwOnMaxRedirections arguments default', async t => {
+    t = tspl(t, { plan: 1 })
+
+    try {
+      await request(t, 'localhost', undefined, 'http://localhost', {
+        method: 'GET',
+        maxRedirections: 1,
+        throwOnMaxRedirections: 'INVALID'
+      })
+
+      t.fail('Did not throw')
+    } catch (err) {
+      t.strictEqual(err.message, 'throwOnMaxRedirections must be a boolean')
+    }
+
+    await t.completed
+  })
+
   test('should not follow redirects when using ReadableStream request bodies', async t => {
     t = tspl(t, { plan: 3 })
 

test/types/index.test-d.ts

@@ -22,5 +22,5 @@ const handler: Dispatcher.DispatchHandler = {}
 
 const redirectHandler = new Undici.RedirectHandler(dispatcher.dispatch, 10, {
   path: '/', method: 'GET'
-}, handler, false) as RedirectHandler
+}, handler)
 expectAssignable<RedirectHandler>(redirectHandler)

types/dispatcher.d.ts

@@ -135,8 +135,6 @@ declare namespace Dispatcher {
     signal?: AbortSignal | EventEmitter | null;
     /** This argument parameter is passed through to `ConnectData` */
     opaque?: TOpaque;
-    /** Default: false */
-    redirectionLimitReached?: boolean;
     /** Default: `null` */
     responseHeaders?: 'raw' | null;
   }
@@ -145,8 +143,6 @@ declare namespace Dispatcher {
     opaque?: TOpaque;
     /** Default: `null` */
     signal?: AbortSignal | EventEmitter | null;
-    /** Default: false */
-    redirectionLimitReached?: boolean;
     /** Default: `null` */
     onInfo?: (info: { statusCode: number, headers: Record<string, string | string[]> }) => void;
     /** Default: `null` */
@@ -168,8 +164,6 @@ declare namespace Dispatcher {
     protocol?: string;
     /** Default: `null` */
     signal?: AbortSignal | EventEmitter | null;
-    /** Default: false */
-    redirectionLimitReached?: boolean;
     /** Default: `null` */
     responseHeaders?: 'raw' | null;
   }

types/handlers.d.ts

@@ -6,7 +6,6 @@ export declare class RedirectHandler implements Dispatcher.DispatchHandler {
     maxRedirections: number,
     opts: Dispatcher.DispatchOptions,
     handler: Dispatcher.DispatchHandler,
-    redirectionLimitReached: boolean
   )
 }
 

types/interceptors.d.ts

@@ -8,7 +8,7 @@ export default Interceptors
 declare namespace Interceptors {
   export type DumpInterceptorOpts = { maxSize?: number }
   export type RetryInterceptorOpts = RetryHandler.RetryOptions
-  export type RedirectInterceptorOpts = { maxRedirections?: number }
+  export type RedirectInterceptorOpts = { maxRedirections?: number, throwOnMaxRedirections?: boolean }
 
   export type ResponseErrorInterceptorOpts = { throwOnError: boolean }
   export type CacheInterceptorOpts = CacheHandler.CacheOptions