Skip to content
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

Implement struct fx.Annotated to inject named values without relying on fx.Out #633

Closed
wants to merge 1 commit into from

Conversation

dimiro1
Copy link

@dimiro1 dimiro1 commented Jul 20, 2018

See: #610

This is a version for review, there are a few things I would like to receive feedback.

  • I am not familiar with the codebase, so, I am not sure if the implementation in the *app is the best possible implementation;
  • I needed to update dig to use master, because of the dig.Name was not released yet;
  • I still need to add documentation;
  • There are a couple of questions, how to deal with Optional dependencies? and How to implement groups?

@CLAassistant
Copy link

CLAassistant commented Jul 20, 2018

CLA assistant check
All committers have signed the CLA.

@abhinav
Copy link
Collaborator

abhinav commented Jul 24, 2018

Hello and thanks for the PR! This is a good start.

To answer your questions (in-order):

  • Doing this in *app is fine for now. If we end up adding another type like
    Annotated, then we would probably move part of the logic into methods on
    those types.

  • That's alright. We can tag a release when this is ready and re-pin.

  • :)

  • We don't need to worry about optionals with this. You can't provide
    something as optional, you can only depend on other things as optional,
    which is still possible by having you constructor consume an fx.In.

    For groups, we'll add a dig.Grouped or similar API to dig. IMO that's not
    a blocker for a v1 of fx.Annotated.

),
fx.Invoke(func(in in) {
assert.NotNil(t, in.A, "expected in.A to be injected")
assert.Equal(t, "foo", in.A.name, "expected to get a type 'a' of name 'foo'")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of putting assertions in the Invoke, consider moving them out with
the use of an fx.Populate.

var result struct {
    fx.In
    
    A *a `name:"foo"`
}

app := fxtest.New(t,
    ...,
    fx.Populate(&result),
)
defer app.RequireStart().RequireStop()

assert.[..]

Otherwise, if we have a bug that neglects to call that fx.Invoke, the
assertion will be ignored and the test will still pass.

@@ -404,6 +404,13 @@ func (app *App) provide(constructor interface{}) {
return
}

if a, ok := constructor.(Annotated); ok {
if err := app.container.Provide(a.Fn, dig.Name(a.Name)); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the case where the constructor is just a function, we should probably
verify that we're not producing fx.Annotateds into the container. It would
be easy for someone to assume that they can do func() fx.Annotated to get
the same behavior so we can catch that now and give them a better error
message.

You should be able to adapt some of the code in "internal/fxreflect".ReturnTypes
to get a list of reflect.Types being provided by the constructor and check
that one of them isn't reflect.TypeOf(Annotated{}). (The code in that
package has some duplication with dig; we plan on exposing APIs in dig to
reduce that in the future.)

@dimiro1
Copy link
Author

dimiro1 commented Jul 27, 2018

Thanks for your feedback @abhinav. Next week I will have time to work on it again.

@glibsm
Copy link
Collaborator

glibsm commented Oct 10, 2018

@dimiro1 Looks like this PR has stalled.

Are you able to pick this back up at some point, or this kind of functionality is no longer desired for your use case?

@dimiro1
Copy link
Author

dimiro1 commented Oct 12, 2018

Hello @glibsm, sorry for that. I was planning to work on the PR but I could not find time to work on it. Feel free to close the PR.

@abhinav
Copy link
Collaborator

abhinav commented Oct 12, 2018

@dimiro1 Thanks for the contribution nonetheless. We have a good idea of the
direction for this feature now and this PR will be helpful as soon as someone
has a chance to work on it.

@glibsm
Copy link
Collaborator

glibsm commented Nov 7, 2018

Superseeded by #656

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants