-
Notifications
You must be signed in to change notification settings - Fork 121
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
e94e0c4
to
81fd7fa
Compare
There was a problem hiding this 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] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) {}
}
There was a problem hiding this comment.
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
What edge cases are those? Do you mind expanding upon? I would like to add a unit test to cover the edge cases.
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 |
This PR introduces a simpler way to define
ServiceHandler
implementations using plain functions, closely mirroring Go’shttp.HandleFunc
.Context
Today,
connect-go
requires creating structs implementing service interfaces. While idiomatic, this:Why This Matters
By adopting a
HandleFunc
-style API:net/http
practices that Go developers already know.Example:
This approach requires no new concepts beyond what every Go developer already uses for HTTP servers.