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 @AssistedInject objects to be injected and provided by @Provides methods #2370

Comments

@bubenheimer
Copy link

bubenheimer commented Feb 9, 2021

I got some singleton/reusable, expensive-to-create-and-update key-value/preference objects in my app that I create by calling factory methods from @Provides methods. I'd think it's a common and valid pattern, but not possible with @AssistedInject. I can work around it with ease, but every time I come across my hand-coded factory I wonder why it has to be there. I have to add code comments to explain and defend the design flaw.

This is what I'd like to do:

class ValueCache @AssistedInject constructor(repository: Repository, @Assisted key: String) {
    @AssistedFactory interface Factory {
        fun create(key: String): ValueCache
    }
}

// In some module...    
@Provides @Reusable @Named("LikesRed") fun provideUserLikesRedCache(cacheFactory: ValueCache.Factory):
    ValueCache = cacheFactory.create("userLikesRed")

@Provides @Reusable @Named("LikesGreen") fun provideUserLikesGreenCache(cacheFactory: ValueCache.Factory):
    ValueCache = cacheFactory.create("userLikesGreen")

Dagger does not support providing @AssistedInject types

Dagger does not support injecting @AssistedInject type

If I understand correctly, it's essentially an arbitrary restriction in this specific case to dumb things down for inexperienced users in the general case. It violates API orthogonality in the specific case. There are various precursors in Dagger doing similar handholding restriction things, and they are generally a hard pill to swallow for an experienced user wanting to write clean, low-boilerplate code; I wish Dagger was generally maintained with less bias toward disallowing things.

A flag to disable the restriction could perhaps address the issue while maintaining the restriction in the general case, like an annotation parameter: @AssistedInject(allowInjection=true). Or a compile flag to disable the restriction in general, at the cost of letting me shoot myself in the foot. I prefer that, though, over having cluttered code.

@bubenheimer bubenheimer changed the title Allow @AssistedInject objects to be provided from @Provides methods Allow @AssistedInject objects to be injected and provided by @Provides methods Feb 9, 2021
@bcorso
Copy link

bcorso commented Feb 9, 2021

@bubenheimer thanks for the feature request!

We have plans to support providing/injecting qualified assisted inject types, which looks like is your use case, it's just not implemented yet.

(However, we don't have any plan on supporting the general case of providing non-qualified assisted inject types)

@bubenheimer
Copy link
Author

Thank you, I think that will address my use cases!

@TevJ
Copy link

TevJ commented Feb 10, 2021

I also experienced this issue when attempting to migrate from Jake's original AssistedInject library which allowed this pattern to the Dagger integrated version. Good to know a fix is coming, thanks!

copybara-service bot pushed a commit that referenced this issue Feb 18, 2021
Allows qualified @AssistedInject/@AssistedFactory types to be provided and injected as normal bindings.

Fixes #2370

RELNOTES=Fixes #2370: Qualified @AssistedInject/@AssistedFactory types can now be provided and injected as normal bindings.
PiperOrigin-RevId: 357652329
@bubenheimer
Copy link
Author

As it turns out the fix in Dagger 2.33 does not actually work for most of my use cases. I often prefer custom types to @Named annotations, so no go due to no annotation. I cannot justify adding annotations for the sake of having auto-generated factories. These use cases need the @AssistedFactory restrictions to be further relaxed.

@bcorso
Copy link

bcorso commented Feb 26, 2021

I often prefer custom types to @Named annotations, so no go due to no annotation.

@bubenheimer do you have an example of what you're trying to do?

@bubenheimer
Copy link
Author

bubenheimer commented Feb 26, 2021

Modifying my original example for custom types; there would also be type-specific adapters for reading values in the mix, but that's just an implementation detail:

class ValueCache<T : Any> @AssistedInject constructor(repository: Repository, @Assisted key: String) {
    @AssistedFactory interface Factory<T : Any> {
        fun create(key: String): ValueCache<T>
    }

    // and also...

    @AssistedFactory interface AltFactory {
        fun <T : Any> create(key: String): ValueCache<T>
    }
}

// In some module...    
@Provides @Reusable fun provideCache(cacheFactory: ValueCache.Factory<ColorPreference>):
    ValueCache<ColorPreference> = cacheFactory.create("preferredUserColor")

enum class ColorPreference { RED, YELLOW, GREEN }

@bcorso
Copy link

bcorso commented Feb 27, 2021

I cannot justify adding annotations for the sake of having auto-generated factories

Are you thinking you would need to create a new @Named for each type?

Just to clarify, a single @Default qualifier would be enough to handle all types, e.g. @Default ValueCache<ColorPreference>, @Default ValueCache<SomeOtherThing>, etc.

I do understand that adding a qualifier may seem unnecessary, but we do believe it's best to avoid confusion when injecting an @AssistedInject type, and making this work without qualifiers is not something we currently plan on supporting.

@bubenheimer
Copy link
Author

Adding an artificial annotation at every injection site is polluting, and easily missed; this is one benefit of custom types, less pollution, fewer errors. Writing a custom factory is a thousand times preferable over carrying around nonsensical annotations. It hurts my brain to consider such a thing.

You got the use case. You got the blueprint library behavior in front of you. You got more practical frameworks taking over. I guess that's not enough to change Dagger's ways.

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