-
Notifications
You must be signed in to change notification settings - Fork 206
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
Introduce Scope to Dig #305
Conversation
Codecov Report
@@ Coverage Diff @@
## master #305 +/- ##
==========================================
- Coverage 98.54% 98.45% -0.10%
==========================================
Files 18 19 +1
Lines 1172 1228 +56
==========================================
+ Hits 1155 1209 +54
- Misses 10 11 +1
- Partials 7 8 +1
Continue to review full report at Codecov.
|
Marking this as WIP because I want to play around with an alternate implementation for |
provide.go
Outdated
// same types are requested multiple times, the previously produced value will | ||
// be reused. | ||
// | ||
// In addition to accepting constructors that accept dependencies as separate |
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.
More succinct rewording:
Provide accepts argument types or dig.In structs as dependencies as well as separate return values or dig.Out structs for results.
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.
Gave it a high level first pass. Will give it another go later.
I think with the current implementation, invokes will ignore legitimate errors from constructors?
} | ||
n.order = c.newGraphNode(n) | ||
s.newGraphNode(n, n.orders) |
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.
Passing n.orders
into newGraphNode feels like a leak of internal state here,
but I can't think of an obvious alternative to this just yet.
// String representation of the entire Scope | ||
func (s *Scope) String() string { | ||
b := &bytes.Buffer{} | ||
fmt.Fprintln(b, "nodes: {") |
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.
nit, but thought i would call it out: fmt.Fprintln can return an error which isn't handled here.
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.
yea it's a valid point, though we're writing it to a bytes.Buffer and not some external stream, and doing err check for every single Fprintln in this func can be expensive + hurts readability. i can add it though if you think the tradeoff is worth.
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.
nah, i think you are just adding existing code. just thought i would mention in case it comes up again.
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.
FWIW bytes.Buffer.Write always returns a nil error, so checking errors here would be unnecessary. I don't personally use errcheck much, but it will also ignore this case when the writer is known to be a bytes.Buffer at compile time.
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.
lgtm. talked offline about adding permutations for tests for options that could interfere with this diff eg DryRun(true)
and DeferAcyclicVerification()
. Maybe there are others I didn't think of. I think its fine not to do that to existing ones, but for new ones, let's try and test more options.
_, isErrMissingTypes := err.(errMissingTypes) | ||
_, isErrMissingDeps := err.(errMissingDependencies) | ||
if err != nil && !isErrMissingTypes && !isErrMissingDeps { |
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.
I'm not sure I understand what's going here fully:
err
is assigned in a loop above. The value of err
here will be the last thing assigned to it before the loop exits.
So this will report an error only if the a constructor in the middle somewhere failed, but one of the ones after it succeeded.
Is there risk here in implementing the scope-resolution logic at such a high level in the parameter tree?
If we implemented it on the leaf nodes (paramSingle and maybe paramGroupedSlice), we might be able to control how these operate in a multi-scope environment. Like, paramSingle.Build could call getValueProviders on current containerStore, then its parent, and so on until it finds a provider or runs out of stores.
|
||
func newScope() *Scope { | ||
s := &Scope{ | ||
name: "container", |
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.
Two things:
- Is there a reason we went with "container" as the name of the root node instead of empty string?
- Since the name must always be set and is the only thing that changes, should this perhaps be a parameter (
newScope(name)
)?
func (s *Scope) appendLeafScopes(dest []*Scope) []*Scope { | ||
dest = append(dest, s) | ||
for _, cs := range s.childScopes { | ||
dest = cs.appendLeafScopes(dest) | ||
} |
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.
I don't recall if we already discussed naming for this operation.
"leaf scopes" seems inaccurate; these are all nodes in the subtree rooted at this node.
Perhaps "appendSubtree" would be more appropriate?
var stores []containerStore | ||
for s := s; s != nil; s = s.parentScope { | ||
stores = append(stores, s) | ||
} | ||
for i, j := 0, len(stores)-1; i < j; i, j = i+1, j-1 { | ||
stores[i], stores[j] = stores[j], stores[i] | ||
} | ||
return stores |
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.
This is getScopesFromRoot but with a different type.
We can probably just call getScopesFromRoot, and then build a slice casting those elements:
scopes := getScopesFromRoot()
stores := make([]containerStore, len(scope))
for i, s := range scopes {
stores[i] = s
}
// String representation of the entire Scope | ||
func (s *Scope) String() string { | ||
b := &bytes.Buffer{} | ||
fmt.Fprintln(b, "nodes: {") |
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.
FWIW bytes.Buffer.Write always returns a nil error, so checking errors here would be unnecessary. I don't personally use errcheck much, but it will also ignore this case when the writer is known to be a bytes.Buffer at compile time.
This contains some miscellaneous fixes from additional feedback from uber-go#305. Notably: - Change err handling logic during scoped parameter building to be more clear on what it's doing. - Rename appendLeafScopes to appendSubscopes. - Change getStoresFromRoot to use getScopesFromRoot.
* Misc. fixes from #305 This contains some miscellaneous fixes from additional feedback from #305. Notably: - Make provider resolution for parameter building iterate the other way around (from self to root) and break once a provider is found. - Rename appendLeafScopes to appendSubscopes. - Rename getScopesFromRoot to ancestors - Change getStoresFromRoot to use storesToRoot. Co-authored-by: Abhinav Gupta <mail@abhinavg.net> Co-authored-by: Abhinav Gupta <abg@uber.com>
This adds fx.Module Option which is a first-class object for supporting scoped operations on dependencies. A Module can consist of zero or more fx.Options. By default, Provides to a Module is provided to the entire App, but there is a room for adding an option to scope that to a Module. Module can wrap Options such asSupply/Extract, Provide, and Invoke but there are some Options that don't make sense to put under Module. For example, StartTimeout, StopTimeout, WithLogger explicitly errors out when supplied to a Module. Implementation-wise, a Module corresponds to dig.Scope which was added in uber-go/dig#305. Extra bookkeeping is done by the module struct which contains the provides and invokes to a Scope. Co-authored-by: Abhinav Gupta <mail@abhinavg.net> Co-authored-by: Abhinav Gupta <abg@uber.com> Co-authored-by: Abhinav Gupta <mail@abhinavg.net>
This adds fx.Module Option which is a first-class object for supporting scoped operations on dependencies. A Module can consist of zero or more fx.Options. By default, Provides to a Module is provided to the entire App, but there is a room for adding an option to scope that to a Module. Module can wrap Options such asSupply/Extract, Provide, and Invoke but there are some Options that don't make sense to put under Module. For example, StartTimeout, StopTimeout, WithLogger explicitly errors out when supplied to a Module. Implementation-wise, a Module corresponds to dig.Scope which was added in uber-go/dig#305. Extra bookkeeping is done by the module struct which contains the provides and invokes to a Scope. Co-authored-by: Abhinav Gupta <mail@abhinavg.net> Co-authored-by: Abhinav Gupta <abg@uber.com> Co-authored-by: Abhinav Gupta <mail@abhinavg.net>
* fx.Annotate: make variadic params optional by default If annotate is passed a variadic function, the dependency listed as the variadic parameter should be optional unless otherwise specified with the `optional:false` parameter tag. This commit addresses that bug. * Update annotated_test.go Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com> * fx.Annotate: make variadic params optional by default If annotate is passed a variadic function, the dependency listed as the variadic parameter should be optional unless otherwise specified with the `optional:false` parameter tag. This commit addresses that bug. * Update annotated_test.go Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com> * Temporarily pin Dig dependency to master (#827) This temporarily pins the Dig dependency in Fx to the master branch which has dig.Scope in preparation for adding fx.Module which is the corresponding user-facing API in Fx. In addition, this fixes a few tests to expect the new error message format that was changed with the graph refactoring PR in uber-go/dig#301. * Add fx.Module (#830) This adds fx.Module Option which is a first-class object for supporting scoped operations on dependencies. A Module can consist of zero or more fx.Options. By default, Provides to a Module is provided to the entire App, but there is a room for adding an option to scope that to a Module. Module can wrap Options such asSupply/Extract, Provide, and Invoke but there are some Options that don't make sense to put under Module. For example, StartTimeout, StopTimeout, WithLogger explicitly errors out when supplied to a Module. Implementation-wise, a Module corresponds to dig.Scope which was added in uber-go/dig#305. Extra bookkeeping is done by the module struct which contains the provides and invokes to a Scope. Co-authored-by: Abhinav Gupta <mail@abhinavg.net> Co-authored-by: Abhinav Gupta <abg@uber.com> Co-authored-by: Abhinav Gupta <mail@abhinavg.net> * fx.Module: Reorganize code (#832) In #830, I reverted some of the code moves to make reviewing the change easier. This change adds back those moves on top of that PR. This reverts commit e4d006b. This reverts commit 8aa68c0. Co-authored-by: Abhinav Gupta <abg@uber.com> * Finish incomplete merge * Only make variadic params optional if no other tags are specified * remove print statements * Update annotated.go Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com> * add extra test * Fix test "Provide variadic function with no optional params" * Explain why we're adding the optional tag * Fix test case, verified failing on master Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com> Co-authored-by: Sung Yoon Whang <sungyoon@uber.com> Co-authored-by: Abhinav Gupta <mail@abhinavg.net> Co-authored-by: Abhinav Gupta <abg@uber.com>
This adds fx.Module Option which is a first-class object for supporting scoped operations on dependencies. A Module can consist of zero or more fx.Options. By default, Provides to a Module is provided to the entire App, but there is a room for adding an option to scope that to a Module. Module can wrap Options such asSupply/Extract, Provide, and Invoke but there are some Options that don't make sense to put under Module. For example, StartTimeout, StopTimeout, WithLogger explicitly errors out when supplied to a Module. Implementation-wise, a Module corresponds to dig.Scope which was added in uber-go/dig#305. Extra bookkeeping is done by the module struct which contains the provides and invokes to a Scope. Co-authored-by: Abhinav Gupta <mail@abhinavg.net> Co-authored-by: Abhinav Gupta <abg@uber.com> Co-authored-by: Abhinav Gupta <mail@abhinavg.net>
* fx.Annotate: make variadic params optional by default If annotate is passed a variadic function, the dependency listed as the variadic parameter should be optional unless otherwise specified with the `optional:false` parameter tag. This commit addresses that bug. * Update annotated_test.go Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com> * fx.Annotate: make variadic params optional by default If annotate is passed a variadic function, the dependency listed as the variadic parameter should be optional unless otherwise specified with the `optional:false` parameter tag. This commit addresses that bug. * Update annotated_test.go Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com> * Temporarily pin Dig dependency to master (#827) This temporarily pins the Dig dependency in Fx to the master branch which has dig.Scope in preparation for adding fx.Module which is the corresponding user-facing API in Fx. In addition, this fixes a few tests to expect the new error message format that was changed with the graph refactoring PR in uber-go/dig#301. * Add fx.Module (#830) This adds fx.Module Option which is a first-class object for supporting scoped operations on dependencies. A Module can consist of zero or more fx.Options. By default, Provides to a Module is provided to the entire App, but there is a room for adding an option to scope that to a Module. Module can wrap Options such asSupply/Extract, Provide, and Invoke but there are some Options that don't make sense to put under Module. For example, StartTimeout, StopTimeout, WithLogger explicitly errors out when supplied to a Module. Implementation-wise, a Module corresponds to dig.Scope which was added in uber-go/dig#305. Extra bookkeeping is done by the module struct which contains the provides and invokes to a Scope. Co-authored-by: Abhinav Gupta <mail@abhinavg.net> Co-authored-by: Abhinav Gupta <abg@uber.com> Co-authored-by: Abhinav Gupta <mail@abhinavg.net> * fx.Module: Reorganize code (#832) In #830, I reverted some of the code moves to make reviewing the change easier. This change adds back those moves on top of that PR. This reverts commit e4d006b9cb4eb6eede8e1ca672cb4d74f679ddf2. This reverts commit 8aa68c04a85ac15123bdbaffa70ce7dac478660e. Co-authored-by: Abhinav Gupta <abg@uber.com> * Finish incomplete merge * Only make variadic params optional if no other tags are specified * remove print statements * Update annotated.go Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com> * add extra test * Fix test "Provide variadic function with no optional params" * Explain why we're adding the optional tag * Fix test case, verified failing on master Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com> Co-authored-by: Sung Yoon Whang <sungyoon@uber.com> Co-authored-by: Abhinav Gupta <mail@abhinavg.net> Co-authored-by: Abhinav Gupta <abg@uber.com>
This introduces the concept of Dig.Scope, which is a scoped container for modifications to the dependency graph. A Scope may have one or more Scopes as children. A Scope also has one parent Scope, except for
Container
, which is the root Scope. This change is necessary before we can start working on APIs for graph modifications and decorations.Things to note:
Provide
to a Scope means it will be made available to itself and any of its descendent Scopes (More on this later).Container
, which is the existing API for DI container, is the just the rootScope
.To make things easier to review, I have broken down the PRs into few parts:
Dig.Scope
type (this PR)Provide
s to aScope
to be propagated to all the Scopes without having to touch the root Scope.Decorate
method to Dig.Scope, which lets you decorate a type provided to aScope
.Notes on Implementation:
Each
Scope
struct keeps track of the following things:constructorNode
s,providers
,groups
, andvalues
all track things that were provided directly to this Scope. It does not contain anything that was provided by its parent, child, or anything in its ancestry tree. This is so that we can avoid having to copy over these every single time a new Scope is created, which is expensive in terms of memory and computation.gh
still holds all the nodes in the entire dependency graph for nodes that impact this Scope. This is so that we can avoid re-computing the dependency graph every single time a graph algorithm needs to be run.Ref: GO-939