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

Use generics for subscriptions responses #356

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HaraldNordgren
Copy link
Contributor

No description provided.

@HaraldNordgren HaraldNordgren changed the title Use generics to avoid type casting Use generics for subscriptions to avoid type casting data channels Sep 28, 2024
Copy link
Collaborator

@benjaminjkraft benjaminjkraft left a comment

Choose a reason for hiding this comment

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

(I know this is still a draft, just figured I'd take a peek -- feel free to ignore!)

This is a neat idea!

I worry that it changes the interface a bit too much though. In the tests it's fine but in real code you often pass around the client or attach it to some struct used for many queries (DI-style).

Of course the most straightforward way would be if we could scope generics to methods, but Go doesn't allow that. I wonder if we could do something equivalent-ish by having two structs:

  1. client (as-is) which users pass around
  2. per-query-client (generic) which we construct inline, use for the query, and then throw away
    That still changes the Client interface probably, which is not ideal but more okay (and for subscriptions it's totally fine since it's brand new). Although maybe then it doesn't help that much, if the interface between 1 and 2 needs to be too complex. Anyway, I haven't thought this through, just sharing an idea!

@HaraldNordgren
Copy link
Contributor Author

HaraldNordgren commented Sep 29, 2024

@benjaminjkraft Thanks a lot for your feedback! I have thought about this a lot, and I think you are absolutely right. Introducing generics on the client layer like I did, would worsen the user experience. So it's not worth doing.

I will reduce the scope of this PR and still see if I find any useful refactor using generics 🤗

@HaraldNordgren HaraldNordgren changed the title Use generics for subscriptions to avoid type casting data channels Use generics for subscriptions and graphql.Response Sep 29, 2024
@HaraldNordgren HaraldNordgren changed the title Use generics for subscriptions and graphql.Response Use generics for subscriptions responses Sep 29, 2024
@HaraldNordgren
Copy link
Contributor Author

HaraldNordgren commented Sep 30, 2024

Hi @benjaminjkraft, I landed on this quite minimal refactor in the end. Let me know if you think it's useful or not.

I could also be fine with throwing this away. My real goal was to avoid the type casting of interfaceChan but I couldn't figure out a way to do it without making the library harder to use, which definitely would have been a bad thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants