Skip to content

fix(interceptor): add throwOnMaxRedirect to types and interceptor opts#5066

Open
maruthang wants to merge 1 commit intonodejs:mainfrom
maruthang:interceptor/issue-4376-throwOnMaxRedirect-types
Open

fix(interceptor): add throwOnMaxRedirect to types and interceptor opts#5066
maruthang wants to merge 1 commit intonodejs:mainfrom
maruthang:interceptor/issue-4376-throwOnMaxRedirect-types

Conversation

@maruthang
Copy link
Copy Markdown
Contributor

This relates to...

Fixes #4376. Continues the work from the now-closed #4377 (author abandoned after #4924 merged the docs in the opposite direction).

Rationale

Issue #4376 reported that throwOnMaxRedirect was missing from the TypeScript declarations, and that the documented usage redirect({ maxRedirections: 3, throwOnMaxRedirect: true }) did not actually work because the interceptor-level default was never plumbed through to the underlying RedirectHandler.

After #4924 settled the canonical name as throwOnMaxRedirect (singular, matching existing code), the remaining gaps are:

  1. The option is missing from RedirectInterceptorOpts in types/interceptors.d.ts, so TypeScript users get a compile error when following the docs.
  2. lib/interceptor/redirect.js only accepts maxRedirections as a default; throwOnMaxRedirect set on the interceptor itself is silently dropped — only the per-request value works.
  3. RedirectHandler does not validate the option type.

Changes

Bug Fixes

  • Plumb throwOnMaxRedirect from createRedirectInterceptor defaults through dispatchOpts so redirect({ maxRedirections: 3, throwOnMaxRedirect: true }) behaves as documented.
  • Add throwOnMaxRedirect?: boolean to RedirectInterceptorOpts in types/interceptors.d.ts.
  • Validate opts.throwOnMaxRedirect in RedirectHandler and throw InvalidArgumentError('throwOnMaxRedirect must be a boolean') for non-boolean values.
  • Add a JS regression test for the interceptor-level default and validation, plus a new tsd type test for RedirectInterceptorOpts.

Status

The `throwOnMaxRedirect` option was respected per-request inside
`RedirectHandler` but was missing from the TypeScript declarations and
was never plumbed from `redirect({ throwOnMaxRedirect: true })` (the
documented usage). Add it to `RedirectInterceptorOpts`, forward the
interceptor-level default through `dispatchOpts`, and validate the
type. Refs nodejs#4376, nodejs#4377.
Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.09%. Comparing base (e1211a5) to head (bfc0e79).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5066      +/-   ##
==========================================
- Coverage   93.10%   93.09%   -0.02%     
==========================================
  Files         110      110              
  Lines       35788    35792       +4     
==========================================
  Hits        33322    33322              
- Misses       2466     2470       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@maruthang
Copy link
Copy Markdown
Contributor Author

Friendly nudge @mcollina — approval + green CI + codecov happy. Ready to land whenever convenient. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

throwOnMaxRedirects is documented, but misnamed, and not in type definitions

3 participants