Decorators don't seem to be invoked in some circumstances, unless explicitly asked for in the invoked function
preslavrachev opened this issue · 6 comments
My understanding of decorators was of a way to potentially extend, or even overwrite specific dependencies. This idea has served me well, until I figured out what seems to be an edge case. Check the sample code below (or try it in the Playground):
package main
import (
"log"
"go.uber.org/fx"
)
type Config struct {
a string
}
type Bar struct {
val string
}
type Foo struct {
Bar *Bar
}
func core() fx.Option {
return fx.Provide(
func() Config {
log.Println("Instantiating Abc")
return Config{}
},
func(a Config) *Bar {
log.Println("Instantiating Bar from the core provider")
return &Bar{val: "abc"}
},
func(b *Bar) *Foo {
log.Println("Instantiating Foo")
return &Foo{Bar: b}
},
)
}
func main() {
app := fx.New(
core(),
fx.Decorate(
func(a Config) Config {
log.Println("Decorating Abc")
a.a = "b"
return a
},
func(a Config) *Bar {
log.Println("Instantiating Bar from the decorator", a)
return &Bar{val: "xyz"}
},
),
fx.Invoke(run),
)
app.Run()
}
func run(f *Foo, /* The decorator will be invoked only, if I explicitly ask for Config here */) {
log.Println(f.Bar.val)
}The decorator for Config will not be called, unless I specifically ask for a Config instance in my run function. The fact that Config is not a pointer makes no difference here. Why should I be required to ask for a Config instance in the invoked function, if I am not going to need it? In my mind, this seems like a potential loophole for bugs.
Either that, or perhaps, I misunderstood how decorators work.
Hey @preslavrachev, thanks for bringing up this issue. We're actually aware of this issue and already fixed it in uber-go/dig#329 which has been shipped as part of the dig v1.14.1 release. We'll also be doing a release on fx shortly that uses this version of Dig so you can wait till that too.
Will close this issue but let me know if you have any further questions regarding this.
@sywhang Fantastic! It's great that you guys already saw and fixed that. Please, drop me a line when you get the latest fx release out, so I can update our modules accordingly. Thanks!
@preslavrachev fx version v1.17.1 was released earlier today. Feel free to try it out and let me know if you have any questions :) .
@sywhang Sorry to disturb you again, but I looked at https://github.com/uber-go/fx/blob/v1.17.1/go.mod and it seems like it still refers to the old dig version. My quick local test shows that simply upgrading to fx v1.17.1 did not resolve the issue (assuming, because of the old dig version)
@preslavrachev omg you are right. I accidentally omitted that change before releasing the new version...Sorry about that!
Looks like you already figured out the workaround (pinning Dig to v1.14.1 manually).
Thanks for reporting this, and you shouldn't need this workaround starting the next release of fx.
@sywhang Thank you 👍