uber-go / fx

A dependency injection based application framework for Go.
https://uber-go.github.io/fx/
MIT License
5.89k stars 291 forks source link

Allow name and group Result Tags #998

Open mbolt35 opened 1 year ago

mbolt35 commented 1 year ago

Is your feature request related to a problem? Please describe.

This is a documented limitation, but it's somewhat unclear why such a limitation exists:

In the Annotated struct:

// ...
// A name option may not be provided if a group option is provided.
Name string

// ...
// A group option may not be provided if a name option is provided.
Group string

It seems reasonable that a constructor's output could be both annotated with a name as well as exist within a group.

Describe the solution you'd like A clear and concise description of what you want to happen.

The ability to use a single tag, specifying name and group, ie:

fx.Provide(
    fx.Annotate(
        NewMemoryStorageStrategy,
        fx.As(new(StorageStrategy)),
        fx.ResultTags(`name:"memory" group:"storage"`),
    ),
    fx.Annotate(
        NewFileStorageStrategy,
        fx.As(new(StorageStrategy)),
        fx.ResultTags(`name:"file" group:"storage"`),
    ),
)

Which would allow for injection of a group of StorageStrategies:

type StorageExpirationMonitor struct {
    strategies []StorageStrategy 
} 

func NewStorageExpirationMonitor(strats []StorageStrategy) *StorageExpirationMonitor {
    return &StorageExpirationMonitor{
        strategies: strats,
    }
}

func (smm *StorageExpirationMonitor) ExpireAll() {
    for _, s := range smm.strategies {
        fmt.Printf("Expiring: %T\n", s)
    }
}

fx.Provide(
    // ...
    fx.Annotate(
        NewStorageExpirationMonitor,
        fx.ParamTags(`group:"storage"`),
    ),
    // ...
)

while also being able to control (at the module) which StorageStrategy is used for other implementations, ie:

type ResultCache struct {
    storage StorageStrategy 
}

func NewResultCache(storage StorageStrategy) *ResultCache {
    return &ResultCache{
        storage: storage,
    }
}

fx.Provide(
    // ...
    fx.Annotate(
        NewResultCache,
        fx.ParamTags(`name:"file"`), // perhaps we have a separate test module that leverages name:"memory"
    ),
    // ...
)

Of course, it's possible there may be a major problem case with allowing this functionality that I am missing. Apologies if that is the case.

Describe alternatives you've considered A clear and concise description of any alternative solutions or features you've considered.

This type of functionality could be implemented on the user side by adding a StorageType() string func to StorageStrategy which could be used to collect all of the StorageStrategy instances in a manager type (using group:"storage"), and having the manager provide a ByStorageType(string) StorageStrategy method. Then inject the "manager" into any implementations that require a specific strategy type. ie: Create the "Provider" ourselves and write a simple scheme for retrieving instances by index/type.

So, this isn't a must-have issue, and these type scenarios aren't too frequent, but there are some conveniences this feature would provide that would improve quality of life.

Is this a breaking change? We do not accept breaking changes to the existing API. Please consider if your proposed solution is backwards compatible. If not, we can help you make it backwards compatible, but this must be considered when we consider new features.

Since this appears to be an intentional restriction, I assume that allowing this functionality would be backwards compatible.

Additional context Add any other context or screenshots about the feature request here.

Thank you for an amazing open source project!

sywhang commented 1 year ago

Thank you for the detailed feature request. We've actually received a similar feature request internally within Uber and have started tracking this work.

See also: https://github.com/uber-go/fx/issues/1036.

jquirke commented 1 year ago

Just curious, why can't you leverage output structures?


type Result struct {
    fx.Out 

    FileStorageStrategy FileStorageStrategy `name:"file"`
    FileStorageStrategyGroup  FileStorageStrategy`group:"storage"`

}

Since this is rather awkward, and the limitation is in the dig Container, a short term solution is that, fx.ResultTags() is "syntactic sugar" over the dig container that reflectively generates a wrapper function that remaps the results in a dig.Out structure, perhaps it could take care of this problem?

`
mbolt35 commented 1 year ago

Just curious, why can't you leverage output structures?

We could absolutely leverage a work-around method here without too much of a problem. In general, I've found that using the ResultTags is a bit more straight forward for our current workflow, but again, definitely not a show stopper by any means. Really appreciate the help here, thanks again for a great open source project!