A first draft of proxy auto-detection #326
Conversation
|
Interesting. Seems I need to handle rejections on the promise to have the builds pass. And the tests run under a different context, given the error |
…ctation of proxy setting
|
Also, I know it hasn’t been your style up until now, but could you credit “Louis St-Amour” or visibly link to this pull request in the Changelog, if this goes in? I’m probably not alone in thinking that a little recognition can go a long way – and a subtle reminder in the change log that this is an open source piece of software might encourage more developer-customers to submit PRs like mine as a way of collectively giving back. |
| @@ -17,7 +17,7 @@ export function init () { | |||
| httpProxy: '', | |||
| httpsProxy: '', | |||
| noProxy: '', | |||
| proxyEnabled: false, | |||
| proxyConfiguration: 'auto', | |||
There was a problem hiding this comment.
Should extract this to constants.js for reusability.
| return settings; | ||
| } | ||
|
|
||
| if (settings.proxyEnabled === true) { |
There was a problem hiding this comment.
This should probably still have an extra state for off in order to respect the current behavior?
| } | ||
| } else { | ||
| setOpt(Curl.option.PROXY, ''); |
There was a problem hiding this comment.
This line disabled Curl from automatically using HTTP_PROXY and HTTPS_PROXY and NO_PROXY environment variables. Maybe this could be part of the auto configuration, though?
| } | ||
| if (noProxy) { | ||
| setOpt(Curl.option.NOPROXY, noProxy); | ||
| const proxyPromise = new Promise((resolve) => { |
There was a problem hiding this comment.
We should probably move this to a separate function (similar to network/authentication.js) now that it's so complex.
| @@ -509,7 +541,7 @@ export function _actuallySend (renderedRequest, workspace, settings) { | |||
| respond({statusMessage, error}); | |||
| }); | |||
|
|
|||
| curl.perform(); | |||
| proxyPromise.then(() => curl.perform()); | |||
There was a problem hiding this comment.
Insomnia prefers async/await over promises, so this can be change to
await proxyPromise;
// ...
curl.perform();
I love the idea of linking the PR in the changelog @LouisStAmour. That would be a great way to show the involvement from the community. |
|
Are you still working on this @LouisStAmour? |
|
Ah sorry, I meant to get back to it but was swamped with work. Might have a chance this Saturday though. :) |
|
No problem @LouisStAmour. I'll leave this PR open for ya 😄 |
|
I'm going to close this PR for now since it's getting kind of stale but feel free to bring it back to life if you want. I think it would be a great feature 😄 |
proxyEnabledboolean to aproxyConfigurationsetting for clarity, but this could be converted back to a boolean if desired.proxyConfiguration(haven't investigated how this works, just copiedresponse...)network.jsto wait for a promise from the proxy lookupproxyConfigurationset to manual, existing logic still applies, untouched.Notes
electronrequired innetwork.jsrefers to theRendererbut to keep the code portable, I reference bothelectron.sessionandelectron.remote.getCurrentWebContents().session. It only now occurs to me that if I haven't tested the code block withelectron.session, I should probably remove it, as I'm not sure if I needelectron.sessionorelectron.session.defaultSessionor something else. (I'm relatively new to Electron...)proxyConfigurationsetting only affects Curl calls when set to manual. How to use a PAC could also be documented or linked to. (It was kind of magic to see the Charles Web Proxy auto-configuration get picked up by this code...)Screenshots
Related Issue: #215