Remove axios dependency #374
Conversation
| "Accept-Encoding": "gzip,deflate", | ||
| Connection: "Keep-Alive", | ||
| }), | ||
| keepalive: false, |
There was a problem hiding this comment.
we do this elsewhere ? (keepalive:false)
There was a problem hiding this comment.
Ok so before we had this code:
if (isNodeLike) {
// keepAlive pools and reuses TCP connections, so it's faster
axiosProps.httpAgent = new http.Agent({ keepAlive: true });
axiosProps.httpsAgent = new https.Agent({ keepAlive: true });
}So it should be keepalive: true I guess.
I assume the reason we only did it in node before is because it wasn't supported in browser, so now we should always have it on?
| } | ||
| } | ||
| const response = await fetch(this.getAuthMetadataEndpointFromClusterUri(kustoUri), { | ||
| // TODO - is this still true for fetch? |
There was a problem hiding this comment.
Its easy to test - just use a not real cluster url
although btw i think this is not the behavior in c# - if cluster doesnt response we throw in C# ...
Or is the case here needs to be a real cluster that is just down (also easy to test)
There was a problem hiding this comment.
So I see it's working, though I'm not 100% about if it's the same scenario.
I think we can remove this for now and see if someone complains?
| timeout, | ||
| body: payload, | ||
| headers: { ...this._baseRequest.headers, ...headers }, | ||
| duplex: isPayloadStream ? "half" : undefined, |
There was a problem hiding this comment.
NodeJS throws if it's not set. I'll add a comment.
Like Gil suggested.
It makes the code simpler, and doesn't seem to have problems for the browser