feat: add proxy-agent#1070
feat: add proxy-agent#1070mcollina merged 11 commits intonodejs:mainfrom RafaelGSS:feat/add-proxy-agent
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1070 +/- ##
==========================================
+ Coverage 94.93% 94.96% +0.03%
==========================================
Files 38 39 +1
Lines 3709 3734 +25
==========================================
+ Hits 3521 3546 +25
Misses 188 188 ☔ View full report in Codecov by Sentry. |
mcollina
left a comment
mcollina
left a comment
There was a problem hiding this comment.
Could you add another test with a path as well?
Looks good!
|
docs? |
|
I've improved the test coverage and included API documentation in this new class. There is a remaining check to do before making it ready for review which is the usage with HTTPS. I'll do it and share feedback ASAP. |
|
|
||
| If you proxy requires basic authentication, you can send it via the `proxy-authorization` header. | ||
|
|
||
| > In latest versions of `undici` you can use [`ProxyAgent`](docs/api/ProxyAgent.md) to avoid some settings. |
There was a problem hiding this comment.
I would consider rewording this:
- after the first release where this will be available, the version in which it is available will not be the "latest" anymore. When this code is released this comment is basically redundant because the ProxyAgent is available and that's all users need to know. This mention should simply go in the release notes. I would also move it to the top as it's probably going to be the preferred way to set it up
- "avoid some settings" could be reworded as well
There was a problem hiding this comment.
Do you think that this entire document should be refactored to use ProxyAgent or a recommendation on top of the file:
Connecting through a proxy is possible by:
- Using [AgentProxy](docs/api/ProxyAgent.md).
- Configuring `Client` or `Pool` constructor.should be enough?
| @@ -0,0 +1,189 @@ | |||
| 'use strict' | |||
There was a problem hiding this comment.
in the agent implementation I see it's dealing with possibly multiple clients. is it something worth testing as well?
There was a problem hiding this comment.
If I'm not mistaken, when dealing with Proxy just one client is set.
Example:
setGlobalDispatcher(new ProxyAgent('http://localhost:8000/'))
async function main() {
const res1 = await request('http://localhost:3000/undici')
console.log('response received', res1.statusCode)
const res2 = await request('http://localhost:4000/undici')
console.log('response received', res2.statusCode)
}
main()It will only set http://localhost:8000 as a client.
setting client http://localhost:8000/
response received 200
response received 201
This comment has been minimized.
This comment has been minimized.
|
Communication with origin HTTPS is now working. It wasn't working because I've missed the |
| headers: { | ||
| ...opts.headers, | ||
| host, | ||
| 'User-Agent': 'undici' |
There was a problem hiding this comment.
Let's remove this user-agent if it's not strictly needed. If it's strictly required by proxies, let's make sure it's user-configurable (right now is not if I read the code correctly).
There was a problem hiding this comment.
It's not required by proxies and it's totally user-configurable:
const {
statusCode,
headers,
trailers,
body
// send the request via the http://localhost:8000/ HTTP proxy
} = await request('http://localhost:3000/undici', { headers: { 'User-Agent': 'Example/0.2.1' } })Removed support to pre-defined UserAgent.
| import MockPool = require('./types/mock-pool') | ||
| import MockAgent = require('./types/mock-agent') | ||
| import mockErrors = require('./types/mock-errors') | ||
| import ProxyAgent from './types/proxy-agent' |
There was a problem hiding this comment.
This line does not correspond with the actual import of ProxyAgent (index.js#L17).
6 participants
Address: #569.
There is a remaining point to be discussed. I've implemented it and tested it against other HTTP libraries (request, axios) and here are a quick HTTP response summary:
Undici
Axios
Request
You will notice that other HTTP libraries answer the HTTP request with
Connection: closethrough the proxy. Nonetheless,undicikeeps the connection open withkeep-alive. I'm supposing that it's was designed by default, I'm sharing it just to align expectations.TODO: