-
Notifications
You must be signed in to change notification settings - Fork 110
WIP proxy support #284
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
base: beta
Are you sure you want to change the base?
WIP proxy support #284
Conversation
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.
Pull Request Overview
This PR adds proxy support by implementing child process spawning when proxy environment variables are detected but Node.js proxy support isn't explicitly enabled via NODE_USE_ENV_PROXY="1"
.
- Refactors main.js to wrap the existing logic in a
run()
function that checks for proxy configuration - Adds proxy detection logic that spawns a child process with
NODE_USE_ENV_PROXY="1"
when needed - Includes test cases and workflow updates to verify proxy functionality
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
main.js | Wraps existing logic in async run() function with proxy detection and child process spawning |
tests/main-spawns-sub-process-for-proxy.test.js | Adds test for proxy child process behavior (WIP with TODO items) |
.github/workflows/test.yml | Updates workflow with proxy environment variables and adds dedicated proxy test job |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
||
test("spawns a child process if proxy is set and NODE_USE_ENV_PROXY is not set", async (t) => { | ||
// https://nodejs.org/api/test.html#class-mocktracker | ||
// TODO: why u not work |
Copilot
AI
Sep 12, 2025
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.
[nitpick] The comment contains informal language. Consider updating to: // TODO: Mock not working as expected
// TODO: why u not work | |
// TODO: Mock not working as expected |
Copilot uses AI. Check for mistakes.
main.js
Outdated
env: { ...process.env, NODE_USE_ENV_PROXY: "1" }, | ||
stdio: "inherit", | ||
}); | ||
child.on("exit", (code) => process.exit(code)); |
Copilot
AI
Sep 12, 2025
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 exit code could be null when the child process is terminated by a signal. This should handle null values: child.on('exit', (code) => process.exit(code || 1));
child.on("exit", (code) => process.exit(code)); | |
child.on("exit", (code) => process.exit(code ?? 1)); |
Copilot uses AI. Check for mistakes.
main.js
Outdated
child.on("exit", (code) => process.exit(code)); | ||
// TODO: return promise that resolves once sub process exits (for testing) | ||
return; |
Copilot
AI
Sep 12, 2025
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 TODO indicates incomplete implementation that affects testability. Consider implementing the promise-based approach to properly support testing scenarios.
child.on("exit", (code) => process.exit(code)); | |
// TODO: return promise that resolves once sub process exits (for testing) | |
return; | |
// Return a promise that resolves once sub process exits (for testing) | |
return new Promise((resolve, reject) => { | |
child.on("exit", (code) => { | |
process.exit(code); | |
resolve(code); | |
}); | |
child.on("error", (err) => { | |
reject(err); | |
}); | |
}); |
Copilot uses AI. Check for mistakes.
note to selves: current implementation in Node.js |
main.js
andpost.js
NODE_USE_ENV_PROXY="1"