feat(binding-http): add http server middleware #1027
Conversation
Add a middleware for the http server to handle raw http requests
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #1027 +/- ##
==========================================
- Coverage 75.79% 75.61% -0.19%
==========================================
Files 72 72
Lines 14871 14887 +16
Branches 1428 1431 +3
==========================================
- Hits 11272 11257 -15
- Misses 3566 3597 +31
Partials 33 33
|
| export default class HttpMiddleware { | ||
| public handler: MiddlewareRequestHandler; | ||
|
|
||
| constructor(handler: MiddlewareRequestHandler) { | ||
| this.handler = handler; | ||
| } | ||
|
|
||
| public handleRequest(req: http.IncomingMessage, res: http.ServerResponse, next: () => void): Promise<void> { | ||
| debug("Hit middleware"); | ||
| return this.handler(req, res, next); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can't we just get rid of this class and use directly the MiddlewareRequestHandler. I don't see the benefit of having this container class other than having a debug message. 🤔
There was a problem hiding this comment.
Maybe, it would be useful to have a class that takes as input a list of MiddlewareRequestHandlers and configure the next function accordingly? (just got this requirement from my local refactoring, but feel free to add it later)
There was a problem hiding this comment.
Can't we just get rid of this class and use directly the MiddlewareRequestHandler. I don't see the benefit of having this container class other than having a debug message.
The class was meant more as a base class that can a developer can extend to handle more complex logic, like keeping a "state" between on request and the other, or any other logic that comes to mind.
But I agree the MiddlewareRequestHandler function is what gets executed inside the HttpServer, so we can get rid of the class. Just let me know how to proceed.
Maybe, it would be useful to have a class that takes as input a list of
MiddlewareRequestHandlers and configure the next function accordingly? (just got this requirement from my local refactoring, but feel free to add it later)
How would you configure the next function based on the MiddlewareRequestHandler? I didn't really get it 🤔
There was a problem hiding this comment.
Maybe, it would be useful to have a class that takes as input a list of
MiddlewareRequestHandlers and configure the next function accordingly? (just got this requirement from my local refactoring, but feel free to add it later)
Maybe the type of the middleware parameter in the HttpServer's constructor could also be changed to HttpMiddleware | HttpMiddleware[] to cover that? Although things might indeed become a bit complicated in the array case when it comes to the next function.
Maybe an alternative could be to let users pass in functions of type
export type AlternativeMiddlewareRequestHandler = (
req: http.IncomingMessage,
res: http.ServerResponse,
abort: () => void,
) => Promise<void>;and then the server does the chaining internally. In this case, I replaced next() with an abort() function which would allow users to stop the processing – the next middleware would be called by the server instead if no abort() happened before.
Do you think this could work? Or would having a next() function instead give users more control?
There was a problem hiding this comment.
I was thinking of something like this:
let middlewares: MiddlewareRequestHandler[] // initialized somewhere
const first = middlewares.shift()
const entries = middleware.entries()
first(req,res, () => entries.next().value[1]()) // needs to check also if we are done but removed for giving the ideathis way the middleware can control whether to pass to the next function or not, without introducing the abort. Going back to the PR itself @Luca8991 I would remove HTTPMiddleware class and discuss further improvements in a future PR.
There was a problem hiding this comment.
@relu91 I've pushed the commit that removes the HTTPMiddleware class
Add a middleware for the http server to handle raw http requests.
An example on how to use the middleware can be found in the tests: https://github.com/Luca8991/node-wot/blob/master/packages/binding-http/test/http-server-test.ts#L53-L101