-
Notifications
You must be signed in to change notification settings - Fork 541
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -215,6 +215,15 @@ declare namespace Dispatcher { | |
context: object; | ||
} | ||
export type StreamFactory = (data: StreamFactoryData) => Writable; | ||
export interface Controller { | ||
readonly aborted: boolean; | ||
readonly reason: Error | null; | ||
readonly paused: boolean; | ||
|
||
pause(): void; | ||
resume(): void; | ||
abort(reason: Error): void; | ||
} | ||
export interface DispatchHandlers { | ||
/** Invoked before request is dispatched on socket. May be invoked multiple times when a request is retried when the request at the head of the pipeline fails. */ | ||
onConnect?(abort: () => void): void; | ||
|
@@ -232,6 +241,37 @@ declare namespace Dispatcher { | |
onComplete?(trailers: string[] | null): void; | ||
/** Invoked when a body chunk is sent to the server. May be invoked multiple times for chunked requests */ | ||
onBodySent?(chunkSize: number, totalBytesSent: number): void; | ||
|
||
// New API | ||
|
||
/** Invoked when handler is destroyed */ | ||
onDestroy?(): void; | ||
|
||
/** Invoked after request is starting to be processed */ | ||
onRequestStart?(controller: Controller): void; | ||
/** Invoked after headers data is sent */ | ||
onRequestHeaders?(headers: Record<string, string>): void; | ||
/** Invoked after payload data is sent. */ | ||
onRequestData?(chunk: Buffer | string): void; | ||
/** Invoked after trailers data is sent */ | ||
onRequestTrailers?(trailers: Record<string, string>): void; | ||
/** Invoked after request has finished sending */ | ||
onRequestEnd?(): void; | ||
/** Invoked after request has failed */ | ||
onRequestError?(err: Error): 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 commentThe reason will be displayed to describe this comment to others. Learn more. I think you're not passing a controller here. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
/** 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 commentThe 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 commentThe 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 commentThe 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 |
||
/** Invoked after trailers data has been received */ | ||
onResponseTrailers?(trailers: Record<string, string>): void; | ||
/** Invoked after response has finished */ | ||
onResponseEnd?(): void; | ||
/** Invoked after request has failed */ | ||
onResponseError?(err: Error): void; | ||
} | ||
export type PipelineHandler = (data: PipelineHandlerData) => Readable; | ||
export type HttpMethod = 'GET' | 'HEAD' | 'POST' | 'PUT' | 'DELETE' | 'CONNECT' | 'OPTIONS' | 'TRACE' | 'PATCH'; | ||
|
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