-
Notifications
You must be signed in to change notification settings - Fork 113
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 fetch methods #882
Conversation
Not sure why the changes 166-172 were introduced. I did not mean to make them happen. |
|
||
|
||
// We create a proxy based on Node.js environment variables. | ||
const proxy = process.env.HTTPS_PROXY || process.env.HTTP_PROXY; |
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.
are these common names? should it be GENAISCRIPT_HTTPS_PROXY / GENAISCRIPT_HTTP_PROXY?
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.
There are the ones used by Python and NodeJS as default env variables.
But we can add more in the expression.
${Object.entries(headers) | ||
.map(([k, v]) => `-H "${k}: ${v}"`) | ||
.join(" \\\n")} \\ | ||
.join("\\\n")} \\ |
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.
i think we want to keep the space here?
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.
probably a merge issue
@@ -92,5 +92,8 @@ | |||
"npm-check-updates": "^17.1.11", | |||
"prettier": "^3.3.3", | |||
"zx": "^8.2.2" | |||
}, | |||
"dependencies": { | |||
"https-proxy-agent": "^7.0.5" |
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.
move to devDependencies
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.
This is a feature I need in production. Any GenAIScript that wants to fetch content will need to go through the proxy and will need this.
This is not related to the issue I had with installing packages. And I don't think this will fix it.
LGTM, a few comments.
|
For the documentation, I am not sure we need a dedicated page. One question I don't have the answer for: when using GenAIScript from VS Code, how does GenAIScript get the VS Code proxy config info?
|
I need to research this. This is where the server process is created. https://github.com/microsoft/genaiscript/blob/main/packages/vscode/src/servermanager.ts#L83 |
so we can probably read the "http.proxy" setting and send it to the new process |
I tested the master branch on my work intranet. |
It's dangerous to replace the global fetch as any 3rd party library would be also using it which may lead to some unexpected results. |
How do I reference
|
See https://microsoft.github.io/genaiscript/reference/scripts/fetch/#hostfetch I put them on the host object |
When
HTTP_PROXY
orHTTPS_PROXY
env variables are present, thefetch
method is enriched with a proxy fromhttps-proxy-agent
.Tested locally using
tinyproxy
and the following scripttest.genai.mjs
using
HTTP_PROXY=http://127.0.0.1:3128 node packages/cli/built/genaiscript.cjs run test.genai.mjs
Note: when I created the commit, some changes were added, not sure why.