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

Intercept provided values #408

Open
asmeikal opened this issue Mar 4, 2024 · 7 comments
Open

Intercept provided values #408

asmeikal opened this issue Mar 4, 2024 · 7 comments

Comments

@asmeikal
Copy link

asmeikal commented Mar 4, 2024

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
Copy link
Contributor

JacobOaks commented Mar 4, 2024

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:

  • To be able to specify a default callback once for an entire Fx application
  • Some additional information to get passed to callbacks.

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
Copy link
Author

asmeikal commented Mar 5, 2024

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:

  • the dig.CallbackInfo can contain an error, but for this feature it would never be invoked with an error, since we are only interested in what values are actually constructed
  • we must add information on the constructed value inside the dig.CallbackInfo struct. For providers that return multiple values, there are two choices, and both seem a bit forced to me:
    • invoke the callback multiple times, one for each constructed value, passing it the reflect.Type and reflect.Value of each constructed value
    • invoke the callback once, passing it an array of the reflect.Type and reflect.Value couples for each constructed value

@JacobOaks
Copy link
Contributor

JacobOaks commented Mar 5, 2024

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
Copy link
Contributor

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
Copy link
Author

asmeikal commented Mar 6, 2024

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
Copy link
Contributor

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
Copy link
Author

asmeikal commented Mar 7, 2024

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

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

No branches or pull requests

3 participants