improbable-eng / grpc-web

gRPC Web implementation for Golang and TypeScript
Apache License 2.0
4.39k stars 436 forks source link

Detach IsGrpcWebRequest and IsGrpcWebSocketRequest from WrappedGrpcServer #1118

Open mholt opened 2 years ago

mholt commented 2 years ago

The methods IsGrpcWebRequest and IsGrpcWebSocketRequest have WrappedGrpcServer as a receiver, but they do not use WrappedGrpcServer. They could just be functions: grpcweb.IsGrpcWebRequest() etc.

I am integrating this library into a Caddy plugin and being able to call those methods without having to make a WrappedGrpcServer would be ideal. For now I am just copying+pasting the one-line within them.

Edit: Actually, I think what I really want is for a way to bridge gRPC-web without having to wrap an http.Handler.

Basically, all I need to do is call w.HandleGrpcWebsocketRequest(resp, req) or w.HandleGrpcWebRequest(resp, req) myself within my own http.Handler. The problem is my handler is dynamic and I don't want to make a WrappedGrpcServer for every HTTP request because that's not efficient. Is/can there a way to do the bridging without wrapping a handler?

Another thought: I noticed that the WrappedGrpcServer doesn't actually persist any state between requests. It holds configuration but no extra state. That's excellent! I wonder if we do not need to create a struct that persists for the lifetime of the server then. In other words, could this package be implemented as a config struct with functions like BridgeGRPCWeb() or similar? (As opposed to an entire Server implementation that wraps handlers, etc.)

johanbrandhorst commented 2 years ago

Hi Matt, I just replied in the other issue. Sorry to have taken this long, I hope you haven't spent too much time digging through our internals, as I said in the other issue, I think you're much better off recreating this logic in Caddy directly.

On another note, check out https://connect.build/ if you want to see another implementation of gRPC-Web in Go.

mholt commented 2 years ago

Thanks! Will do.

I still think the two methods could be detached from the WrappedGrpcServer type for some profit. But not important. :) Thank you again!