Functional options to create a client

[ernestoalejo]

I would like to discuss the design of the new client functions, to propose a new one with functional options

Currently we need this to create a client:

client := haberdasher.NewHaberdasherProtobufClient("http://localhost:8000", &http.Client{})
client := haberdasher.NewHaberdasherJSONClient("http://localhost:8000", &http.Client{})

This could be simplified to sane defaults & still allow for customizations of everything with a single function:

// Defaults to protobuf & http.DefaultClient.
client := haberdasher.NewHaberdasherClient("http://localhost:8000")

// Customize the client
client := haberdasher.NewHaberdasherClient("http://localhost:8000", twirp.WithClient(&http.Client{}))

// Customize the format.
client := haberdasher.NewHaberdasherClient("http://localhost:8000", twirp.WithJSON())

// Customize the format & client.
client := haberdasher.NewHaberdasherClient("http://localhost:8000", twirp.WithClient(&http.Client{}), twirp.WithJSON())

This would also give freedom to add new options if a new requirement is found.

What do you think?

[spenczar]

Thanks for the thoughtful feature request! This is an interesting idea.

You outlined the benefits well: it allows freedom in the future if we want to add more options to the client.

I think it's probably overkill for right now, and may even be a little bit worse; a two-parameter constructor accepting an http.Client is going to be very, very clear to anyone, even if they aren't familiar with functional options as a style. I like how simple the current thing is.

So then, we're weighing the potential future benefit of flexibility against this (admittedly small) loss of clarity. What are we really future-proofing here, though - are we really going to be adding many features to clients? I sure hope not.

Twirp's core principle is simplicity, which I think it points us toward keeping things the way they are now. If, in the future, we do need to add more options (even just one or two more), we can revisit this. But I hope we don't need to do that. I really want to keep Twirp's APIs very lean and focused.

Finally, a minor thing: NewHaberdasherClient is obviously a more appealing name, but it clashes with the gRPC generator's output. We briefly used it at Twitch and had to roll it back, because there are folks who need to generate both gRPC and Twirp clients. We don't want to make their lives any harder than they already are.

Of course, we could do NewHaberdasherTwirpClient, or something, so that's not a huge deal.

[ernestoalejo]

I agree with keeping the client as simple as possible, and I didn't think there was a name collision. I will close the issue then.