Skip to content

feat: add service struct #853

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

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

Conversation

yordis
Copy link

@yordis yordis commented Jun 19, 2025

This PR introduces a simpler way to define ServiceHandler implementations using plain functions, closely mirroring Go’s http.HandleFunc.

Context

Today, connect-go requires creating structs implementing service interfaces. While idiomatic, this:

  • Forces unnecessary boilerplate and indirection (extra structs, vtable lookups). Note, I am still using an interface for backwards compatible and avoid breaking changes. The long-term ideal solution, we could remove the vtable all together. But it is less of a concern right now.
  • Complicates dependency injection, especially in codebases favoring functional composition.
  • Makes testing harder—requiring mock structs or complex setup, forced to implement the entire interface even when most cases, we only care about one endpoint.

Why This Matters

By adopting a HandleFunc-style API:

  • Handlers can be defined as pure functions without struct wrappers.
  • Testing becomes trivial—just pass closures, no mocks needed.
  • The pattern exactly matches net/http practices that Go developers already know.
  • The change is fully backward-compatible—interfaces remain available unchanged.

Example:

type MyService struct {
   DoSomethingFunc connect.HandleFunc[Request, Response]
}

This approach requires no new concepts beyond what every Go developer already uses for HTTP servers.

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
@yordis yordis force-pushed the add-service-struct branch from e94e0c4 to 81fd7fa Compare July 12, 2025 23:38
@yordis yordis marked this pull request as ready for review July 12, 2025 23:42
Copy link
Contributor

@emcfarlane emcfarlane left a comment

Choose a reason for hiding this comment

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

The current generator would fail to generate correct code in some edge cases. This needs to be fixed.

The generator doesn't require any connect internals or change the current generated code. So before reviewing further, it may be more beneficial to you, for it to be maintained as a separate generator. Otherwise it would be good to open a discussion to weight the pros/cons of this approach. What do you think?

// TestServiceService provides access to the handlers for the
// connect.test.default_package.TestService service.
type TestServiceService struct {
MethodFunc connect.HandlerFunc[gen.Request, gen.Response]
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a potential collision with a protobuf method that has the suffix Func. This would then generate invalid code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the code will potentially collide with a method that has the Func suffix:

service Service {
  rpc Method (MethodRequest) returns (MethodResponse) {}
  rpc MethodFunc (MethodFuncRequest) returns (MethodFuncResponse) {}
}

Copy link
Author

Choose a reason for hiding this comment

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

uuuufff 😭 What do you recommend here? Honestly in over half a decade never came across such naming, but obviously, that is just my experience

@yordis
Copy link
Author

yordis commented Jul 15, 2025

The current generator would fail to generate correct code in some edge cases. This needs to be fixed.

What edge cases are those? Do you mind expanding upon? I would like to add a unit test to cover the edge cases.

it may be more beneficial to you, for it to be maintained as a separate generator.

Would you be open to add the 2 types to the library, I do not mind experimenting outside this generator, having two types allows me to use the package without yet another package dependency just to include the two types in it.

I could just inline the entire types as well, maybe that is what you wish me to do? I can generate some types inline as well as an alias of the thing underneath. Please share your direction here.

Otherwise, this pattern is such common pattern by now, that, here I am.

Also, worth saying that this is not the final outcome of the situation, just a stop gap between getting it out the door and promote http.HandleFunc style, and eventually figure out the core. The only benefits at that point would be avoiding the vtable of that interface, which, maybe isn't even worth it. I am Ok with that as well, at least for now.

@yordis yordis requested a review from emcfarlane July 17, 2025 20:18
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