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

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

Open
euank opened this issue Aug 14, 2024 · 1 comment
Open

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

euank opened this issue Aug 14, 2024 · 1 comment

Comments

@euank
Copy link
Contributor

euank commented Aug 14, 2024

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
Copy link
Contributor

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

As far as fixing it goes, I hate all of those options 😭 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?

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

No branches or pull requests

2 participants