Skip to content
New issue

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

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

Already on GitHub? # to your account

feat(interceptors): migrate decorator handler to new hooks #3905

Merged
merged 6 commits into from
Dec 1, 2024

Conversation

metcoder95
Copy link
Member

@metcoder95 metcoder95 commented Nov 29, 2024

  • feat(interceptors): migrate decorator handler to new hooks

This relates to...

Rationale

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

assert(!this.#onErrorCalled)

return this.#handler.onBodySent?.(...args)
onResponseError (...args) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I believe we should introduce a new hook to handle pre-request or in-flight request errors (e.g. network timeouts after connect, or runtime errors before connecting with remote peer); it feels quite odd handling these kind of errors through onResponseError when the response has not even being received

Copy link
Member

Choose a reason for hiding this comment

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

onRequestError?

@metcoder95 metcoder95 marked this pull request as draft November 29, 2024 12:20
@metcoder95
Copy link
Member Author

Will adjust the tests for the decorator

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

I'm thinking whether we should just deprecate/remove this?

@metcoder95
Copy link
Member Author

What exactly? The methods? Definitely. But we can aim to land this one to at least unlock the interceptors, and create a further PR to drop them

@metcoder95 metcoder95 marked this pull request as ready for review December 1, 2024 10:52
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 b860c03 into main Dec 1, 2024
36 of 37 checks passed
@metcoder95 metcoder95 deleted the fix/migrate_new_hooks branch December 1, 2024 18:14
@github-actions github-actions bot mentioned this pull request Dec 3, 2024
This was referenced Dec 16, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants