-
Notifications
You must be signed in to change notification settings - Fork 243
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
Proxy support for amp-optimizer #857
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Thanks for creating this! One suggestion: as the code for integrating this is trivial, would you mind implementing this directly instead of adding an additional dependency? |
This reverts commit 8e129fc.
@sebastianbenz I searched for this fix that doesn't require any additional libraries. |
Sorry for not being more clear. Integrating |
When environment variable name https_proxy is available, HttpsProxyAgent is used as node-fetch agent.
@sebastianbenz Sorry for late reaction. |
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.
Thanks a lot! This looks good! One request: can you also add this here https://github.com/ampproject/amp-toolbox/blob/main/packages/optimizer/lib/DomTransformer.js#L156 to enable proxy support at runtime.
@sebastianbenz I checked Should I replace cross-fetch to node-fetch? or any ideas? |
Hm - that's annoying. I think it's fine to replace cross-fetch in that case and we don't release a browser bundle yet which would benefit from cross fetch. |
@sebastianbenz Ok, so let's try replacing "cross-fetch" with "node-fetch" once in a separate branch. |
* Proxy support for amp-optimizer * Environment variable https_proxy support * Revert "Proxy support for amp-optimizer" This reverts commit 8e129fc. * Use https-proxy-agent. When environment variable name https_proxy is available, HttpsProxyAgent is used as node-fetch agent. Co-authored-by: Sebastian Benz <sebastian.benz@gmail.com>
Proxy support using node-fetch-with-proxy instead of node-fetch.
If you need to use proxy, just set the environment variable
https_proxy
.Thanks.