restify/node-restify#878 HTTP proxy improvments (http_proxy et al)
#85
Conversation
2 similar comments
There was a problem hiding this comment.
Looks mostly good to me. A couple of minor nits. Would appreciate another set of eyes on this. cc @micahr @DonutEspresso @jclulow
|
|
||
| // TODO: proxyOpts.headers (see whitelisting of req headers by 'request' |
There was a problem hiding this comment.
Can you please file an issue for this task?
| // HTTP proxy: `options.proxy` wins, else `https_proxy`/`http_proxy` envvars | ||
| // (upper and lowercase) are used. | ||
| if (options.proxy === false) { | ||
| this.proxy = false; |
There was a problem hiding this comment.
We tend to use the convention of self instead of this when accessing or mutating a variable that has already been declared. Mind sticking to this convention?
There was a problem hiding this comment.
Current usage is still largely this.:
$ grep '\bthis\.' lib/HttpClient.js | wc -l
74
$ grep '\bself\.' lib/HttpClient.js | wc -l
28
However, I'm totally cool favouring self.. Updated now.
|
LGTM |
2 similar comments
|
@yunong I've updated for your requests. I've not used GH reviews yet, so I'm not sure what comes next. I only see a "Dismiss review", which I'm pretty sure isn't what I want to click. ;) |
|
Merci @trentm 👍 👍 |
|
Feel free to merge. |
|
I'll publish a new minor rev tomorrow morning. |
|
restify-clients@1.4.0 |
See the changelog entry (https://github.com/restify/clients/pull/85/files#diff-8b1c3fd0d4a6765c16dfd18509182f9dR5) and restify/node-restify#878 discussion for notes.