Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: response error interceptor broken #3805

Merged
merged 4 commits into from
Nov 27, 2024
Merged

Conversation

luddd3
Copy link
Contributor

@luddd3 luddd3 commented Nov 6, 2024

This relates to...

The response-error interceptor

Rationale

The interceptor is currently broken since Request no longer accepts throwOnError as an option.

Changes

  • Expose the interceptor in the same manner as the others.
  • Remove throwOnError as it is considered an invalid argument.
  • Removed broken/incomplete tests

Features

Bug Fixes

Breaking Changes and Deprecations

Status

Expose the interceptor in the same manner as the others.

Remove `throwOnError` as it is considered an invalid argument.
@luddd3 luddd3 marked this pull request as ready for review November 6, 2024 12:27
Comment on lines +88 to +92
return (dispatch) => {
return function Intercept (opts, handler) {
return dispatch(opts, new ResponseErrorHandler(opts, { handler }))
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this makes me having stomach ache

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Care to elaborate or give a suggestion?

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 7, 2024

@metcoder95

When we are already touching this file massively, should we extract the handler into a new file and put it into the handler folder?

@metcoder95
Copy link
Member

It depends on what we do with that handler.
If the handler remains used within the interceptor only, let's keep it close to it; otherwise, let's add it into a separate handler file for easier management.

Copy link
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

@mcollina mcollina merged commit de6aea6 into nodejs:main Nov 27, 2024
31 checks passed
@luddd3 luddd3 deleted the fix-response-error branch November 27, 2024 13:10
@github-actions github-actions bot mentioned this pull request Dec 3, 2024
This was referenced Dec 16, 2024
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.

4 participants