Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

Split RpcResponse into RpcResponse and RpcTransportResponse #3170

Closed

Conversation

lorisleiva
Copy link
Contributor

@lorisleiva lorisleiva commented Aug 27, 2024

This PR renames the current RpcResponse type to RpcTransportResponse and adds a new type definition for RpcResponse which is more consistent with the new RpcRequest definition (from the previous PR).

The benefit of this is that, when defining RpcResponseTransformers, we no longer need to worry about transformer both the json and text paths. Now, only the json path matter. However, if your transformer needs to affect how that JSON is initially parsed from a string, then the fromText function may be added to the RpcResponse object.

The slight annoyance is that, in order to achieve this, we need some weird deferred logic which may confuse people looking at the internals of the RPC proxy.

let response: RpcResponse<TResponse> = { json: () => transportResponse.json() };
const rawResponse: RpcResponse<TResponse> = {
json: async () => {
if (response.fromText) {
return response.fromText(await transportResponse.text()) as TResponse;
} else {
return await transportResponse.json();
}
},
};
response = responseTransformer ? responseTransformer(rawResponse, request) : rawResponse;
return await response.json();

As you can see we need the final response object in order to define the rawResponse object which is then transformed into a response object. It works because the json function is called after all this is resolved but it's a bit weird to read.


Here are some diagrams illustrating the RPC changes made by this PR.

Current state

Current 2 Rpc Refactoring@2x

Proposed state

Proposed 2 Rpc Refactoring@2x

Copy link

changeset-bot bot commented Aug 27, 2024

🦋 Changeset detected

Latest commit: 37d2420

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

@lorisleiva lorisleiva force-pushed the loris/split-rpc-response branch from f7a09e2 to 8a15aad Compare August 27, 2024 14:21
@lorisleiva lorisleiva force-pushed the loris/split-rpc-response branch from 8a15aad to 37d2420 Compare August 27, 2024 14:26
@lorisleiva lorisleiva marked this pull request as ready for review August 27, 2024 14:35
@lorisleiva
Copy link
Contributor Author

We've decided to take another approach on this.

@lorisleiva lorisleiva closed this Aug 28, 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 12, 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.

1 participant