Conversation
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
[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 |
main.js
Outdated
| env: { ...process.env, NODE_USE_ENV_PROXY: "1" }, | ||
| stdio: "inherit", | ||
| }); | ||
| child.on("exit", (code) => process.exit(code)); |
There was a problem hiding this comment.
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)); |
main.js
Outdated
| child.on("exit", (code) => process.exit(code)); | ||
| // TODO: return promise that resolves once sub process exits (for testing) | ||
| return; |
There was a problem hiding this comment.
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); | |
| }); | |
| }); |
|
note to selves: current implementation in Node.js |
main.jsandpost.jsNODE_USE_ENV_PROXY="1"