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

Allow name and group Result Tags #998

Open
mbolt35 opened this issue Dec 10, 2022 · 3 comments
Open

Allow name and group Result Tags #998

mbolt35 opened this issue Dec 10, 2022 · 3 comments

Comments

@mbolt35
Copy link

mbolt35 commented Dec 10, 2022

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

sywhang commented Feb 14, 2023

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: #1036.

@jquirke
Copy link

jquirke commented Mar 5, 2023

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?

`

jquirke added a commit to jquirke/dig that referenced this issue Mar 6, 2023
As per Dig issue:
uber-go#380

In order to support Fx feature requests

uber-go/fx#998
uber-go/fx#1036

We need to be able to drop the restriction, both in terms of options
dig.Name and dig.Group and dig.Out struct annotations on `name` and
`group` being mutually exclusive.

In a future PR, this can then be exploited to populate value group maps
where the 'name' tag becomes the key of a map[string][T]
jquirke added a commit to jquirke/dig that referenced this issue Mar 6, 2023
As per Dig issue:
uber-go#380

In order to support Fx feature requests

uber-go/fx#998
uber-go/fx#1036

We need to be able to drop the restriction, both in terms of options
dig.Name and dig.Group and dig.Out struct annotations on `name` and
`group` being mutually exclusive.

In a future PR, this can then be exploited to populate value group maps
where the 'name' tag becomes the key of a map[string][T]
jquirke added a commit to jquirke/dig that referenced this issue Mar 6, 2023
As per Dig issue:
uber-go#380

In order to support Fx feature requests

uber-go/fx#998
uber-go/fx#1036

We need to be able to drop the restriction, both in terms of options
dig.Name and dig.Group and dig.Out struct annotations on `name` and
`group` being mutually exclusive.

In a future PR, this can then be exploited to populate value group maps
where the 'name' tag becomes the key of a map[string][T]
jquirke added a commit to jquirke/dig that referenced this issue Mar 6, 2023
As per Dig issue:
uber-go#380

In order to support Fx feature requests

uber-go/fx#998
uber-go/fx#1036

We need to be able to drop the restriction, both in terms of options
dig.Name and dig.Group and dig.Out struct annotations on `name` and
`group` being mutually exclusive.

In a future PR, this can then be exploited to populate value group maps
where the 'name' tag becomes the key of a map[string][T]
jquirke added a commit to jquirke/dig that referenced this issue Mar 7, 2023
As part of uber-go#380 we allowed names
and groups tags/options to co-exist to ultimately support Fx feature
request uber-go/fx#998.

We now intend to support Map value groups as per
uber-go/fx#1036. We will do this in 2 steps.

1. This PR will begin tracking any names passed into value groups with
out changing any external facing functionality.

2. a subsequent PR will exploit this structure to support Map value
groups.
@mbolt35
Copy link
Author

mbolt35 commented Mar 7, 2023

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!

jquirke added a commit to jquirke/dig that referenced this issue Mar 9, 2023
As per Dig issue:
uber-go#380

In order to support Fx feature requests

uber-go/fx#998
uber-go/fx#1036

We need to be able to drop the restriction, both in terms of options
dig.Name and dig.Group and dig.Out struct annotations on `name` and
`group` being mutually exclusive.

In a future PR, this can then be exploited to populate value group maps
where the 'name' tag becomes the key of a map[string][T]
jquirke added a commit to jquirke/dig that referenced this issue Mar 9, 2023
As part of uber-go#380 we allowed names
and groups tags/options to co-exist to ultimately support Fx feature
request uber-go/fx#998.

We now intend to support Map value groups as per
uber-go/fx#1036. We will do this in 2 steps.

1. This PR will begin tracking any names passed into value groups with
out changing any external facing functionality.

2. a subsequent PR will exploit this structure to support Map value
groups.
jquirke added a commit to jquirke/dig that referenced this issue Mar 9, 2023
As per Dig issue:
uber-go#380

In order to support Fx feature requests

uber-go/fx#998
uber-go/fx#1036

We need to be able to drop the restriction, both in terms of options
dig.Name and dig.Group and dig.Out struct annotations on `name` and
`group` being mutually exclusive.

In a future PR, this can then be exploited to populate value group maps
where the 'name' tag becomes the key of a map[string][T]
jquirke added a commit to jquirke/dig that referenced this issue Mar 9, 2023
As part of uber-go#380 we allowed names
and groups tags/options to co-exist to ultimately support Fx feature
request uber-go/fx#998.

We now intend to support Map value groups as per
uber-go/fx#1036. We will do this in 2 steps.

1. This PR will begin tracking any names passed into value groups with
out changing any external facing functionality.

2. a subsequent PR will exploit this structure to support Map value
groups.
jquirke added a commit to jquirke/dig that referenced this issue Mar 9, 2023
As per Dig issue:
uber-go#380

In order to support Fx feature requests

uber-go/fx#998
uber-go/fx#1036

We need to be able to drop the restriction, both in terms of options
dig.Name and dig.Group and dig.Out struct annotations on `name` and
`group` being mutually exclusive.

In a future PR, this can then be exploited to populate value group maps
where the 'name' tag becomes the key of a map[string][T]
jquirke added a commit to jquirke/dig that referenced this issue Mar 9, 2023
As part of uber-go#380 we allowed names
and groups tags/options to co-exist to ultimately support Fx feature
request uber-go/fx#998.

We now intend to support Map value groups as per
uber-go/fx#1036. We will do this in 2 steps.

1. This PR will begin tracking any names passed into value groups with
out changing any external facing functionality.

2. a subsequent PR will exploit this structure to support Map value
groups.
jquirke added a commit to jquirke/dig that referenced this issue Mar 9, 2023
As per Dig issue:
uber-go#380

In order to support Fx feature requests

uber-go/fx#998
uber-go/fx#1036

We need to be able to drop the restriction, both in terms of options
dig.Name and dig.Group and dig.Out struct annotations on `name` and
`group` being mutually exclusive.

In a future PR, this can then be exploited to populate value group maps
where the 'name' tag becomes the key of a map[string][T]
jquirke added a commit to jquirke/dig that referenced this issue Mar 9, 2023
As part of uber-go#380 we allowed names
and groups tags/options to co-exist to ultimately support Fx feature
request uber-go/fx#998.

We now intend to support Map value groups as per
uber-go/fx#1036. We will do this in 2 steps.

1. This PR will begin tracking any names passed into value groups with
out changing any external facing functionality.

2. a subsequent PR will exploit this structure to support Map value
groups.
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