fix: add options for corporate proxy support #437
Conversation
|
|
| export type Fetcher = (url: string) => Promise<FetchResponse>; | ||
| export type Fetcher = ( | ||
| url: string, | ||
| options: Record<string, unknown> |
There was a problem hiding this comment.
This options object is awfully vague, do we have any more details on it?
| @@ -0,0 +1,25 @@ | |||
| { | |||
| const proxy = getProxy(); | ||
| const fetchOptions = proxy | ||
| ? { | ||
| agent: new ProxyAgent(proxy, { |
There was a problem hiding this comment.
I need to read up on this ProxyAgent.
is it generally safe to use it even when no proxy is needed?
does it have any disadvantage in this flow?
There was a problem hiding this comment.
well if there is no proxy set, it will be a blank object, so no proxyagent. right now it doesn't work at all without this with corporate proxy
| @@ -15,7 +15,7 @@ import { forEach, isPlainObject } from "lodash"; | |||
|
|
|||
| describe("the UI5 language assistant ui5 model", () => { | |||
| // The default timeout is 2000ms and getSemanticModel can take ~3000-5000ms | |||
| const GET_MODEL_TIMEOUT = 10000; | |||
| const GET_MODEL_TIMEOUT = 20000; | |||
There was a problem hiding this comment.
Is the increase timeout because your internet connection is slow or because the usage of the proxy agent causes a performance regression?
There was a problem hiding this comment.
i'll recheck this, it was especially when i tried to use axios, might not need the increase anymore.
| @@ -283,7 +284,7 @@ context("The ui5-language-assistant semantic model package API", () => { | |||
| }); | |||
| // TODO: assert no cyclic references in extends or implements or parent - maybe not in a test | |||
| }); | |||
| }); | |||
| }).timeout(GET_MODEL_TIMEOUT); | |||
There was a problem hiding this comment.
if there was no timeout before, does it mean it was the default 2000ms?
if so what changed that requires x10 larger timeout?
There was a problem hiding this comment.
will check this again with the new proxy
| @@ -3,7 +3,8 @@ | |||
| "compilerOptions": { | |||
| "rootDir": ".", | |||
| "outDir": "lib", | |||
| "baseUrl": "." | |||
| "baseUrl": ".", | |||
| "strictNullChecks": false | |||
| @@ -2,6 +2,8 @@ import { zipObject, keys, map, noop } from "lodash"; | |||
| import { resolve } from "path"; | |||
| import { writeFile, mkdirs, pathExists } from "fs-extra"; | |||
| import fetch from "node-fetch"; | |||
| import getProxy from "get-proxy"; | |||
There was a problem hiding this comment.
test-utils does an import for these proxy related packages but without declaring any deps.
Only the types packages are declared but that is not sufficient for runtime.
This works because of how yarn hoists packages to the root node_modules dir, but it is still in-correct deps management when we do not declare a runtime dependency...
| agent: new ProxyAgent(proxy, { | ||
| // Options, with all defaults | ||
| tunnel: true, // If true, will tunnel all HTTPS using CONNECT method | ||
| timeout: 5000, // Time in milli-seconds, to maximum wait for proxy connection to establish |
There was a problem hiding this comment.
is 5000 a reasonable timeout? or too large?
There was a problem hiding this comment.
the 5000 was the default from the repo
|
Closed in favor of #475 |
fix #285