feat!: Add req.hostname and req.port#4766
Conversation
|
Is there a better way to get the |
lib/route.js
Outdated
| const request = new context.Request(id, params, req, query, childLogger, context) | ||
| const reply = new context.Reply(res, request, childLogger) | ||
|
|
||
| request.port = Number(request.host && request.host.split(':').length > 1 ? request.host.split(':')[1] : null) |
There was a problem hiding this comment.
This can be inferred directly from the headers when requested. This might not be really needed here 🤔
There was a problem hiding this comment.
But in case of a proxy, the port might not be there. So how can I infer the port?
There was a problem hiding this comment.
Have into account that this information is already available for the Request object when it's being instantiated. For instance, if trustProxy is set, you can infer it with the host property, see how it works here
| return this.raw.headers.host || this.raw.headers[':authority'] | ||
| } | ||
| }, | ||
| hostname: { |
There was a problem hiding this comment.
This is not correctly tested, can we add some tests for it?
There was a problem hiding this comment.
Do you mean tests for the Request object or tests for the incoming request when the server is listening?
There was a problem hiding this comment.
You change all the existing test from hostname to host.
So, there is lack of testing the hostname and port property.
Co-authored-by: Carlos Fuentes <me@metcoder.dev>
|
This must be rebased on top of the |
Co-authored-by: Carlos Fuentes <me@metcoder.dev>
|
@mcollina @metcoder95 any update on this pr? |
|
This is marked as |
done 👍 |
|
I think we need to rebase |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
fixes #4682
Checklist
npm run testandnpm run benchmarkand the Code of conduct