-
Notifications
You must be signed in to change notification settings - Fork 524
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
feat: new hooks API #3054
base: main
Are you sure you want to change the base?
feat: new hooks API #3054
Conversation
145ad9e
to
0444839
Compare
onRequestEnd?(): void; | ||
|
||
/** Invoked after response is starting to be processed */ | ||
onResponseStart?(controller: Controller): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're not passing a controller here.
Also, in the onRequestStart
above, you don't pass it so I would remove the comment at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we have 2 different controllers, one for the request and one for the response, they may run in parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit, rest LGTM!
|
||
if (channels.headers.hasSubscribers) { | ||
channels.headers.publish({ request: this, response: { statusCode, headers, statusText } }) | ||
} | ||
|
||
try { | ||
return this[kHandler].onHeaders(statusCode, headers, resume, statusText) | ||
this[kHandler].onResponseHeaders?.(parseHeaders(headers), statusCode, statusText) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we are no longer pass raw headers. We parse them immediately. As far as I can tell this is what most users do and it's unfortunate to force middleware to work on raw headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShogunPanda @metcoder95 @mcollina any thoughts here? Is this a mistake (i.e. performance blocker) or just being pragmatic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latter. Let's be pragmatic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on pragmatism
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice work; I can see the potential simplification of coordinating the flow control through the hooks thanks to the controller.
Just have some open questions left.
/** Invoked after headers data has been received */ | ||
onResponseHeaders?(headers: Record<string, string>, statusCode: number, statusText?: string): void; | ||
/** Invoked after response payload data is received. */ | ||
onResponseData?(chunk: Buffer | string): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, how are we planning for the hook to hint the controller about backpressure, as it seems it does not receives the controller?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking the user saves a reference to the controller from the start hook. By I guess we could always send in the controller?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair, we can keep it simple and keep the controller for that
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3054 +/- ##
==========================================
- Coverage 93.51% 93.50% -0.02%
==========================================
Files 89 89
Lines 24331 24408 +77
==========================================
+ Hits 22754 22822 +68
- Misses 1577 1586 +9 ☔ View full report in Codecov by Sentry. |
@mcollina @ShogunPanda @metcoder95
|
I would keep one controller which acts on both.
I definitely would allow to also control the request flow. |
Agree with @ShogunPanda towards a single controller, though I believe that most of the time implementers will look to control the stream flow over the response rather than the request; by providing a controller that allows them to also alter the flow of the request we might enable scenarios where users puts themselves into a weird spot and The last being said, I'd prefer to keep the controller only for the response and we can evaluate later if there's a need for one over the response.
Couldn't we achieve that already with |
Actually, you can be sending and receiving at the same time. Though I'm not sure how common that is.
Yes, that's also possible. Though less "nice". @metcoder95 @ShogunPanda So in theory we could totally skip the request hooks (at least for now). However, there is one problem. We need to be able to abort the request before the response has been received ( i.e.
But I guess we can just implement that and leave the rest for the future. Or can you think of a nicer way? Preferably without any One alternative is to keep |
Gosh, you are right. Why do I always forget about pipelining? XD
Let's skip it for now. I would just say let's code it in a way we can introduce it later in a semver-minor. |
Agree and in sync with @ShogunPanda, let's do that 👍 |
While this is not breaking per se. It's pretty much unusable with existing handlers...