-
Notifications
You must be signed in to change notification settings - Fork 345
Add support for func-types to fx.As()
#1249
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
ed3ecc8
7e64651
b7b1714
a898732
bc9a41a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ import ( | |
| "strings" | ||
|
|
||
| "go.uber.org/dig" | ||
|
|
||
| "go.uber.org/fx/internal/fxreflect" | ||
| ) | ||
|
|
||
|
|
@@ -1145,6 +1146,19 @@ var _ Annotation = (*asAnnotation)(nil) | |
| // constructor does NOT provide both bytes.Buffer and io.Writer type; it just | ||
| // provides io.Writer type. | ||
| // | ||
| // Example for function-types: | ||
| // | ||
| // type domainHandler func(ctx context.Context) error | ||
| // | ||
| // func anyHandlerProvider() func(ctx context.Context) error { | ||
| // ... | ||
| // } | ||
| // | ||
| // fx.Provider( | ||
| // anyHandlerProvider(), | ||
| // fx.As(new(domainHandler)), | ||
| // ) | ||
| // | ||
| // When multiple values are returned by the annotated function, each type | ||
| // gets mapped to corresponding positional result of the annotated function. | ||
| // | ||
|
|
@@ -1211,8 +1225,8 @@ func (at *asAnnotation) apply(ann *annotated) error { | |
| continue | ||
| } | ||
| t := reflect.TypeOf(typ) | ||
| if t.Kind() != reflect.Ptr || t.Elem().Kind() != reflect.Interface { | ||
| return fmt.Errorf("fx.As: argument must be a pointer to an interface: got %v", t) | ||
| if t.Kind() != reflect.Ptr || !(t.Elem().Kind() == reflect.Interface || t.Elem().Kind() == reflect.Func) { | ||
| return fmt.Errorf("fx.As: argument must be a pointer to an interface or function: got %v", t) | ||
| } | ||
| t = t.Elem() | ||
| at.types[i] = asType{typ: t} | ||
|
|
@@ -1265,8 +1279,11 @@ func (at *asAnnotation) results(ann *annotated) ( | |
| continue | ||
| } | ||
|
|
||
| if !t.Implements(at.types[i].typ) { | ||
| return nil, nil, fmt.Errorf("invalid fx.As: %v does not implement %v", t, at.types[i]) | ||
| if !((at.types[i].typ.Kind() == reflect.Interface && t.Implements(at.types[i].typ)) || | ||
| t.ConvertibleTo(at.types[i].typ)) { | ||
| return nil, | ||
| nil, | ||
| fmt.Errorf("invalid fx.As: %v does not implement or is not convertible to %v", t, at.types[i]) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we make these two separate checks? This will keep the code cleaner and give better error messages. if at.types[i].typ.Kind() == reflect.Interface {
if !t.Implements(at.types[i].typ) {
return nil, nil, fmt.Errorf("invalid fx.As: %v does not implement %v", t, at.types[i])
}
} else if !t.ConvertibleTo(at.types[i].typ) {
return nil, nil, fmt.Errorf("invalid fx.As: %v cannot be converted to %v", t, at.types[i])
}
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thx, I split this condition |
||
| } | ||
| field.Type = at.types[i].typ | ||
| fields = append(fields, field) | ||
|
|
@@ -1300,7 +1317,11 @@ func (at *asAnnotation) results(ann *annotated) ( | |
|
|
||
| newOutResult := reflect.New(resType).Elem() | ||
| for i := 1; i < resType.NumField(); i++ { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any reason we would need to check |
||
| newOutResult.Field(i).Set(getResult(i, results)) | ||
| if newOutResult.Field(i).Kind() == reflect.Func { | ||
| newOutResult.Field(i).Set(getResult(i, results).Convert(newOutResult.Field(i).Type())) | ||
| } else { | ||
| newOutResult.Field(i).Set(getResult(i, results)) | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: should we flip these conditions to make the control flow more/appear consistent with the check above? if newOutResult.Field(i).Kind() == reflect.Interface {
newOutResult.Field(i).Set(getResult(i, results))
} else {
newOutResult.Field(i).Set(getResult(i, results).Convert(newOutResult.Field(i).Type()))
}
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
| } | ||
| outResults = append(outResults, newOutResult) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -33,6 +33,7 @@ import ( | |||||
|
|
||||||
| "github.com/stretchr/testify/assert" | ||||||
| "github.com/stretchr/testify/require" | ||||||
|
|
||||||
| "go.uber.org/fx" | ||||||
| "go.uber.org/fx/fxevent" | ||||||
| "go.uber.org/fx/fxtest" | ||||||
|
|
@@ -442,6 +443,8 @@ func TestAnnotatedAs(t *testing.T) { | |||||
| type myStringer interface { | ||||||
| String() string | ||||||
| } | ||||||
| type myProvideFunc func() string | ||||||
| type myInvokeFunc func() string | ||||||
|
|
||||||
| newAsStringer := func() *asStringer { | ||||||
| return &asStringer{ | ||||||
|
|
@@ -477,6 +480,32 @@ func TestAnnotatedAs(t *testing.T) { | |||||
| assert.Equal(t, s.String(), "another stringer") | ||||||
| }, | ||||||
| }, | ||||||
| { | ||||||
| desc: "value type convertible to target type", | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: consider downscoping the names of these cases since convertible types are locked down to just functions right now
Suggested change
|
||||||
| provide: fx.Provide( | ||||||
| fx.Annotate(func() myProvideFunc { | ||||||
| return func() string { | ||||||
| return "provide func example" | ||||||
| } | ||||||
| }, fx.As(new(myInvokeFunc))), | ||||||
| ), | ||||||
| invoke: func(h myInvokeFunc) { | ||||||
| assert.Equal(t, "provide func example", h()) | ||||||
| }, | ||||||
| }, | ||||||
| { | ||||||
| desc: "anonymous value type convertible to target type", | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we add a test case for a function vaoue that is not convertible to the target type?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thx, I added new test case. |
||||||
| provide: fx.Provide( | ||||||
| fx.Annotate(func() func() string { | ||||||
| return func() string { | ||||||
| return "anonymous func example" | ||||||
| } | ||||||
| }, fx.As(new(myInvokeFunc))), | ||||||
| ), | ||||||
| invoke: func(h myInvokeFunc) { | ||||||
| assert.Equal(t, "anonymous func example", h()) | ||||||
| }, | ||||||
| }, | ||||||
| { | ||||||
| desc: "provide with multiple types As", | ||||||
| provide: fx.Provide(fx.Annotate(func() (*asStringer, *bytes.Buffer) { | ||||||
|
|
@@ -1806,9 +1835,9 @@ func TestAnnotateApplySuccess(t *testing.T) { | |||||
| func assertApp( | ||||||
| t *testing.T, | ||||||
| app interface { | ||||||
| Start(context.Context) error | ||||||
| Stop(context.Context) error | ||||||
| }, | ||||||
| Start(context.Context) error | ||||||
| Stop(context.Context) error | ||||||
| }, | ||||||
| started *bool, | ||||||
| stopped *bool, | ||||||
| invoked *bool, | ||||||
|
|
||||||
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.
Let's update line 1129 as well:
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.
Thx, appropriate changes have been made.