ngrok / ngrok-go

Embed ngrok secure ingress into your Go apps as a net.Listener with a single line of code.
MIT License
666 stars 64 forks source link

Session.ListenAndHandleHTTP has an awkward to use type signature #178

Open euank opened 1 month ago

euank commented 1 month ago

Problem

The type signature for ListenAndHandleHTTP takes a pointer to an interface (*http.Handler) for the handler instead of just the interface directly.

This is awkward because, for example, the following errors out:

ngrok.ListenAndHandleHTTP(
    context.Background(),
    config.HTTPEndpoint(),
    http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
    }),
)

// cannot use config.HTTPEndpoint() (value of type config.Tunnel) as *"net/http".Handler value in argument to ngrok.ListenAndHandleHTTP: config.Tunnel does not implement *"net/http".Handler (type *"net/http".Handler is pointer to interface, not interface)

I believe it was intended for that last argument to be http.Handler, just like the stdlib http.ListenAndServe takes.

What next

So, how do we solve this?

Well, in my opinion we update it to not be a pointer, but my main question is one of backwards compatibility. Technically that's a breaking change, so do we:

  1. Release v2 (golang.ngrok.com/ngrok/v2)
  2. Add a new method, deprecate the old (ngrok.ListenAndHandleHTTPReal or whatever, naming is hard)
  3. Update it in place, hope no one's using it

It's unfortunate that the v1 to v2 jump in a go module makes the import path more confusing looking (with 'ngrok' no longer being the trailing part), but I figure we'll have to do it eventually anyway, so maybe that's the right answer?

I figured I'd ask to see if we have any opinions on this, happy to put up a PR for whichever if we have consensus on the path!

jrobsonchase commented 4 weeks ago

Well that's annoying. Probably some bad copy-pasta.

As far as fixing it goes, I hate all of those options :sob: But we're actively working on a new API for v2, so we could just wait until we're ready to release it and address this issue at that time.

Another (grosser, imo) option may be to replace the argument type with any, and then do some type assertions to do the right thing in the method body?