Skip to content
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

Allow to configure a proxy using environment vars #4233

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

UFOMelkor
Copy link
Contributor

See the latest comment in #4084.

I do need to test it again at work, but if you have some spare time, you might provide some feedback on whether this implementation would be okay or you would like follow another approach.

Copy link
Member

@nathanlesage nathanlesage left a comment

Choose a reason for hiding this comment

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

Thank you for this initiative! There are a few questions, however:

  • Which types of requests does this affect? Only the ones made by got, anything that uses the net module, or some other subset?
  • Does this affect requests made by Electron (none node code) as well?
  • We should probably allow users to configure Proxies in the settings, similar to in other apps ("Use system settings", "use no proxy", "specify proxy")
  • Can we disable Proxies altogether if not needed? I.e., call bootstrap() programmatically, maybe even after the config provider has booted up? If this only needs to happen before the first request, we can easily query the config provider which doesn't make a request itself

@@ -0,0 +1,3 @@
declare module 'global-agent' {
Copy link
Member

Choose a reason for hiding this comment

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

There's a typo in the file name

source/main.ts Outdated
@@ -47,6 +48,9 @@ if (!app.requestSingleInstanceLock()) {

// If we reach this point, we are now booting the first instance of Zettlr.

// Use a proxy if one has been configured
bootstrap()
Copy link
Member

Choose a reason for hiding this comment

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

If not necessary to do so before the appReady event, please move this call out of the main file and into the boot cycle. If this needs to be called first thing even before the boot cycle, please add a comment why.

@UFOMelkor
Copy link
Contributor Author

  • Which types of requests does this affect? Only the ones made by got, anything that uses the net module, or some other subset?
  • Does this affect requests made by Electron (none node code) as well?

According to the docs, it affects everything that uses node's http.request. But I'm not as deep enough into electron to know, but I would guess that this won't effect any requests made by electron. Are there specific requests I might test?

  • We should probably allow users to configure Proxies in the settings, similar to in other apps ("Use system settings", "use no proxy", "specify proxy")
  • Can we disable Proxies altogether if not needed? I.e., call bootstrap() programmatically, maybe even after the config provider has booted up? If this only needs to happen before the first request, we can easily query the config provider which doesn't make a request itself

I'll test a later calling point when next time at work. If it is possible to call it after the config provider has booted up, I would guess the way to go would be adding a service provider for applying proxy configuration after the config provider but before the update provider?

@nathanlesage
Copy link
Member

Thanks for the update! Regarding Electron requests: I was specifically thinking about images. These are automatically loaded once an image is added to the document. So one thing you could test is see if image loading in Markdown files after proxying are also using the proxy or if they bypass it.

Then, I think it would suffice to add a comment explaining what is affected just so that we know (especially since this is a side effect that we may wonder about in the future).

Regarding where and how: that's a good question. On the one hand it's environment setup so the most correct place would be the end checker, but then when it's user configurable it can only run after the config provider has booted up. I'm a little bit hesitant regarding another service provider just for one setting. The question is: could there be potentially more settings except the proxy configuration that may fall into this category of needing to run early but still only after the config? One thing I could think about is the language setting, which needs to run super early but is currently instantiated only after the config has loaded.

Maybe a dedicated function within the config provider that sets up envs from the config ...? Hmmm...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants