-
Couldn't load subscription status.
- Fork 44
Allow passing network options from Entrypoint to Network Provider #652
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow passing network options from Entrypoint to Network Provider #652
Conversation
src/entrypoints/entrypoints.ts
Outdated
| constructor(options: { | ||
| networkProviderUrl: string; | ||
| networkProviderKind: string; | ||
| networkProviderOptions?: NetworkProviderConfig; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the constructors of ProxyNetworkProvider and ApiNetworkProvider, this is named "config". Maybe, for consistency, rename the parameter to networkProviderConfig?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @andreibancioiu, thanks a lot for all the feedback.
I agree with you, I rename it.
src/entrypoints/entrypoints.ts
Outdated
| options.networkProviderOptions = options.networkProviderOptions || {}; | ||
|
|
||
| if (options.clientName) { | ||
| options.networkProviderOptions.clientName = options.clientName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such a line:
options.networkProviderOptions.[...] = ...
Will mutate the input argument networkProviderOptions, which might cause downstream issues (if that input argument is re-used by the caller). Maybe clone the options (below is just an example, not an actual suggestion)?
networkProviderOptions = { ...options.networkProviderOptions, ...{ clientName: options.clientName } };
Maybe a cleaner way would be to simply drop the input argument clientName, and only have networkProviderConfig? However, that would be a breaking change, and we shouldn't apply it (yet!).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of this line either.
I went down this route by mimicking what I saw in the extended Entrypoints, such as
mx-sdk-js-core/src/entrypoints/entrypoints.ts
Line 272 in eec19ba
| options = options || {}; |
I'll try to suggest a better approach.
src/entrypoints/entrypoints.ts
Outdated
| if (options.clientName) { | ||
| options.networkProviderOptions.clientName = options.clientName; | ||
| } else if (options.networkProviderOptions?.headers?.["User-Agent"]) { | ||
| options.networkProviderOptions.clientName = options.networkProviderOptions.headers["User-Agent"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an opaque trick (a hack), and assumes that "client name" and "header user agent" are synonyms for network providers (somehow, it breaks a bit their encapsulation).
Maybe drop this logic, and, if desired, a separate PR can be made, but not for the entrypoints. See the file below for a possible place of adjustment:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you understood, I wanted to avoid doing a breaking change in the active major version.
After looking more closely at the code you suggested, I also think it's good to move it in this dedicated function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About the clientName and User-Agent header, my opinion is:
clientNameis good for general usage, as you want to collect stats. It's easy to set and abstracts the technical implications. And you already build a custom User-Agent from that, that looks like a good approach.- More advanced users may want to force a very specific
User-Agent. This shouldn't be altered later in the Network Provider. - One of them must be specified for sure, otherwise you get the console error message and the "unknown" fallback. With a priority to be discussed (probably User-Agent > clientName).
So yes, it's probably a topic for the next major release.
…ypoint constructor
… feat/entrypoint-network-provider-options
Hello,
Summary
This PR adds support for passing custom network options from the
Entrypointto theNetwork Provider. This allows developers to configure headers and other request-related parameters when initializing the entrypoint.Changes
networkProviderOptionsfield in the Entrypoint options.clientNamecan be set via eitheroptions.clientNameor fallback toUser-Agentheader.Motivation
Provides more flexibility for advanced developers needing to customize the network layer (e.g. for authentication, logging, or custom headers).