fix(language-server): use https and http proxy agent to make fetcher …

[ndrsg]

…work behind proxy

Added custom agent to fetcher / node-fetch to be able to GET ui5-model behind a proxy (e.g.
corporate proxy). Therefore look at environment variables HTTPS_PROXY, HTTP_PROXY and NO_PROXY and
respect them depending on request url.

I'm passing agent as any, because of incompatibility of agent-base typings:
TooTallNate/proxy-agents#108

fix #285

[bd82]

Thanks for making this PR @ndrsg 🍰

Review

Custom User Agent vs using a different http[s] library

• Did you consider/evaluate replacing the fetch library with one that handles the proxy settings automatically?
• Could there be edge cases we are missing?
  • e.g: I checked for example that process.env["http_proxy"] === process.env["HTTP_PROXY"] (case insensitive env var names).

Logic structure.

Could we avoid mixing up the building of the userAgent and the fetch call?
e.g:

let fetchOptions = {};

if (process.env["http_proxy"] && otherCondition) {
   fetchOptions.agent = new HttpProxyAgent(process.env["HTTP_PROXY"]);
}
else if (HTTPS_CONDITION) {
   fetchOptions.agent = new HttpsProxyAgent(process.env["HTTPS_PROXY"]);
}

// only call `fetch` once.
fetch(url, fetchOptions)

Basically this would enable us to extract the new logic to a separate function which could be useful:

• Once we fix the code coverage checks on the language-server package we will need to exclude the http-agent related logic.
• Generally cleaner code with greater separation of concerns.

Console usage

I wonder if we should keep these or remove these.
We have an output channel in the VSCode extension, however I am not sure if it is accessible from the language server.

ESLint errors

You can tell eslint to ignore errors in certain lines, probably something like:

• /* eslint-disable-next-line @typescript-eslint/no-explicit-any */
• // eslint-disable-next-line @typescript-eslint/no-explicit-any

See relevant docs:

• https://eslint.org/docs/user-guide/configuring#disabling-rules-with-inline-comments

[tal-sapan]

Hi,

I agree with @bd82.
My comments:

1. Extract the proxy handling to a different function which will return the agent based on the URL.
2. If possible, use a library for checking the environment variables and handling edge cases (like different case of env var names, no_proxy handling - instead of substring I think it would be better to check the host).
3. Only log the usage of a proxy, and maybe also when you find the URL is in no_proxy. If the env vars are not set I don't think there is a need to log. Also, in the log it might be useful to log the proxy URL.
4. Only calculate the agent once, before iterating over libs. The library URL always begins with the baseUrl (which contains the schema and host). There is no need to calculate it for every library name, and it would prevent writing many identical logs to the console (which goes to the output channel of the extension).
5. Instead of casting to any you can cast to the expected type (Agent). If typescript doesn't accept it as-is, you can cast to unknown first and then to Agent. It is closer to your intention than any and explains better what you tried to do.

[ndrsg]

Hello @bd82 and @tal-sapan ,

thanks for your review, I want to reply to some points:

Did you consider/evaluate replacing the fetch library with one that handles the proxy settings automatically?

Yes I considered, but thought better not adding an additional dependency. If you think its a better option to use, e.g. https://github.com/axios/axios , i can also try to integrate it.

Could there be edge cases we are missing?
e.g: I checked for example that process.env["http_proxy"] === process.env["HTTP_PROXY"] (case insensitive env var names).

Yes that's true, i forgot about JSON Case Sensitivity.

Extracting functionality to another function might be a good idea in general. fetch allows to pass a function that returns an agent instead of passing an agent directly.

Only calculate the agent once, before iterating over libs. The library URL always begins with the baseUrl (which contains the schema and host). There is no need to calculate it for every library name, and it would prevent writing many identical logs to the console (which goes to the output channel of the extension).

Sorry, I'm not sure if I understand that correctly, can you give me an example here?

Instead of casting to any you can cast to the expected type (Agent). If typescript doesn't accept it as-is, you can cast to unknown first and then to Agent. It is closer to your intention than any and explains better what you tried to do.

Okay.

[tal-sapan]

Hi @ndrsg,

Yes I considered, but thought better not adding an additional dependency. If you think its a better option to use, e.g. https://github.com/axios/axios , i can also try to integrate it.

This library looks good to me and seems to handle more cases (for example, both lowercase and uppercase proxy env vars, no_proxy env var that contains *). However it seems it has a PR open regarding an issue using a proxy with https schema:
axios/axios#3070
It seems the PR is still being worked on so maybe it will be fixed soon.

Only calculate the agent once, before iterating over libs. The library URL always begins with the baseUrl (which contains the schema and host). There is no need to calculate it for every library name, and it would prevent writing many identical logs to the console (which goes to the output channel of the extension).

Sorry, I'm not sure if I understand that correctly, can you give me an example here?

Sure, I'll explain in more detail.
The place where you added the calculation of the proxy is during the fetch call, which is executed for every UI5 library (sap.m, sap.f, sap.tnt etc) unless it was found in the cache. Each library definition file is downloaded from a different URL which is specific to the library, but all the URLs have the same host, so the proxy configuration will be the same for all of them. Therefore, there is no need to calculate the proxy host for every library URL.
It's not a costly process (especially compared to the actual network call) so it doesn't matter very much, but you print the proxy to the log for every URL. Since all these logs will be the same it's redundant.
If you only perform the calculation once and reuse the same proxy configuration, the log will also be printed once.
This means moving the calculation of the proxy to right before this line:

ui5-language-assistant/packages/language-server/src/ui5-model.ts

Line 54 in 6bdccb0

await Promise.all(

[bd82]

Yes I considered, but thought better not adding an additional dependency. If you think its a better option to use, e.g. https://github.com/axios/axios , i can also try to integrate it.

Generally it is better to add a new dependency for this due to possible edge cases complexity.
However if adding a new dependency requires too much refactoring, or if there are open issues with
the 3rd party library then it's fine to implement our own solution currently and defer upgrade to a 3rd party library to the future.

Please note that process.env is already case insensitive but only on windows.

• https://nodejs.org/api/process.html#process_process_env

I guess it would suffice to handle all upper or all lower case on linux (HTTP_PROXY vs http_proxy)

[babuilyas]

I found a hack resolved this for me in vscode while working on other related issue.
My dev machine runs on Windows, so adopt appropriate your OS.
Windows Shell and Power Shell uses same proxy, so setup it with following command

Step 1: C:>netsh winhttp set proxy http://172.16.0.38:808 bypass-list="localhost;127.0.0.1"
[Requires Admin rights]

Step 2: Launch VSCode from command with
C:\your-ui5-project> code .

Step 3: Optional confirm proxy in VScode terminal with
C:\your-ui5-project> netsh winhttp show proxy
Result: Step 1 proxy output

Step 4: Confirm proxy is working in VScode terminal with
C:\your-ui5-project> curl "https://www.google.com"
Result: StatusCode : 200
StatusDescription : OK

Now try code-completion and inline documentation etc.
It works for me 👍 and should work for you.

Credit goes to @petermuessig

[babuilyas]

Working behind a proxy in Node project is a challenge. Not all module consider the process env proxy settings when making http calls. So try the hack shared above, and your ref. module may be using env proxy settings before http calls.