add useQueryClientResolver to rpc client #720
Conversation
| }) => { | ||
| const tmClient = await connectComet(rpcEndpoint); | ||
| const client = new QueryClient(tmClient); | ||
| let client = queryClientResolver ? await queryClientResolver(rpcEndpoint) : await createConnectCometQueryClient(rpcEndpoint); |
There was a problem hiding this comment.
should still be const, right?
| let client = queryClientResolver ? await queryClientResolver(rpcEndpoint) : await createConnectCometQueryClient(rpcEndpoint); | |
| const makeClient = queryClientResolver || createConnectCometQueryClient; | |
| const client = await makeClient(rpcEndpoint); |
| }: { | ||
| rpcEndpoint: string | HttpEndpoint; | ||
| queryClientResolver: (rpcEndpoint: string | HttpEndpoint) => Promise<QueryClient>; |
There was a problem hiding this comment.
isn't this optional?
| queryClientResolver: (rpcEndpoint: string | HttpEndpoint) => Promise<QueryClient>; | |
| queryClientResolver?: (rpcEndpoint: string | HttpEndpoint) => Promise<QueryClient>; |
"resolver" is a little confusing to me. This makes a client. Consider treating the current behavior as the an explicit default:
| queryClientResolver: (rpcEndpoint: string | HttpEndpoint) => Promise<QueryClient>; | |
| makeClient?: (rpcEndpoint: string | HttpEndpoint) => Promise<QueryClient> = createConnectCometQueryClient; |
| @@ -40,3 +40,15 @@ export const createRpcClient = async (rpcEndpoint: string | HttpEndpoint) => { | |||
|
|
|||
| return rpc; | |||
| } | |||
|
|
|||
| export const createTm34QueryClient = async (rpcEndpoint: string | HttpEndpoint) => { | |||
There was a problem hiding this comment.
why is this change necessary?
There was a problem hiding this comment.
Please excuse me, I think I need to keep this as the default values for makeClient
| @@ -443,6 +443,8 @@ See [LCD Clients](#lcd-clients) for more info. | |||
| | `rpcClients.scopedIsExclusive` | will allow both scoped bundles and all RPC Clients | `true` | | |||
| | `rpcClients.enabledServices` | which services to enable | [`Msg`,`Query`,`Service`] | | |||
| | `rpcClients.instantOps` | will generate instant rpc operations in the file `service-ops.ts` under root folder, which contains customized classes having selected rpc methods | `undefined` | | |||
| | `rpcClients.useConnectComet` | will use connectComet function to get a tendermint client | `undefined` | | |||
| | `rpcClients.useQueryClientResolver` | allow user to pass a query client resolver to create query client in createRPCQueryClient function | `undefined` | | |||
There was a problem hiding this comment.
does this need to be a codegen option? couldn't the code just allow it always as an optional param?
There was a problem hiding this comment.
I believe we have to do it for people who don't want generate this code.
| @@ -43,7 +43,7 @@ export function buildQuery<TReq, TRes>(opts: QueryBuilderOptions<TReq, TRes>) { | |||
|
|
|||
| export interface ITxArgs<TMsg> { | |||
| signerAddress: string; | |||
| message: TMsg; | |||
| message: TMsg | TMsg[]; | |||
There was a problem hiding this comment.
is this change intentional? seems unrelated to the PR
There was a problem hiding this comment.
It's a minor fix for other purpose, never mind about this.
There was a problem hiding this comment.
Higher level… I see this is the status quo:
export const createRPCQueryClient = async ({
rpcEndpoint
}: {
rpcEndpoint: string | HttpEndpoint;
}) => {
const tmClient = await connectComet(rpcEndpoint);
const client = new QueryClient(tmClient);What I'd really like is for createRPCQueryClient to do less. It shouldn't even need to import connectComet, imo. That alone pulls in Axios which is quite opinionated.
What I'd really like to see is for createRPCQueryClient to itself take the Tendermint34Client value that needs to be passed to the QueryClient constructor. In some other module you could have a convenience factory that takes an HttpEndpoint and chooses an HTTP client but this module should prescribe the query transport.
Would that be feasible? Then you wouldn't need a config option that complicates your docs, testing and maintenance. It's just a more flexible factoring.
|
I don't why this's merged, I didn't press any button yet, strange. |
Thank you very much for you review, we'll have a better way to handle this after some higher priority issues are handled. |
No description provided.