feat: dump interceptor #3118
feat: dump interceptor #3118
Conversation
lib/interceptor/dump.js
Outdated
| return true | ||
| } | ||
|
|
||
| // TODO: shall we forward the rest of the data to the handler or better to abort? |
There was a problem hiding this comment.
Suggestions? I might be had it wrong here 🤔
There was a problem hiding this comment.
you must keep the data flowing from the request or error/abort it. The latter will destroy the connection. It should be a user choice, but I would error/abort the socket by default.
Co-authored-by: Robert Nagy <[email protected]>
Do you mean to not directly abort the connection, but rather let it flow? Or how so? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3118 +/- ##
==========================================
- Coverage 94.17% 94.15% -0.03%
==========================================
Files 90 91 +1
Lines 24559 24692 +133
==========================================
+ Hits 23128 23248 +120
- Misses 1431 1444 +13 ☔ View full report in Codecov by Sentry. |
lib/interceptor/dump.js
Outdated
| return true | ||
| } | ||
|
|
||
| // TODO: shall we forward the rest of the data to the handler or better to abort? |
There was a problem hiding this comment.
you must keep the data flowing from the request or error/abort it. The latter will destroy the connection. It should be a user choice, but I would error/abort the socket by default.
|
@ronag ptal |
ronag
left a comment
ronag
left a comment
There was a problem hiding this comment.
I have more opinions. Will try to have a better look as soon as I can.
Co-authored-by: Robert Nagy <[email protected]>
ronag
left a comment
ronag
left a comment
There was a problem hiding this comment.
I would remove waitForTrailers. Just makes things complicated for an edge case.
This relates to...
Rationale
Relates to #2835
Changes
Features
Bug Fixes
Breaking Changes and Deprecations
Status