fix: make proxy nodejs specific logic available only for nodejs runtime #716
Conversation
|
There is an occurrence of |
| const proxyUrl = process.env.HTTPS_PROXY ?? process.env.https_proxy | ||
| const certs = process.env.AWS_CA_BUNDLE ? [readFileSync(process.env.AWS_CA_BUNDLE)] : undefined | ||
| // short term solution to fix webworker bundling, broken due to this node.js specific logic in here | ||
| const isNodeJS: boolean = typeof process !== 'undefined' && process.release && process.release.name === 'node' |
There was a problem hiding this comment.
Why don't we pass HTTPS_PROXY and AWS_CA_BUNDLE as a config value from the runtime - the same way as we started doing for other env vars? It would not be that complex, but more clean.
There was a problem hiding this comment.
As this is very short term, I opted for the easiest solution that did the job, imo a more clean solution makes sense if it stays in the codebase indefinitely. Passing down via runtime is another option, not that complex but more work than this for sure (see https://github.com/aws/language-servers/pull/711/files).
If you have a strong preference for the runtime option, I am good with implementing that solution here as well, let me know!
Problem
this is a short term fix, to fix the webworker bundling. We will implement a better solution very soon.
We want to avoid having nodejs specific logic (process.env.VARIABLE) in lsp-codewhisperer as it breaks the webworker bundling, for which
processis not defined.Solution
If/else check to see if we are in node.js env
I tested the changes by testing the proxy
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.