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

Rename RpcRequest to RpcApiRequestPlan #3146

Merged
merged 1 commit into from
Sep 1, 2024
Merged

Conversation

lorisleiva
Copy link
Collaborator

@lorisleiva lorisleiva commented Aug 23, 2024

In this PR, we continue cleaning up the type namespace by renaming RpcRequest to RpcApiRequestPlan. This is because:

  • RpcRequest will be used to define a proper request interface that will unify the RPC layers.
  • RpcApiRequestPlan better describes what the type actually does. If you call myRpcApi.myMethod({ foo: 42 }), then you will be given an object that contains everything you need to send it to a transport without actually sending it to a transport. This is the role of the Rpc type which wraps the RpcApi proxy and transforms this RpcApiRequestPlan into a PendingRpcRequest which contains the desired send function.

Copy link

changeset-bot bot commented Aug 23, 2024

🦋 Changeset detected

Latest commit: 769effe

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

@@ -63,7 +63,7 @@ function makeProxy<TRpcMethods, TRpcTransport extends RpcTransport>(

function createPendingRpcRequest<TRpcMethods, TRpcTransport extends RpcTransport, TResponse>(
rpcConfig: RpcConfig<TRpcMethods, TRpcTransport>,
pendingRequest: RpcRequest<TResponse>,
pendingRequest: PendingRpcApiRequest<TResponse>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice. I think the relationship makes sense. Give me a pending RPC API request, and I'll turn it into a pending RPC request using the transport.

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.

I'm a little bearish on the ‘pending’ moniker because that implies that it, alone, will resolve. I think of this object as more of ‘instructions for making a request’ or ‘a recipe for making a request.’ RpcRequestDescriptor? RpcRequestPlan? RpcRequestRecipe?

@lorisleiva
Copy link
Collaborator Author

lorisleiva commented Aug 26, 2024

@steveluscher I don't mind something like RpcApiRequestPlan. An alternative would be to actually return an object with a send function, much like the PendingRpcRequest, expect that this one would require a function that returns a response from a given request. What do you think?

EDIT: I went with RpcApiRequestPlan but let me know if you prefer the alternative.

@lorisleiva lorisleiva force-pushed the loris/rename-rpc-request branch 2 times, most recently from 7ac67e3 to 5b947a4 Compare August 26, 2024 19:48
@lorisleiva lorisleiva changed the title Rename RpcRequest to PendingRpcApiRequest Rename RpcRequest to RpcApiRequestPlan Aug 26, 2024
@lorisleiva lorisleiva marked this pull request as ready for review August 26, 2024 19:56
Copy link
Collaborator Author

lorisleiva commented Sep 1, 2024

Merge activity

  • Sep 1, 2:06 PM EDT: @lorisleiva started a stack merge that includes this pull request via Graphite.
  • Sep 1, 2:09 PM EDT: Graphite rebased this pull request as part of a merge.
  • Sep 1, 2:10 PM EDT: @lorisleiva merged this pull request with Graphite.

@lorisleiva lorisleiva changed the base branch from loris/rename-rpc-response to graphite-base/3146 September 1, 2024 18:07
@lorisleiva lorisleiva changed the base branch from graphite-base/3146 to master September 1, 2024 18:07
@lorisleiva lorisleiva merged commit 628177f into master Sep 1, 2024
7 checks passed
@lorisleiva lorisleiva deleted the loris/rename-rpc-request branch September 1, 2024 18:10
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 16, 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.

3 participants