-
Notifications
You must be signed in to change notification settings - Fork 208
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
Build value group values using their original scope #393
Merged
JacobOaks
merged 1 commit into
uber-go:master
from
JacobOaks:value_groups_decorator_scope_fix
Aug 4, 2023
Merged
Build value group values using their original scope #393
JacobOaks
merged 1 commit into
uber-go:master
from
JacobOaks:value_groups_decorator_scope_fix
Aug 4, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We call simple providers with their original scope. (Ref: https://github.com/uber-go/dig/blob/master/param.go#L287) This allows decorators to be applied even if the constructor is exported via `dig.Export(true)`. However, we call value group value providers with whatever scope we found them in: (Ref: https://github.com/uber-go/dig/blob/master/param.go#L609) This has the consequence of causing exported value group value providers within a module to not be able to be decorated by decorators within the same module, when using `dig.Export(true)`, unlike their simple provider counterpart. For example (playground link: https://go.dev/play/p/R2uqaTvT57P): ```go type Foo struct{} type FooResults struct { dig.Out Foo Foo `group:"foos"` } func NewFoo(s string) FooResults { fmt.Printf("String in NewFoo: %q\n", s) return FooResults{ Foo: Foo{}, } } type Bar struct{} func NewBar(s string) Bar { fmt.Printf("String in NewBar: %q\n", s) return Bar{} } type UseFooAndBarParams struct { dig.In Foos []Foo `group:"foos"` Bar Bar } func UseFooAndBar(UseFooAndBarParams) {} func main() { c := dig.New() c.Provide(func() string { return "base" }) child := c.Scope("child") child.Decorate(func(s string) string { return s + "-decorated" }) child.Provide(NewFoo, dig.Export(true)) child.Provide(NewBar, dig.Export(true)) } // Output: // String in NewFoo: "base" // String in NewBar: "base-decorated" ``` Since we use `dig.Export(true)` by default in Fx, this is the default behavior for value group providers, see uber-go/fx#1104 This commit changes `callGroupProviders` to use the value group provider's original scope as well, and adds a test to verify the behavior is fixed.
Codecov Report
@@ Coverage Diff @@
## master #393 +/- ##
=======================================
Coverage 98.39% 98.39%
=======================================
Files 22 22
Lines 1492 1492
=======================================
Hits 1468 1468
Misses 15 15
Partials 9 9
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
sywhang
approved these changes
Aug 3, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this! LGTM.
alexisvisco
pushed a commit
to alexisvisco/dig
that referenced
this pull request
Aug 31, 2023
We call simple providers with their original scope. (Ref: https://github.com/uber-go/dig/blob/master/param.go#L287) This allows decorators to be applied even if the constructor is exported via `dig.Export(true)`. However, we call value group value providers with whatever scope we found them in: (Ref: https://github.com/uber-go/dig/blob/master/param.go#L609) This has the consequence of causing exported value group value providers within a module to not be able to be decorated by decorators within the same module, when using `dig.Export(true)`, unlike their simple provider counterpart. For example (playground link: https://go.dev/play/p/R2uqaTvT57P): ```go type Foo struct{} type FooResults struct { dig.Out Foo Foo `group:"foos"` } func NewFoo(s string) FooResults { fmt.Printf("String in NewFoo: %q\n", s) return FooResults{ Foo: Foo{}, } } type Bar struct{} func NewBar(s string) Bar { fmt.Printf("String in NewBar: %q\n", s) return Bar{} } type UseFooAndBarParams struct { dig.In Foos []Foo `group:"foos"` Bar Bar } func UseFooAndBar(UseFooAndBarParams) {} func main() { c := dig.New() c.Provide(func() string { return "base" }) child := c.Scope("child") child.Decorate(func(s string) string { return s + "-decorated" }) child.Provide(NewFoo, dig.Export(true)) child.Provide(NewBar, dig.Export(true)) } // Output: // String in NewFoo: "base" // String in NewBar: "base-decorated" ``` Since we use `dig.Export(true)` by default in Fx, this is the default behavior for value group providers, see uber-go/fx#1104 This commit changes `callGroupProviders` to use the value group provider's original scope as well, and adds a test to verify the behavior is fixed.
JacobOaks
added a commit
to JacobOaks/fx
that referenced
this pull request
Dec 6, 2023
This upgrades Fx to use the latest Dig version 1.17.1, which contains a bug fix that would solve issues uber-go#1104 and uber-go#1137 (uber-go/dig#393).
JacobOaks
added a commit
to uber-go/fx
that referenced
this pull request
Dec 6, 2023
This upgrades Fx to use the latest Dig version 1.17.1, which contains a bug fix that would solve issues #1104 and #1137 (uber-go/dig#393).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We call simple providers with their original scope. (ref) This allows decorators to be applied even if the constructor is exported via
dig.Export(true)
.However, we call value group value providers with whatever scope we found them in: (ref)
This has the consequence of causing exported value group value providers within a module to not be able to be decorated by decorators within the same module, when using
dig.Export(true)
, unlike their simple provider counterpart.For example (playground):
Since we use
dig.Export(true)
by default in Fx, this is the default behavior for value group providers, see uber-go/fx#1104This commit changes
callGroupProviders
to use the value group provider's original scope as well, and adds a test to verify the behavior is fixed.