Tool 233 extend user agent on js network providers #67
Conversation
src/NetworkProviderConfig.ts
Outdated
| @@ -0,0 +1,5 @@ | |||
| import { AxiosRequestConfig } from 'axios'; | |||
There was a problem hiding this comment.
Filename should be with lower case, for consistency. Name of interface can also match the file name, interface NetworkProviderConfig.
src/apiNetworkProvider.ts
Outdated
|
|
||
| constructor(url: string, config?: AxiosRequestConfig) { | ||
| constructor(url: string, config?: ExtendedAxiosRequestConfig) { |
src/apiNetworkProvider.ts
Outdated
| private backingProxyNetworkProvider; | ||
| private userAgentPrefix = 'sdk-network-providers/api' |
There was a problem hiding this comment.
In the near future, we will unify the JS libraries. Thus, let's use multiversx-sdk/api (or something similar - "multiversx" is necessary, to stand out from other possible libraries that talk to MultiversX APIs).
Constants can also be moved to constants.ts.
src/proxyNetworkProvider.ts
Outdated
|
|
||
| // TODO: Find & remove duplicate code between "ProxyNetworkProvider" and "ApiNetworkProvider". | ||
| export class ProxyNetworkProvider implements INetworkProvider { | ||
| private url: string; | ||
| private config: AxiosRequestConfig; | ||
| private config: ExtendedAxiosRequestConfig; | ||
| private userAgentPrefix = 'sdk-network-providers/proxy' |
There was a problem hiding this comment.
As above, multiversx-sdk/proxy.
src/userAgent.ts
Outdated
| import { AxiosHeaders } from "axios"; | ||
| import { ExtendedAxiosRequestConfig } from "./NetworkProviderConfig"; | ||
|
|
||
| export function setUserAgent(userAgentPrefix: string, config: ExtendedAxiosRequestConfig | undefined) { |
There was a problem hiding this comment.
extendUserAgent or something similar (we are not clearing what has been previously set)?
There was a problem hiding this comment.
We should add a small unit test.
There was a problem hiding this comment.
done, tests added
src/userAgent.ts
Outdated
| import { AxiosHeaders } from "axios"; | ||
| import { ExtendedAxiosRequestConfig } from "./NetworkProviderConfig"; | ||
|
|
||
| export function setUserAgent(userAgentPrefix: string, config: ExtendedAxiosRequestConfig | undefined) { |
There was a problem hiding this comment.
In the current calls of this function, config is never undefined. Thus, we can simplify the function signature (and the first lines of the implementation / dropping the first check).
src/userAgent.ts
Outdated
|
|
||
| const headers = AxiosHeaders.from(config.headers as AxiosHeaders).normalize(true); | ||
| if (!config.clientName) { | ||
| console.log("Can you please provide the client name of the aplication that uses the sdk?") |
There was a problem hiding this comment.
Typo. Can also be as advice instead of question e.g. "please provide ..., to be used for (shallow) analytics."
| }; | ||
|
|
||
| const headers = AxiosHeaders.from(config.headers as AxiosHeaders).normalize(true); | ||
| if (!config.clientName) { |
There was a problem hiding this comment.
Check can be above const headers = ... (so that we have the checks nicely grouped above).
src/userAgent.ts
Outdated
| if (!config.clientName) { | ||
| console.log("Can you please provide the client name of the aplication that uses the sdk?") | ||
| } | ||
| const resolvedClientName = config.clientName || 'unknown'; |
There was a problem hiding this comment.
Placeholder can be moved to constants.ts.
src/userAgent.ts
Outdated
| const resolvedClientName = config.clientName || 'unknown'; | ||
|
|
||
| const currentUserAgent = headers.hasUserAgent() ? headers.getUserAgent() : ''; | ||
| const newUserAgent = `${currentUserAgent} ${userAgentPrefix}${resolvedClientName}`.trim(); |
There was a problem hiding this comment.
A space is missing between userAgentPrefix and resolvedClientName.
src/constants.ts
Outdated
| @@ -3,3 +3,5 @@ import { Address } from "./primitives"; | |||
|
|
|||
| export const MaxUint64AsBigNumber = new BigNumber("18446744073709551615"); | |||
| export const EsdtContractAddress = new Address("erd1qqqqqqqqqqqqqqqpqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqzllls8a5w6u"); | |||
| export const MetricsPrefix = "multiversx-sdk" | |||
There was a problem hiding this comment.
baseUserAgent (or something similar) instead of metricsPrefix?
src/apiNetworkProvider.ts
Outdated
| this.url = url; | ||
| this.config = { ...defaultAxiosConfig, ...config }; | ||
| this.backingProxyNetworkProvider = new ProxyNetworkProvider(url, config); | ||
| const proxyConfig = JSON.parse(JSON.stringify(this.config)); |
There was a problem hiding this comment.
How about structuredClone(), does it help?
There was a problem hiding this comment.
in order to use this I need to upgrade the types/node to a version higher then 17, maybe ww will do that with a major release
| assert.equal(localProxyProvider.config.headers.getUserAgent(), expectedProxyUserAgent); | ||
| }); | ||
|
|
||
| it("should keep the set userAgent and add the sdk to it", async function () { |
danielailie
dismissed stale reviews from danidrasovean and andreibancioiu
via
9f9e675
danielailie
deleted the
TOOL-233-extend-user-agent-on-js-network-providers
branch
No description provided.