Skip to content

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
@github-actions github-actions bot mentioned this pull request Mar 12, 2025
@github-actions github-actions bot mentioned this pull request May 12, 2025
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