Allow name and group Result Tags

Question

Allow name and group Result Tags

mbolt35 opened this issue a year ago · 3 comments

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!

Answer 1 · 2023-02-14T19:07:04.000Z

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