fix: if scheme is http:// use an HTTP agent #75
Conversation
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #75 +/- ##
==========================================
+ Coverage 69.55% 70.18% +0.63%
==========================================
Files 1 1
Lines 358 369 +11
Branches 32 36 +4
==========================================
+ Hits 249 259 +10
- Misses 108 109 +1
Partials 1 1
Continue to review full report at Codecov.
|
src/index.ts
Outdated
| @@ -80,6 +82,8 @@ export interface RequestCallback<T = any> { | |||
| (err: Error | null, response: Response, body?: T): void; | |||
| } | |||
|
|
|||
| // tslint:disable-next-line variable-name | |||
| const HttpProxyAgent = require('http-proxy-agent'); | |||
There was a problem hiding this comment.
For both of these ... I think we should delay the require. For 90%+ of cases here, I'd guess folks aren't using proxies. For those cases, I would prefer to lazy load the module required.
src/index.ts
Outdated
| @@ -130,15 +134,21 @@ function requestToFetchOptions(reqOpts: Options) { | |||
| uri = uri + '?' + params; | |||
| } | |||
|
|
|||
| const isHttp = /^http:\/\//.test(uri); | |||
There was a problem hiding this comment.
This is a total nit, and calls out how lame of a programmer I am. Instead of using a regex - could we just say:
const isHttp = uri.startsWith('http://');There was a problem hiding this comment.
I think you're right, I still use contains and regexes more than I think I need to, since I haven't done as well updating my brain to reflect JavaScript's API improvements.
|
@callmehiphop @JustinBeckwith okay, addressed additional review and added test for HTTP proxy. |
we were always creating an
https.Agent, even though we accept anhttp://scheme.