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

Combine RpcResponseTransformer and RpcResponseTransformerFor #3162

Merged
merged 1 commit into from
Sep 1, 2024

Conversation

lorisleiva
Copy link
Collaborator

@lorisleiva lorisleiva commented Aug 27, 2024

This PR combines the RpcResponseTransformer and RpcResponseTransformerFor types by using an optional type parameter.

Initially, RpcResponseTransformer was defining a generic function (as opposed to a generic type that applies its type parameter to the function). However, this was only needed in the rpc-api file to do:

config.responseTransformer<TypeExpectationHere>;

The same can be achieved using:

config.responseTransformer as RpcResponseTransformer<TypeExpectationHere>;

Without loosening any types as config.responseTransformer<T> was equivalent to a type-cast.

This makes it less tedious to define response transformers as we don't have to always cast our return type to an abstract type T even when we know the real return type.

Copy link

changeset-bot bot commented Aug 27, 2024

⚠️ No Changeset found

Latest commit: 48fa39a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

@lorisleiva lorisleiva force-pushed the loris/add-more-request-transformers branch from 26f7290 to 800d312 Compare August 30, 2024 16:38
@lorisleiva lorisleiva force-pushed the loris/combine-rpc-response-transformer-types branch from dcb68fd to 2a3aa2f Compare August 30, 2024 16:38
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

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:36 PM EDT: Graphite rebased this pull request as part of a merge.
  • Sep 1, 2:37 PM EDT: @lorisleiva merged this pull request with Graphite.

@lorisleiva lorisleiva changed the base branch from loris/add-more-request-transformers to graphite-base/3162 September 1, 2024 18:32
@lorisleiva lorisleiva changed the base branch from graphite-base/3162 to master September 1, 2024 18:34
@lorisleiva lorisleiva force-pushed the loris/combine-rpc-response-transformer-types branch from 2a3aa2f to 48fa39a Compare September 1, 2024 18:35
@lorisleiva lorisleiva merged commit c312964 into master Sep 1, 2024
8 checks passed
@lorisleiva lorisleiva deleted the loris/combine-rpc-response-transformer-types branch September 1, 2024 18:37
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