Skip to content
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

Clean up SolanaRpcApi: no longer extend RpcApiMethods #3213

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

mcintyre94
Copy link
Contributor

@mcintyre94 mcintyre94 commented Sep 6, 2024

Extending RpcApiMethods removes the ability to access typed keys on SolanaRpcApi etc, because it's typed as [method: string]: RpcApiMethod.

In this PR we change from:

interface GetAssetApi extends RpcApiMethods {

To:

type GetAssetApi = {

(Thanks @lorisleiva!)

This allows createRpcApi to maintain its typing, meaning that it still constrains any API methods we attempt to build an RPC for. But we now have typesafe keyof SolanaRpcApi etc.

I've also removed the export of RpcApiMethods, since this should now be used only as a constraint internally and not a type externally.

I've added a typetest with a bit more detail, but this basically makes this work:

'getAccountInfo' satisfies keyof SolanaRpcApi;
// @ts-expect-error RPC API does not have this method
'someMadeUpMethod' satisfies keyof SolanaRpcApi;

Previously someMadeUpMethod would satisfy it, because keyof SolanaRpcApi was just string.

Copy link

changeset-bot bot commented Sep 6, 2024

🦋 Changeset detected

Latest commit: 728d517

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mcintyre94 mcintyre94 force-pushed the rpc-api-methods-cleanup branch from b48b241 to c483d1e Compare September 6, 2024 15:00
Comment on lines +33 to 34
// @ts-expect-error - Mocking RPC methods
target[p as keyof GraphQLCompliantRpc] = jest
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@buffalojoec I'm not sure why this throws an error with this change, but it seems very test-specific so I'm not too worried about it. LMK if this is pointing at a problem I've overlooked though!

Copy link
Contributor

@lorisleiva lorisleiva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better 👌

Copy link
Contributor Author

mcintyre94 commented Sep 6, 2024

Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I dug up the original reason why I added this.

The change to the API methods went in here, and it was really only to tee up this change which added the createJsonRpcApi generic helper.

It seems like somewhere along the way the helper was updated and it will no longer throw a type error, cool! Seems like it might be the change from this:

const params = config?.parametersTransformer
    ? config?.parametersTransformer(rawParams, methodName)
    : rawParams;
const responseTransformer = config?.responseTransformer
    ? config?.responseTransformer<ReturnType<TRpcMethods[TMethodName]>>
    : (rawResponse: unknown) => rawResponse as ReturnType<TRpcMethods[TMethodName]>;
return {
    methodName,
    params,
    responseTransformer,
};

... to this:

const rawRequest = { methodName, params: rawParams };
const request = config?.requestTransformer
    ? config?.requestTransformer(Object.freeze(rawRequest))
    : rawRequest;
return Object.freeze({
    ...request,
    ...(config?.responseTransformer
        ? {
                responseTransformer: config.responseTransformer as RpcResponseTransformer<
                    ReturnType<TRpcMethods[TMethodName]>
                >,
            }
        : {}),
});

Note: This is mirrored in rpc-subscriptions-api, so that API should also get the same treatment.

@mcintyre94
Copy link
Contributor Author

Note: This is mirrored in rpc-subscriptions-api, so that API should also get the same treatment.

Thanks! Added a new PR to the stack: #3218

Copy link
Collaborator

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added via Giphy

How's autocomplete speed with this; same same?

@mcintyre94
Copy link
Contributor Author

Yep, still seems ~instant for me!

Copy link
Contributor Author

mcintyre94 commented Sep 9, 2024

Merge activity

@mcintyre94 mcintyre94 merged commit 3fc388f into master Sep 9, 2024
9 checks passed
@mcintyre94 mcintyre94 deleted the rpc-api-methods-cleanup branch September 9, 2024 17:11
mcintyre94 added a commit that referenced this pull request Sep 9, 2024
…onsMethods (#3218)

Same idea as #3213 but for subscriptions

Also added a typetest to verify that keys of RpcSubscriptions are now correctly typed
@github-actions github-actions bot mentioned this pull request Sep 10, 2024
Copy link
Contributor

Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants