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

🤔 Using interface{} to support int or string addresses is confusing #555

Closed
peterbourgon opened this issue Jul 6, 2020 · 7 comments
Closed

Comments

@peterbourgon
Copy link

This is not strictly a bug, but neither is it a feature request; please feel free to reclassify.

// Listen serves HTTP requests from the given addr or port.
// You can pass an optional *tls.Config to enable TLS.
func (app *App) Listen(address interface{}, tlsconfig ...*tls.Config) error {
	// Convert address to string
	addr, ok := address.(string)
	if !ok {
		port, ok := address.(int)
		if !ok {
			return fmt.Errorf("listen: host must be an `int` port or `string` address")
		}
		addr = strconv.Itoa(port)
	}
	if !strings.Contains(addr, ":") {
		addr = ":" + addr
	}
	. . .

Using the empty interface{} to express "int or string" is highly nonidiomatic and confusing to callers. Go is not a dynamically typed language; its users expect to leverage types to help them read and write their programs. Reading the API docs for this method, it's not clear what type I should pass for address. I see that you allow ints or strings, presumably intended as a convenience, but there's no way for me to know that without reading the source. It's also different to the way essentially every other Listen-type API works, i.e. they take an addr string — which, for the record, can be either host:port or simply :port to bind on all interfaces.

@Fenny
Copy link
Member

Fenny commented Jul 7, 2020

Welcome @peterbourgon!
In short, I agree with your statement but let me try to explain the design choice and why we still use it today.

Fiber started as a private project in 2019 to experiment and maybe port some of my microservices running on Express to Go. My goal was to create an identical API, but quickly came to a conclusion this was almost impossible to pull off ( no support for overloading of methods and operators ).

I had to make some design choices to use interface{} or variadic arguments in some methods to pull off the Identical API idea without bloating the API design. A good example would be app.Use(), I knew this would be a key feature in the design that must support a prefix input or a single handler to mimic the Express middleware routing behavior.

app.Use(handler)
app.Use("/api", handler)
app.Use("/api", handler, handler, handler)

If I could go back in time, maybe I would have done it differently. The reason we are still using type conversion or variadic arguments for a small set of methods is to avoid breaking changes for those who are running Fiber in production.

I'm happy you opened this issue, this means the API documentation is not enough to avoid confusion. Therefore I suggest we improve the code comments for the Listen method to display the possible signatures in the IDE ( like ctx.go#L273 ).

After saying the above, we could change this in v2 or add all possible signatures to the current API

func (app *App) Listen(address interface{}, tlsconfig ...tls.Config) error {}
func (app *App) Listener(ln net.Listener) error
func (app *App) ListenerWithTls(ln net.Listener, tlsconfig *tls.Config) error
func (app *App) ListenOnPort(port int) error {}
func (app *App) ListenOnAddress(address string) error {}
func (app *App) ListenOnPortWithTls(port int, tlsconfig *tls.Config) error {}
func (app *App) ListenOnAdressWithTsl(address string, tlsconfig *tls.Config) error {}

I prefer the first, but let me know what you think 👍

@Fenny Fenny changed the title 🐛 Using interface{} to support int or string addresses is confusing 🤔 Using interface{} to support int or string addresses is confusing Jul 7, 2020
@thomasvvugt
Copy link
Contributor

thomasvvugt commented Jul 9, 2020

Hi @peterbourgon and @Fenny,

Thanks for adding the possible signatures in the API documentation.
Despite I wouldn't recommend variadic or interface{} arguments in 99% of the cases, I do agree with Fenny here.

Modifying methods like fiber.Listen() and fiber.Use() to avoid sticking to Express's behavior is highly impacting for those running Fiber in production.
Therefore I'd like to propose that during the development of v2, methods using these sorts of arguments should be reviewed and their potential to be worked out differently.

Thanks for bringing this up nonetheless! 👍🏼

@peterbourgon
Copy link
Author

peterbourgon commented Jul 9, 2020

My goal was to create an identical API

This is the heart of the discussion, I think.

Given this goal, there's a tension between keeping things familiar to users coming from a different language/framework, and keeping things sensical and idiomatic in the actual language of implementation. I think it's great to try and make a similar API, but if there are situations where creating an identical API would result in highly nonidiomatic code, or code that subverts essential properties of the language, I think it's important to defer to the language.

I think it's important because it's ultimately harmful to your users — and to their customers! — to suggest that programming in Go is essentially the same as programming in Javascript. The two languages are sufficiently different that using them successfully — correctly, without bugs, and without performance problems — requires a different mental model, and different programming behavior.

In my opinion, this is a case where aiming for an identical API to Fiber has produced code that subverts essential properties of the actual implementation language. Go is not a dynamically typed language, and while the empty interface interface{} gives you some of the properties of dynamic typing, it comes with several drawbacks: performance costs related to type assertions, safety costs from shifting what would be compile-time errors to runtime errors, and — my main point — readability/coherence costs from not knowing what are valid types for arguments.

In dynamic languages, programmers might be used to paying these costs by default — either implicitly, e.g. automatic type boxing/unboxing and the possibility of runtime type errors; or explicitly, e.g. always needing to consult documentation to know what are the valid types for a parameter. But one important and rather central characteristic of statically typed languages is that programmers shouldn't be paying these costs by default.

I'm happy you opened this issue, this means the API documentation is not enough to avoid confusion.

I think one point I'm trying to make is that, in statically typed languages, users shouldn't need to consult API documentation to figure out what are the valid types for arguments to a function. Documentation should describe the behavior and semantics of a function, not it's mechanical characteristics.

A good example would be app.Use(), I knew this would be a key feature in the design that must support a prefix input or a single handler to mimic the Express middleware routing behavior.

app.Use(handler)
app.Use("/api", handler)
app.Use("/api", handler, handler, handler)

Another point I'm trying to make is that, in statically typed languages, a function that supports multiple different types and number of arguments is actually dangerous, not convenient. You're giving up an essential property of the language, which is compile-time type checking, to produce an API that can be expressed equally conveniently as e.g.

func (a *App) Use(path string, handlers... Handler)
func (a *App) UseRoot(handlers... Handler)

func (app *App) Listen(address interface{}, tlsconfig ...tls.Config) error {}
func (app *App) Listener(ln net.Listener) error
func (app *App) ListenerWithTls(ln net.Listener, tlsconfig *tls.Config) error
func (app *App) ListenOnPort(port int) error {}
func (app *App) ListenOnAddress(address string) error {}
func (app *App) ListenOnPortWithTls(port int, tlsconfig *tls.Config) error {}
func (app *App) ListenOnAdressWithTsl(address string, tlsconfig *tls.Config) error {}

With app.Use, there were two valid ways to call it: with an explicit route and set of handlers, or with no explicit route (presumably defaulting to /?) and a set of handlers. In that case, since there are only two forms, and since the second form is essentially sugar for the first, two top-level functions is a good way to express the API.

With app.Listen, there are apparently many valid ways to call it, with many combinations of valid inputs. In this case, one common approach is with a config struct. Here's one way that might work.

type Config struct {
	Listener net.Listener // used by default
	Address  string       // used if Listener is not provided, syntax is ":8080" or "localhost:8080"
	TLS      *tls.Config  // optional
}

There is no need to allow both int and string types to be provided as the address parameter — another example of, in my opinion, sacrificing essential type safety for superficial convenience. The config struct approach permits callers to use it, conveniently and with type safety, as e.g.

a.Listen(app.Config{Listener: ln}) // one form
a.Listen(app.Config{Address: ":8080"}) // another form
a.Listen(app.Config{Address: "localhost:8000", TLS: &tls.Config{...}})

I've presented my case, but this is your project, and of course you make the decisions :)

@kiyonlin
Copy link
Contributor

Hi, @peterbourgon
I think there is no big problem with using interface{} in Listen at least semantically. If the users have contacted a similar web framework before, they will know what to pass here, because the function signature has already stated that it is an address; if users have never used these web framework, even if the address is of type string, they will not know what to pass in. And they may confuse with host:port and :port, what's the difference?

Furthermore, even if the type of address is clearly string, if the user does not read the documentation or source code, there are also many confusions with stdlib's http.ListenAndServe(addr string, handler Handler)(just talk about addr):

  • "" is legal and will be replaced with ":http"
  • ":8080" is legal
  • "localhost" is legal
  • "127.0.0.1:" is legal and will randomize the port
  • "[::1]" is a legal IPv6 address
  • ...

In short, I don’t think using interface{} in Listen will cause trouble to experienced developers. But if interface{} is abused in some other scenarios, it may cause various problems as you said.

Thanks.

@peterbourgon
Copy link
Author

I think there is no big problem with using interface{} in Listen at least semantically.

In Go, interface{} carries specific semantic implications, which this usage does not meet. It is also a "feature of last resort" to be used only when necessary, not a general-purpose tool like e.g. Java Objects.

Furthermore, even if the type of address is clearly string, if the user does not read the documentation or source code, there are also many confusions with stdlib's http.ListenAndServe(addr string, handler Handler)(just talk about addr):

Yes, that's true. My point was there is a categorical difference between not knowing the acceptable schema of the string, and not knowing what types are acceptable.

@kiyonlin
Copy link
Contributor

@peterbourgon
I agree with your talk about interface{} in Go.

The semantics I am talking about are more natural language semantics than procedural language semantics. In this case, the concept of type is indeed ignored.

And I get your point. I can only say that using an explicit type would be more rigorous. But in terms of use, I feel like I said above, there will be no major problems. Those who understand will know what argument is passed in.

I am very happy to discuss with you, and I feel that you have a deeper understanding of programming languages.

@Fenny
Copy link
Member

Fenny commented Jul 12, 2020

@peterbourgon thank you're providing valuable feedback that might change my mind regarding some design decisions, much appreciated 🍺 This is something that is definitely gonna be discussed when we start drawing our v2 plans.

We discussed the possibilities for changes in v1 for many days in our Discord channel, but we all agreed that it won't be doable and stay backward compatible. Stability favors this choice, feel free to close this issue unless you have further comments or ideas.

Thanks!

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

4 participants