-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
Enhance: Refine proxy support; Add proxy support to Logseq Sync #7711
Conversation
andelf
commented
Dec 13, 2022
•
edited
Loading
edited
- Add Proxy support for Logseq Sync
- Use system proxy when possible
- Use App-scope Proxy for Electron, node-fetch, Logseq Sync
- Refine proxy settings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
616665b
to
4a44369
Compare
src/electron/electron/handler.cljs
Outdated
(defmethod handle :testProxyUrl [_win [_ url]] | ||
(let [start-ms (.getTime (js/Date.))] | ||
(-> (utils/fetch url) | ||
(p/timeout 5000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need a more reasonable default proxy timeout value here.
src/electron/electron/utils.cljs
Outdated
(if (and agent-opts (not-empty (:protocol agent-opts))) | ||
(set-fetch-agent agent-opts) | ||
(when-let [sess (.. @*win -webContents -session)] | ||
(p/let [proxy (.resolveProxy sess "https://www.google.com") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "electron-way" to acquire system proxy settings.
Need a more proper URL here, like https://api.logseq.com
or AWS's s3 domain.
(for [{:keys [label value selected]} options] | ||
[:option (cond-> | ||
{:key label | ||
:default-value (or value label)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix: <option>
does not have a default-value
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
7452f70
to
09ad883
Compare
09ad883
to
60c5168
Compare
@tiensonqin @xyhp915 New changes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
refactor: use system proxy when possible chore: update yarn.lock
60c5168
to
d469a2d
Compare