8000 feat: Added support for all proxy modes by zeeker999 · Pull Request #24937 · electron/electron · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: Added support for all proxy modes #24937

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Oct 27, 2020
Merged

feat: Added support for all proxy modes #24937

merged 2 commits into from
Oct 27, 2020

Conversation

zeeker999
Copy link
Contributor
@zeeker999 zeeker999 commented Aug 11, 2020 •
edited by nornagon
Loading

Description of Change

This pull request introduces following changes:

  • Extended the setProxy() to support all proxy modes including direct, auto_detect, fixed_servers, pac_script and system.
  • Api for reloading proxy configuration.

Checklist

Release Notes

Notes: Added support for explicitly specifying direct, auto_detect or system modes in session.setProxy().

8000
@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Aug 11, 2020
@deepak1556
Copy link
Member

Thanks for the PR!

Can you split the closeAllConnections() and closeIdleConnections() to a separate PR.

Also can you add some tests, it would be difficult to test system and auto_detect mode but you can unittest the api and also verify the other modes.

deepak1556
deepak1556 reviewed Aug 11, 2020
View reviewed changes
@zeeker999 zeeker999 changed the title feat: Complete the api for setting proxy feat: Added support for all proxy modes Aug 12, 2020
@zeeker999
Copy link
Contributor Author

Thanks for the PR!

Can you split the closeAllConnections() and closeIdleConnections() to a separate PR.

Also can you add some tests, it would be difficult to test system and auto_detect mode but you can unittest the api and also verify the other modes.

Finished, please review my changes.

@zeeker999 zeeker999 mentioned this pull request Aug 12, 2020
Merged
7 tasks
deepak1556
deepak1556 reviewed Aug 12, 2020
View reviewed changes
Copy link
Member
@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with minor nits

@deepak1556
Copy link
Member

changes look good, but this will need review from api working group before being merged.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Aug 12, 2020
@zeeker999 zeeker999 force-pushed the proxy-master branch 2 times, most recently from ac7eb75 to b46c3e6 Compare August 20, 2020 05:43
nornagon
nornagon suggested changes Aug 24, 2020
View reviewed changes
Copy link
Contributor
@nornagon nornagon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new APIs looks good to me, but I'd like to know a couple of things:

  1. Can the mode be set using the already-existing proxyRules string? Does the mode parameter provide access to functionality that's not currently accessible?
  2. Can you say a little more about the use-case that motivates this API?

Thanks for the well-formatted PR (and test cases)!

@jkleinsc
8000 Copy link
Member

The @electron/wg-api approved this at the August 24, 2020 meeting

@zeeker999
Copy link
Contributor Author
zeeker999 commented Aug 26, 2020 •
edited
Loading

The new APIs looks good to me, but I'd like to know a couple of things:

  1. Can the mode be set using the already-existing proxyRules string? Does the mode parameter provide access to functionality that's not currently accessible?
  1. Can you say a little more about the use-case that motivates this API?

Thanks for the well-formatted PR (and test cases)!

The new APIs looks good to me, but I'd like to know a couple of things:

  1. Can the mode be set using the already-existing proxyRules string?

If it's unspecified, it will be automatically determined based on other specified options and matches the current behaviour. The mode is set to pac_script if pacScript is specified, otherwise it's set to fixed_servers.

Does the mode parameter provide access to functionality that's not currently accessible?
Without the mode parameter:

  • It's unclear how to set the proxy to direct mode, it's much clear to call setProxy({ mode: 'direct' }) then setProxy({proxyRules: ''})
  • There's no way to set proxy back to system/auto_detect mode after setting a proxy for a session
  1. Can you say a little more about the use-case that motivates this API?

Let me quote a part of reply from pull request #24945

The real world use case is a user want to access to a site requires a proxy and he get a proxy list from somewhere.

  1. Our product randomly select a proxy from the list and try to navigate the target site
  2. Do some basic checks to verify whether the proxy is good and the website is accessable
  3. Goto step 1 if the check failed

The problem is these proxies maybe from the same provider and the origin is same too. Everything is ok when the origin of doesn't match previous one, otherwise the connection is reused when the server supports http 2 or keep-alived connections which are true for most of sites.

There's no way to switch the proxy back to system mode without this change to setProxy.

@zeeker999 zeeker999 requested a review from nornagon August 29, 2020 02:22
nornagon
nornagon suggested changes Oct 20, 2020
View reviewed changes
Copy link
Contributor
@nornagon nornagon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarifications, and sorry for dropping the ball on this review. The general shape of the API looks good to me, just a couple of minor comments before we can get this landed.

@zeeker999 zeeker999 force-pushed the proxy-master branch 7 times, most recently from 0dfeafb to 789d4d7 Compare October 25, 2020 22:38
@nornagon
Copy link
Contributor

@LuoJinghua looks like the build is failing, but otherwise lgtm :)

@zeeker999
feat: Added support for all proxy modes
40e2ed5 
This commit extended setProxy to support all proxy modes including
direct, auto_detect, pac_script, fixed_servers and system.
@zeeker999
Copy link
Contributor Author

@LuoJinghua looks like the build is failing, but otherwise lgtm :)

Fixed.

@deepak1556 deepak1556 mentioned this pull request Oct 26, 2020
Merged
4 tasks
@zcbenz zcbenz merged commit 201fc11 into electron:master Oct 27, 2020
@release-clerk
Copy link
release-clerk bot commented Oct 27, 2020

Release Notes Persisted

Added support for explicitly specifying direct, auto_detect or system modes in session.setProxy().

@zeeker999 zeeker999 mentioned this pull request Nov 18, 2020
Closed
@unfurl-links unfurl-links bot mentioned this pull request Mar 7, 2021
Closed
zeeker999 added a commit to zeeker999/electron that referenced this pull request Mar 13, 2021
@zeeker999
feat: Added support for all proxy modes (electron#24937)
eccfd59 
* feat: Added support for all proxy modes

This commit extended setProxy to support all proxy modes including
direct, auto_detect, pac_script, fixed_servers and system.

* feat: New api for reload proxy configurations
zeeker999 added a commit to zeeker999/electron that referenced this pull request Mar 13, 2021
@zeeker999
feat: Added support for all proxy modes (electron#24937)
9046d66 
* feat: Added support for all proxy modes

This commit extended setProxy to support all proxy modes including
direct, auto_detect, pac_script, fixed_servers and system.

* feat: New api for reload proxy configurations
zeeker999 added a commit to zeeker999/electron that referenced this pull request Apr 9, 2021
@zeeker999
feat: Added support for all proxy modes (electron#24937)
bcbf517 
* feat: Added support for all proxy modes

This commit extended setProxy to support all proxy modes including
direct, auto_detect, pac_script, fixed_servers and system.

* feat: New api for reload proxy configurations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deepak1556 deepak1556 deepak1556 left review comments

@nornagon nornagon nornagon approved these changes

@zcbenz zcbenz zcbenz approved these changes

Assignees
No one assigned
Labels
api-review/approved ✅
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@zeeker999 @deepak1556 @jkleinsc @nornagon @zcbenz
0