uber-go / dig

A reflection based dependency injection toolkit for Go.
https://go.uber.org/dig
MIT License
3.78k stars 206 forks source link

Intercept provided values #408

Open asmeikal opened 4 months ago

asmeikal commented 4 months ago

Describe the solution you'd like We need to intercept the provided values after they are constructed, to check for implemented interfaces and possibly call some initialization methods.

We have an implementation ready that adds a WithProvidedCallback option to the container that serves this purpose. The callback will receive all values after they are constructed, along with name/group information and type information.

Describe alternatives you've considered This could be implemented as a ProvideOption, but would need a list of values as input of the callback, since constructors can return multiple values, and would be duplicated each time Provide is called on the container.

The callback could be implemented also as a ScopeOption to override behaviour in nested scopes, but since it would be the first actual option of this kind, we skipped implementation for now.

Is this a breaking change? No, even if the callback is implemented as a ScopeOption.

Additional context We are open sourcing an extremly opinionated dependency injection framework we use to develop microservices in Go. This (small) framework handles automatically health check registration, calling initialization and destruction methods at service startup and destruction, registration of prometheus metrics. Instead of relying on lifecycle hooks like Fx does, our framework relies on interfaces implemented by the provided values, e.g. by looking for the Start method on provided values and invoking it at application startup. This reduces code duplication, as most of this shared logic can be implemented with single methods on each component.

JacobOaks commented 4 months ago

Hi @asmeikal, thanks for the issue & PR.

Dig already supports a callback system where you can register dig.Callback using dig.WithProviderCallback and dig.WithDecoratorCallback.

What are your thoughts on extending this system to support the needs you described? It seems like, specifically, you'd like:

I think both of these things can be accomplished in a backwards-compatible way without creating a second callback system. What do you think?

asmeikal commented 4 months ago

Hi @JacobOaks, as mentioned in the initial message I wouldn't rely on the existing options, since they are registered at the provider level and would result in a strange interface.

If you are suggesting instead to rely on the dig.Callback for this new option, I'm not sure how to add the required information in the dig.CallbackInfo struct, since it contains information about the provider or decorator invocation, and the feature we are looking for is to get information on the constructed values instead.

I'm having these doubts, specifically:

JacobOaks commented 4 months ago

Hey @asmeikal!

I'd like to hear what other maintainers think, but I'm not necessarily opposed to creating a top-level fx.Option API that sets a default callback for an app. I'm also not opposed to adding the constructed values into CallbackInfo as slices of types and values. I think that would be generally helpful and not that hard to do. I would be hesitant to have an entirely separate, second callback system. That seems like it will get messy and confusing quick.

As for CallbackInfo containing an error, if you don't want to handle that case in your callback, is there a reason you can't simply return early if err != nil?

tchung1118 commented 4 months ago

Before we add a whole new top-level API for registering a global callback for all providers, could you consider adding the constructed values into CallbackInfo? I think it would be a more contained change than what you're proposing and could still satisfy your requirements. For dealing with providers returning multiple results, I would suggest using a slice of reflect.Values, instead of tuples of reflect.Type and reflect.Value. You can still get reflect.Type from reflect.Value if you need to via https://pkg.go.dev/reflect#Value.Type.

asmeikal commented 4 months ago

Hi @tchung1118, thank you and @JacobOaks both for the feedback. I updated the PR to rely on the existing CallbackInfo struct, which now contains a slice of reflect.Values for all the values constructed by the function, following the suggestion from @tchung1118. I also updated the tests for the provider and decorator callbacks, that receive the constructed values too. I'm still adding a new option at the container level, to intercept all constructed values.

JacobOaks commented 4 months ago

Hey @asmeikal thanks for the update! This looks like a step in the right direction.

I do think we should stray away from having a separately stored callback at the scope level though unless there is really strong justification for it as I think it will introduce extra complexity and confusion around callbacks in general. You mention that your use-case is to build a DI framework on top of dig. Is there any reason this framework can't insert the callback w/ every call to dig.Provide and dig.Decorate? Fx for example does this exact thing: https://github.com/uber-go/fx/blob/6ddbd0359be94efe0d729b6799a942936d4374b4/module.go#L198.

Let me know your thoughts!

asmeikal commented 4 months ago

@JacobOaks yeah you're right! I updated the PR by removing the container callback.