dig
dig copied to clipboard
Add API to provide varargs
Typically, variadic arguments are optional - the function needs to handle len(varargs)==0 already. Making dig automatically treat varargs as optional would make a wider variety of plain-Go functions work out of the box.
This needs further discussion.
Currently, constructors that have variadic arguments cannot be called from
dig. For NewFoo(...Option) *Foo, you cannot provide []Option or Option.
So before we make variadic arguments optional, we have to figure out how, if
at all, users will be able to Provide them to constructors.
After discussions with @alsamylkin and @akshayjshah, there are two solutions to this:
- Disallow providing variadic arguments to constructors. A function
NewFoo(*Bar, ...Option) *Foomay beProvided into the container but we will supply no method of filling in that...Option. The constructor will be called with just the*Bar. We are categorizing supplying variadic arguments as a complex use case, the kind for which we expect people to usedig.In,dig.Out, or wrapper functions. An advantage of this solution is that we can add support for providing variadic arguments later if a better solution becomes possible.
@alsamylkin suggested that under this solution, if necessary, `fx` could
provide a function like `fx.Expand` that lets users explicitly opt into
vararg-slice conversion. As in,
fx.Provide(fx.Expand(NewFoo))
Is equivalent to,
fx.Provide(func(b *Bar, opts []Option) *Foo { return NewFoo(b, opts...) })
- Allow providing variadic arguments as slices by default (optional as per
this issue). A function
NewFoo(*Bar, ...Option)may beProvided. Additionally, a[]Optionmay optionally be provided. The function will be called with that slice expanded as if withopts.... Arguably, this is how a user would expect to provide values for variadic arguments. Another advantage is that most existing constructors users write will Just Work without any wrapping.
CC @alsamylkin @akshayjshah @breerly @glibsm
I'm fine with the first option above - treating all variadic arguments as unmet optional dependencies - in the short term. If we can't come to agreement quickly, let's land that change and continue discussion.
For a 1.0 release, I think that the second option (providing variadic arguments as slices) is better. One of dig's design goals was to let plain-Go constructor functions work correctly without extra ceremony. Functional options are very common, so we shouldn't treat them as an advanced use case - they're regular constructors, and they should just work.
Concretely, "no extra ceremony" means that for any constructor NewFoo, graph.Provide(NewFoo) should work. Furthermore, changing NewFoo from func(*A) to func(*A, ...FooOption) shouldn't immediately require changes to the way it's provided to dig graphs.
While I see @alsamylkin's point about the syntax for passing arguments, I don't think we should choose an implementation that allows only one option. Since varargs and slices are so closely related in Go, providing them as slices seems both simple and obvious to me. I suspect that most experienced Gophers will feel the same.
I think we see the problem from different sides: callee and the caller. While the callee does receive a slice, the caller doesn't have it, the compiler is going to create it. DIG is the caller in this case, so it needs to find all the arguments with this type, but it doesn't support multiple instances.
So DIG already fails the "no extra ceremony" approach with constructors that expect several arguments of the same type. This is how I see varargs and think it represents a larger portion of issues, that we think is ok to wrap with a struct and named fields.
The reason I prefer the first option is that I can totally be wrong and we can add this feature without breaking changes, while if a second approach will become undesirable, it would be a breaking change. That's why I prefer a more conservative approach.
Some stats about searching for keywords on Github: func New: 1.1M type option: 6K
Retitling this and tagging it as a post-1.0 feature.
Let's plan to revisit this now that we're post 1.0. Value groups are probably the way to go here.