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.

Sign up for GitHub

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

Jump to bottom

feat: dump interceptor #3118

Merged
merged 34 commits into from
May 15, 2024
Merged

feat: dump interceptor #3118

merged 34 commits into from
May 15, 2024

Conversation

metcoder95
Copy link
Member

@metcoder95 metcoder95 commented Apr 14, 2024 •
edited
Loading

This relates to...

Rationale

Relates to #2835

Changes

  • feat: add dump interceptor
  • fix(types): interceptor definitions

Features

Bug Fixes

Breaking Changes and Deprecations

Status

@metcoder95 metcoder95 requested a review from ronag April 14, 2024 13:53
metcoder95
metcoder95 commented Apr 14, 2024
View reviewed changes
lib/interceptor/dump.js Outdated
return true
}

// TODO: shall we forward the rest of the data to the handler or better to abort?
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggestions? I might be had it wrong here 🤔

Copy link
Member

Choose a reason for hiding this comment

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

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
ronag requested changes Apr 14, 2024
View reviewed changes
Copy link
Member

@ronag ronag left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Should the dump interceptor should also dump on abort?

lib/interceptor/dump.js Outdated Show resolved Hide resolved
ronag
ronag reviewed Apr 14, 2024
View reviewed changes
docs/docs/api/Dispatcher.md Outdated Show resolved Hide resolved
@metcoder95 metcoder95 mentioned this pull request Apr 16, 2024
Closed
@metcoder95 metcoder95 linked an issue Apr 16, 2024 that may be closed by this pull request
interceptors does not have any exported type #2972
Closed
metcoder95
metcoder95 commented Apr 16, 2024
View reviewed changes
lib/interceptor/dump.js Outdated Show resolved Hide resolved
@metcoder95
Copy link
Member Author

Should the dump interceptor should also dump on abort?

Do you mean to not directly abort the connection, but rather let it flow? Or how so?

@codecov-commenter
Copy link

codecov-commenter commented Apr 16, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 90.29851% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 94.15%. Comparing base (8c3d42a) to head (4492ab7).

Files Patch % Lines
lib/interceptor/dump.js 90.15% 13 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

mcollina
mcollina reviewed Apr 16, 2024
View reviewed changes
docs/docs/api/Dispatcher.md Show resolved Hide resolved
lib/interceptor/dump.js Outdated
return true
}

// TODO: shall we forward the rest of the data to the handler or better to abort?
Copy link
Member

Choose a reason for hiding this comment

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

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.

@metcoder95 metcoder95 requested a review from ronag April 17, 2024 10:37
@metcoder95 metcoder95 mentioned this pull request Apr 19, 2024
Closed
13 tasks
@mcollina
Copy link
Member

@ronag ptal

ronag
ronag requested changes Apr 22, 2024
View reviewed changes
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 have more opinions. Will try to have a better look as soon as I can.

lib/interceptor/dump.js Outdated Show resolved Hide resolved
lib/interceptor/dump.js Outdated Show resolved Hide resolved
@metcoder95 @ronag
fix: return on header
2fd173d 
Co-authored-by: Robert Nagy <[email protected]>
ronag
ronag reviewed Apr 23, 2024
View reviewed changes
lib/interceptor/dump.js Outdated Show resolved Hide resolved
ronag
ronag reviewed Apr 23, 2024
View reviewed changes
lib/interceptor/dump.js Outdated Show resolved Hide resolved
ronag
ronag reviewed Apr 23, 2024
View reviewed changes
lib/interceptor/dump.js Outdated Show resolved Hide resolved
ronag
ronag reviewed Apr 23, 2024
View reviewed changes
lib/interceptor/dump.js Outdated Show resolved Hide resolved
ronag
ronag reviewed Apr 23, 2024
View reviewed changes
lib/interceptor/dump.js Outdated Show resolved Hide resolved
ronag
ronag requested changes Apr 23, 2024
View reviewed changes
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 would remove waitForTrailers. Just makes things complicated for an edge case.

@github-actions github-actions bot mentioned this pull request May 17, 2024
Merged
@Uzlopak Uzlopak deleted the feat/dump_interceptor branch June 16, 2024 20:16
@matrix-compute matrix-compute mentioned this pull request Jun 24, 2024
Open
This was referenced Jul 8, 2024
Open
Open
@bburns bburns mentioned this pull request Jul 12, 2024
Open
@WontonSam WontonSam mentioned this pull request Jul 22, 2024
Open
@lholmquist lholmquist mentioned this pull request Aug 2, 2024
Closed
@lholmquist lholmquist mentioned this pull request Aug 7, 2024
Merged
@leonardoadame leonardoadame mentioned this pull request Aug 12, 2024
Open
@snyk-io snyk-io bot mentioned this pull request Aug 19, 2024
Open
@davehorton davehorton mentioned this pull request Aug 21, 2024
Open
@leonardoadame leonardoadame mentioned this pull request Aug 21, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Uzlopak Uzlopak Uzlopak left review comments

@mcollina mcollina mcollina approved these changes

@ronag ronag ronag approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

interceptors does not have any exported type
5 participants
@metcoder95 @codecov-commenter @mcollina @ronag @Uzlopak