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

WIP provide params to invoke #215

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rubensayshi
Copy link

Would it be an idea to let Invoke take arguments that aren't coming from the container?

It seems rather straight forward to add this, which leads me to belief it's not in there for a (good) reason...?

I understand this PR would break BC and it would be better to have a new method, but opening the pR atm purely for discussion as it's also missing any tests atm so it's not ready to be merged anyway.
If there's interest in merging it then I'll add InvokeWithParam or smt and add tests.

would be used something like;

// my super complicated service
type MyService struct {
	Booyaa bool
}

// provide for my super complicated service
func NewMyService() *MyService {
	return &MyService{}
}

func CanWeBooyaa(ctx *gin.Context, service *MyService) {
	ctx.String(200, fmt.Sprintf("booyaa? %v! \n", service.Booyaa))
}

container := dig.New()
container.Provide(NewMyService)

ctx := &gin.Context{}
container.Invoke(CanWeBooyaa, ctx)

@CLAassistant
Copy link

CLAassistant commented Oct 3, 2018

CLA assistant check
All committers have signed the CLA.

@rubensayshi
Copy link
Author

BAD FOR: Exposing to user-land code as a Service Locator.

I noticed this in the README, though I don't really understand why dig would be bad for this nor is there anything I can find in the go space that would be better for it.

@abhinav
Copy link
Collaborator

abhinav commented Oct 3, 2018

Responded about the feature itself in #214.

BAD FOR: Exposing to user-land code as a Service Locator.

I noticed this in the README, though I don't really understand why dig would be bad for this nor is there anything I can find in the go space that would be better for it.

The pattern we want to discourage is making the container available to the
application and using it as a bag of things you can get on demand.

Something like,

func NewThing(c *dig.Container) *Thing {
    var t Thing
    c.Invoke(func(logger *Logger, metrics *Metrics) {
        t.logger = logger
        t.metrics = metrics
    })
    return &t
}

One of the major disadvantages of something like this is that your
dependencies aren't explicit. It's no better than using a
map[string]interface{} to pass parameters around. If I'm testing NewThing,
it's not clear exactly what objects are needed to test it. I have to read the
source code and build a container with the right objects in it.

Further, that's just a step removed from something like,

func NewHandler(c *dig.Container) *Handler {
    return &Handler{c: c}
}

func (h *Handler) HandleRequest(...) {
    h.c.Invoke(func(logger Logger) {
        ...
    })
}

Which, in addition to having the same problems as the other case, is going to
be significantly slower. We definitely don't want the container used on the
request path.

@rubensayshi
Copy link
Author

you're saying significantly slower, but benchmarking dig shows its ~1500ns for a provide, that's not that bad ...

@glibsm
Copy link
Collaborator

glibsm commented Oct 4, 2018

you're saying significantly slower, but benchmarking dig shows its ~1500ns for a provide, that's not that bad ...

Provide is not the only thing that needs to be benched, it's also the Invoke's. Dig is implemented using reflection and the general rule of thumb is "no reflection in the hot path".

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